Closed Bug 1183879 Opened 9 years ago Closed 9 years ago

2,400 instances of "Subdocument container has non-subdocument frame" emitted from layout/base/nsDocumentViewer.cpp during linux64 debug testing

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

> 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?
Attachment #8634424 - Flags: review?(dholbert)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
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.)
Attachment #8634424 - Flags: review?(dholbert)
Attached file backtrace of warning
(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.)
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
Thanks, bz! Sounds like comment 3 is indeed what we need here then.
Flags: needinfo?(erahm)
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.
Attachment #8635604 - Flags: review?(dholbert)
Attachment #8634424 - Attachment is obsolete: true
Flags: needinfo?(erahm)
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
Attachment #8635604 - Flags: review?(dholbert) → review+
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
https://hg.mozilla.org/mozilla-central/rev/c4548d3923d1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: