> 2363 [NNNNN] WARNING: Subdocument container has non-subdocument frame: file layout/base/nsDocumentViewer.cpp, line 2505 This warning [1] shows up in the following test suites: > mozilla-central_ubuntu64_vm-debug_test-reftest-e10s-1-bm68-tests1-linux64-build0.txt:759 > mozilla-central_ubuntu64_vm-debug_test-reftest-e10s-2-bm123-tests1-linux64-build2.txt:545 > mozilla-central_ubuntu64_vm-debug_test-reftest-3-bm52-tests1-linux64-build0.txt:535 > mozilla-central_ubuntu64_vm-debug_test-reftest-1-bm54-tests1-linux64-build2.txt:405 > mozilla-central_ubuntu64_vm-debug_test-mochitest-1-bm117-tests1-linux64-build1.txt:39 > mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-1-bm123-tests1-linux64-build25.txt:37 > mozilla-central_ubuntu64_vm-debug_test-reftest-4-bm121-tests1-linux64-build1.txt:12 > mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-3-bm51-tests1-linux64-build0.txt:5 > mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-2-bm53-tests1-linux64-build12.txt:5 > mozilla-central_ubuntu64_vm-debug_test-mochitest-3-bm115-tests1-linux64-build21.txt:5 > mozilla-central_ubuntu64_vm-debug_test-mochitest-2-bm67-tests1-linux64-build1.txt:5 > mozilla-central_ubuntu64_vm-debug_test-mochitest-other-bm53-tests1-linux64-build0.txt:2 > mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-5-bm52-tests1-linux64-build2.txt:2 > mozilla-central_ubuntu64_vm-debug_test-crashtest-bm53-tests1-linux64-build28.txt:2 > mozilla-central_ubuntu64_vm-debug_test-reftest-2-bm53-tests1-linux64-build27.txt:1 > mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-4-bm122-tests1-linux64-build12.txt:1 > mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-1-bm122-tests1-linux64-build19.txt:1 > mozilla-central_ubuntu64_vm-debug_test-mochitest-5-bm115-tests1-linux64-build29.txt:1 > mozilla-central_ubuntu64_vm-debug_test-mochitest-4-bm121-tests1-linux64-build0.txt:1 It shows up in 196 tests. A few of the most prevalent: > 40 - file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/img-width-slice-2.html > 40 - file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/img-width-slice-1.html > 40 - file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/img-width-meet-2.html > 40 - file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/img-width-meet-1.html > 40 - file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/img-width-meet-1.html > 40 - file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/img-widthAndHeight-slice-1.html > 40 - file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/img-novb-width-meet-1.html > 40 - file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/img-novb-height-meet-1.html > 40 - file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/img-height-meet-2.html > 40 - file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/img-height-meet-1.html Similar to bug 1175289 we may want to switch this to a LAYOUT_WARNING [1] https://hg.mozilla.org/mozilla-central/annotate/e786406bc683/layout/base/nsDocumentViewer.cpp#l2505

Switch from NS_WARNING to LAYOUT_WARNING just as we did in bug 1175289. Daniel, it looks like Mats is out for a while, would you mind reviewing?

I think this is really just a buggy warning (or perhaps a bug around the warning), which we should fix. First, one correction -- from my testing, the *testcases* quoted in comment 0 do *not* actually spam any copies of this warning. Rather, it's their *reference* cases (which contain a bunch of <embed> elements, aka subdocument containers) which are causing this spam. ANALYSIS: It looks like this is approximately what happens when the <embed> loads: (A) Each <embed> receives a call to "NS_NewEmptyFrame" to generate a "dummy" nsFrame while it's loading. (This nsFrame ends up being the "non-subdocument frame" that the warning is complaining about.) (B) When the <embed>'s content finishes loading, we get a call to RecreateFramesForContent, to rebuild it with a *real* nsSubDocumentFrame. ...and in between A and B, we get a call to nsObjectLoadingContent::OnStartRequest for the <embed> data, which makes us set up a nsDocShell and a nsDocumentViewer. And we call nsDocumentViewer::FindContainerView() during nsDocumentViewer::InitInternal, which checks whether we're in a nsSubDocumentFrame and warns because we're actually just in a nsFrame. BOTTOM LINE: I think we should just soften the warning to not warn if we've got a dummy nsFrame.

