Closed Bug 1177819 Opened 4 years ago Closed 4 years ago

~9,000 instances of 'Someone passed native anonymous content directly into frame construction. Stop doing that!' emitted from layout/base/nsCSSFrameConstructor.cpp in linux64 debug tests

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: erahm, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

> 8929 [NNNNN] WARNING: Someone passed native anonymous content directly into frame construction.  Stop doing that!: file layout/base/nsCSSFrameConstructor.cpp, line 6559

This is the 4th most verbose warning [1] during linux64 debug testing. It primarily happens during mochitests:

> mozilla-central_ubuntu64_vm-debug_test-mochitest-2-bm120-tests1-linux64-build9.txt:5158
> mozilla-central_ubuntu64_vm-debug_test-mochitest-4-bm121-tests1-linux64-build6.txt:2394
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-3-bm117-tests1-linux64-build12.txt:517
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-4-bm117-tests1-linux64-build9.txt:378
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-2-bm117-tests1-linux64-build9.txt:177
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-1-bm117-tests1-linux64-build5.txt:149
> mozilla-central_ubuntu64_vm-debug_test-mochitest-other-bm117-tests1-linux64-build14.txt:64
> mozilla-central_ubuntu64_vm-debug_test-mochitest-1-bm116-tests1-linux64-build7.txt:26
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-5-bm51-tests1-linux64-build8.txt:22
> mozilla-central_ubuntu64_vm-debug_test-mochitest-5-bm121-tests1-linux64-build5.txt:22
> mozilla-central_ubuntu64_vm-debug_test-crashtest-bm117-tests1-linux64-build6.txt:17
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-4-bm117-tests1-linux64-build2.txt:4
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-1-bm51-tests1-linux64-build9.txt:1

This occurs in 445 tests, tests w/ a high number of warnings:

> 5158 - dom/imptests/editing/conformancetest/test_runtest.html
> 2016 - editor/libeditor/tests/test_bug640321.html
>  336 - editor/libeditor/tests/browserscope/test_richtext2.html
>  336 - editor/libeditor/tests/browserscope/test_richtext2.html (e10s)

[1] https://hg.mozilla.org/mozilla-central/annotate/56e207dbb3bd/layout/base/nsCSSFrameConstructor.cpp#l6558
So this is a known bug in editor, unfortunately.  It's doing something screwed up that happens to work sometimes by accident but is not guaranteed to work in general.  :(

