1177819 - ~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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Eric Rahm [:erahm]]

> 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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Eric Rahm [:erahm]]

(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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Any thoughts on how to identify the consumer as editor?

What do the stack traces for these warnings look like?

Comment 4

[Eric Rahm [:erahm]]

Attachment: 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|

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 7

[Eric Rahm [:erahm]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Eric Rahm [:erahm]]

(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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Eric Rahm [:erahm]]

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"

Comment 12

[Boris Zbarsky [:bzbarsky]]

> 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..

Comment 13

[Daniel Holbert [: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.

Comment 14

[Daniel Holbert [:dholbert]]

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.)

Comment 15

[Eric Rahm [:erahm]]

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?

Comment 16

[Boris Zbarsky [:bzbarsky]]

I don't know yet.  I haven't had a chance to try to reproduce with the updated test.

Comment 17

[Boris Zbarsky [: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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d44c25d4674a

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Replace the warning about anon content being passed into frame construction with an assert plus whitelisting of the known-OK cases

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e32f059e60c9

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e32f059e60c9