Comment on attachment 8634424 [details] [diff] [review] Switch subdocument container has non-subdocument frame warning to LAYOUT_WARNING So in particular: >+++ b/layout/base/nsDocumentViewer.cpp >@@ -2497,20 +2497,21 @@ nsDocumentViewer::FindContainerView() > if (subdocFrame->GetType() == nsGkAtoms::subDocumentFrame) { > NS_ASSERTION(subdocFrame->GetView(), "Subdoc frames must have views"); > nsView* innerView = > static_cast<nsSubDocumentFrame*>(subdocFrame)->EnsureInnerView(); > containerView = innerView; > } else { >- NS_WARNING("Subdocument container has non-subdocument frame"); >+ // XXX Silenced by default in bug 1183879 >+ LAYOUT_WARNING("Subdocument container has non-subdocument frame"); I think this really wants to be: NS_WARN_IF_FALSE(!subdocFrame->GetType(), "Subdocument container has non-subdocument frame"); (For these empty frames in (A) above, nsFrame::GetType() returns null; so the idea is, don't warn if we've got one of those frames, because it just means we're still loading.)

(bz, can you sanity-check me on comment 2 / comment 3? Backtrace of the warning is attached. In particular, I'm wondering if we should be doing less work here when we have a dummy nsFrame in the outer document. I'm not sure how much of this document-setup-work is useless at that point. If it's significantly useless, then maybe we want to keep the warning as-is and cut things off further up the stack.)

FWIW, we create the outer-document frames here (nsFrame vs. nsSubDocumentFrame) via this map in FindObjectData: > 3619 static const FrameConstructionDataByInt sObjectData[] = { > 3620 SIMPLE_INT_CREATE(nsIObjectLoadingContent::TYPE_LOADING, > 3621 NS_NewEmptyFrame), [...] > 3626 SIMPLE_INT_CREATE(nsIObjectLoadingContent::TYPE_DOCUMENT, > 3627 NS_NewSubDocumentFrame) http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=5d4b602cf88f&mark=3620-3621,3626-3627#3599 ...which is hooked up to <embed> via this line: > 3500 SIMPLE_TAG_CHAIN(embed, nsCSSFrameConstructor::FindObjectData),

> I'm wondering if we should be doing less work here when we have a dummy nsFrame in the > outer document. It looks like we're going to get null out of FindContainerView in nsDocumentViewer::InitInternal (which, btw, we should really move into the aDoCreation block, along with the containerView variable). aDoCreation is true, but aParentWidget is null in that stack trace, so null containerView means we skip creating a prescontext, presshell, etc (worth checking this by stepping through, I suppose). We do end up calling SetNewDocument on the window, but that's definitely needed. We skip InitPresentationStuff, since mPresContext is null at that point. So seems like loosening the warning as described in comment 3 makes perfect sense.

Thanks, bz! Sounds like comment 3 is indeed what we need here then.

With this patch I saw only 6 instances of the warning during linux64 debug testing: > 2 - dom/workers/test/serviceworkers/test_request_context_frame.html > 2 - dom/base/test/test_root_iframe.html > 1 - file:///builds/slave/test/build/tests/reftest/tests/docshell/base/crashtests/403574-1.xhtml > 1 - file:///builds/slave/test/build/tests/reftest/tests/js/xpconnect/crashtests/515726-1.html So I'd say we're good.

Comment on attachment 8635604 [details] [diff] [review] Switch subdocument container has non-subdocument frame warning to NS_WARN_IF_FALSE Looks good! Just one nit on the commit message: >Bug 1183879 - Switch subdocument container has non-subdocument frame warning to NS_WARN_IF_FALSE. r=dholbert This doesn't really get across what's actually changing. Something like this would be better: Bug 1183879 - Soften "non-subdocument frame" warning to also allow dummy nsFrames, which exist while subdocument is loading. r=dholbert r=me with the commit message clarified along those lines

url: https://hg.mozilla.org/integration/mozilla-inbound/rev/c4548d3923d1c3b1feecf727fcf60fab377ab698 changeset: c4548d3923d1c3b1feecf727fcf60fab377ab698 user: Eric Rahm <erahm@mozilla.com> date: Sun Jul 19 12:27:40 2015 -0700 description: Bug 1183879 - Soften "non-subdocument frame" warning to also allow dummy nsFrames, which exist while subdocument is loading. r=dholbert