We could try to suppress the warning for the particular case of the editor consumer, I guess, or even better turn it into an assert for all cases but the editor consumer.
(In reply to Boris Zbarsky [:bz] from comment #1)
> So this is a known bug in editor, unfortunately.  It's doing something
> screwed up that happens to work sometimes by accident but is not guaranteed
> to work in general.  :(
> 
> We could try to suppress the warning for the particular case of the editor
> consumer, I guess, or even better turn it into an assert for all cases but
> the editor consumer.

Any thoughts on how to identify the consumer as editor? AFAICT there are no direct calls to |nsCSSFrameConstructor::GetInsertionPrevSibling| (nor the things that call it) outside of nsCSSFrameConstructor.
> Any thoughts on how to identify the consumer as editor?

What do the stack traces for these warnings look like?
Attached file Example stacks
A few stacks found by sampling the callstack in GDB during testing. None seem to be direct.

It looks like the most common (indirect) entry point is:
  |nsHTMLEditor::CreateAnonymousElement|

There was an event based one:
  |mozilla::EventTargetChainItem::HandleEventTargetChain|

And another editor one:
  |nsHTMLEditor::GetPositionAndDimensions|
Thanks.  So for the CreateAnonymousElement case, which is the case I was thinking of, what we could do is have it set some boolean on the nsCSSFrameConstructor around the RecreateFramesFor call.

But the other cases are a problem; they show us restyling anonymous content in a way that involves reconstructing its frames, which is pretty broken in general: the frames for it will get inserted in a bogus place in the frame tree.  I wonder what the anonymous content actually is in that case and whether it originally came from nsHTMLEditor::CreateAnonymousElement.  Maybe what we could do is have CreateAnonymousElement set a property on the elements it creates and only assert (warn for now?) in CSSFC if that property is not set on the element it's looking at.  Let me try doing some try runs along those lines.
OK, so try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7b0c1925c42 shows some failures from at least content added via document.insertAnonymousContent().  I guess I should whitelist that too.  :(
Blocks: logspam
Splitting out by test here are the worst offenders:

> 5158 - dom/imptests/editing/conformancetest/test_runtest.html
> 2016 - editor/libeditor/tests/test_bug640321.html
> 336 - editor/libeditor/tests/browserscope/test_richtext2.html
> 336 - editor/libeditor/tests/browserscope/test_richtext2.html
> 64 - editor/libeditor/tests/test_htmleditor_keyevent_handling.html
> 28 - browser/devtools/fontinspector/test/browser_fontinspector_theme-change.js
> 26 - browser/devtools/framework/test/browser_toolbox_window_reload_target.js
> 18 - dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html
> 16 - editor/libeditor/tests/test_bug857487.html
> 16 - editor/libeditor/tests/test_bug857487.html
> 16 - editor/libeditor/tests/browserscope/test_richtext.html
> 16 - editor/libeditor/tests/browserscope/test_richtext.html
> 12 - layout/base/tests/test_bug558663.html
> 12 - layout/base/tests/test_bug558663.html
> 10 - layout/generic/test/test_image_selection.html
> 10 - layout/generic/test/test_image_selection.html
> 10 - file:///builds/slave/test/build/tests/reftest/tests/editor/libeditor/crashtests/420439.html
> 10 - editor/libeditor/tests/test_bug417418.html
> 10 - editor/libeditor/tests/test_bug417418.html

(double entries are due to e10s vs not e10s)

This is now the #1 most verbose warning during testing.
Yeah, so I tried replacing this with an assert and whitelisting the places that hit it for good reasons.  You can see the results at https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f92cdb1740b

The good news is that if I just replace that assert back with a warning we'll probably kill off most instances of this warning.  The bad news is that the assert is still being hit on platforms I can't easily debug on at the moment....

If you have a Windows build environment and can reproduce the crashtest crash using the patch at https://hg.mozilla.org/try/rev/3f92cdb1740b and give me some idea of what the node involved is (its tag name and namespace, at least, and maybe some info about its parent) that would be awesome.  If not, I totally understand and can either do more try pushes that log that info or just make the assert back into a warning.
(In reply to Boris Zbarsky [:bz] from comment #8)
> Yeah, so I tried replacing this with an assert and whitelisting the places
> that hit it for good reasons.  You can see the results at
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f92cdb1740b
> 
> The good news is that if I just replace that assert back with a warning
> we'll probably kill off most instances of this warning.  The bad news is
> that the assert is still being hit on platforms I can't easily debug on at
> the moment....
> 
> If you have a Windows build environment and can reproduce the crashtest
> crash using the patch at https://hg.mozilla.org/try/rev/3f92cdb1740b and
> give me some idea of what the node involved is (its tag name and namespace,
> at least, and maybe some info about its parent) that would be awesome.  If
> not, I totally understand and can either do more try pushes that log that
> info or just make the assert back into a warning.

Excellent news! I can setup a windows environment next week, although I imagine it will take some time. In the short term, it would be easier to just log the stack rather than assert and grep through the logs.

If you feel like it's not worth the effort, I'd be happy w/ whitelisting + warning for now and we can file a follow up for the long tail.
Logging the stack is not terribly useful: it will probably just say we're restyling.  The question is what added the element in the past that is now getting restyled.
I've repro'd this on windows (it took quite a few retries, so not 100%). 

Not sure where I can get the tag name and namespace, but here's what I got from inspecting |aChild|:
  - type: mozilla::dom::SVGPolylineElement
  - mNodeName: "polyline"
  - parent-type: mozilla::dom::SVGUseElement
  - parent-mNodeName: "use"
> Not sure where I can get the tag name and namespace

From element->NodeInfo().  But the concrete type and nodename is good enough for my purposes.

So this is presumably the <use xlink:href="#polylineID" transform=""> in dom/smil/crashtests/483584-2.svg.  Daniel, do you understand this test?  It's running animations, but doesn't seem to be waiting for them before completing, so that might be the race that's causing it to not always show the problem..
Flags: needinfo?(dholbert)
(In reply to Boris Zbarsky [:bz] from comment #12)
> So this is presumably the <use xlink:href="#polylineID" transform=""> in
> dom/smil/crashtests/483584-2.svg.  Daniel, do you understand this test?
> It's running animations, but doesn't seem to be waiting for them before
> completing,

I believe it's just serving as a regression test for a hang which used to occur when that testcase was loaded (regardless of whether the animations completed) -- looks like that, from bug 547333 comment 18, at least. (That's where we checked in this test; bug 547333 is a followup to bug 483584.)

> so that might be the race that's causing it to not always show
> the problem..

We can probably adjust this testcase to seek to the end of its animations synchronously, if that would help things here.
I spun off bug 1183359 to make 483584-2.svg drive its animations more directly (and stop them before it completes), to provide better encapsulation there.

(IIUC, that should make it easier to figure out what's going on in this bug here.)
Flags: needinfo?(dholbert)
This is now the #1 most verbose warning, combined with bug 1180036 (#3) we're looking at about 16,500 warnings.

bz, do you need any help working with this further?
I don't know yet.  I haven't had a chance to try to reproduce with the updated test.
Flags: needinfo?(bzbarsky)
OK, with the updated test on Mac I still don't get the test to assert on Mac when run inside the test harness.  :(

But running it just in a browser does trigger the assert sometimes, not always.

So as expected, we're reframing an anonymous <polyline> that's a child of <use>.  We're reframing due to a style flush caused by nsSMILAnimationController::DoSample doing a StyleAnimationValue::ComputeValue (indirectly), which leads to restyle processing.  I'm not sure what exactly caused the restyle to be posted, of course....

Looks like we explicitly reframe the parent of stuff flagged with NS_FRAME_ANONYMOUSCONTENTCREATOR_CONTENT when RecreateFramesForContent happens _except_ if the parent is an nsSVGUseFrame.  I guess this is OK only because nsSVGUseFrame can only have a single child.  Alright, let me try whitelisting that one too.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8637326 - Flags: review?(dholbert) → review+
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/e32f059e60c9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.