1403615 - stylo: Assertion failure: !element->HasAnyOfFlags(Element::kAllServoDescendantBits)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase causes an assertion in m-c rev 20170926-7d15bc419c6c

It's not 100% reliable. Moving line 16 before line 5 makes it more reliable in m-c, but breaks 57.

Assertion failure: !element->HasAnyOfFlags(Element::kAllServoDescendantBits), at /builds/worker/workspace/build/src/dom/base/Element.cpp:4313
#0: NoteDirtyElement, at dom/base/Element.cpp:4374
#1: style::gecko::wrapper::GeckoElement::maybe_restyle, at servo/components/style/gecko/wrapper.rs:685
#2: core::option::Option<&mut atomic_refcell::AtomicRefMut<style::data::ElementData>>::map_or<&mut atomic_refcell::AtomicRefMut<style::data::ElementData>,bool,closure>, at src/libcore/option.rs:421
#3: style::gecko::wrapper::GeckoElement::note_explicit_hints, at servo/components/style/gecko/wrapper.rs:711
#4: geckoservo::glue::Servo_NoteExplicitHints, at servo/ports/geckolib/glue.rs:3028
#5: mozilla::ServoRestyleManager::AttributeChanged, at layout/base/ServoRestyleManager.cpp:1408
#6: mozilla::PresShell::AttributeChanged, at layout/base/RestyleManagerInlines.h:72
#7: nsNodeUtils::AttributeChanged, at dom/base/nsNodeUtils.cpp:146
#8: mozilla::dom::Element::SetAttrAndNotify, at dom/base/Element.cpp:2678
#9: nsStyledElement::SetInlineStyleDeclaration, at dom/base/nsStyledElement.cpp:114
#10: nsDOMCSSDeclaration::ParsePropertyValue, at layout/style/nsDOMCSSDeclaration.cpp:342
#11: nsDOMCSSDeclaration::SetZIndex, at layout/style/nsCSSPropList.h:4578
#12: mozilla::dom::CSS2PropertiesBinding::set_zIndex, at 8880d5b11e593ea4f726ba5a7288400e1c65bd6a585b5f474989b8be12cbf7c5bd8a001a1cb3cc477a80e492f5b011886114931d5beb7eff4136d7097082b3a6/dom/bindings/CSS2PropertiesBinding.cpp:45335
#13: mozilla::dom::GenericBindingSetter, at dom/bindings/BindingUtils.cpp:3016
#14: js::CallJSNative, at js/src/jscntxtinlines.h:293
#15: js::InternalCallOrConstruct, at js/src/vm/Interpreter.cpp:495
#16: InternalCall, at js/src/vm/Interpreter.cpp:540
#17: js::Call, at js/src/vm/Interpreter.cpp:559
#18: js::CallSetter, at js/src/vm/Interpreter.cpp:688
#19: SetExistingProperty, at js/src/vm/NativeObject.cpp:2757
#20: js::NativeSetProperty<(js::QualifiedBool)1u>, at js/src/vm/NativeObject.cpp:2793
#21: js::SetPropertyIgnoringNamedGetter, at js/src/vm/NativeObject.h:1607
#22: mozilla::dom::DOMProxyHandler::set, at dom/bindings/DOMJSProxyHandler.cpp:221
#23: js::Proxy::setInternal, at js/src/proxy/Proxy.cpp:390
#24: js::Proxy::set, at js/src/proxy/Proxy.cpp:400
#25: JSObject::nonNativeSetProperty, at js/src/jsobj.cpp:1031
#26: js::SetProperty, at js/src/vm/NativeObject.h:1606
#27: Interpret, at js/src/vm/Interpreter.cpp:269
#28: js::RunScript, at js/src/vm/Interpreter.cpp:435
#29: js::InternalCallOrConstruct, at js/src/vm/Interpreter.cpp:513
#30: InternalCall, at js/src/vm/Interpreter.cpp:540
#31: js::Call, at js/src/vm/Interpreter.cpp:559
#32: JS::Call, at js/src/jsapi.cpp:2965
#33: mozilla::dom::IdleRequestCallback::Call, at f00e4473040df69657835efe36d48acafe54b1ae625b67811652df55ab5a5d268e9fbdb8e7dd7b4ba37388412ff610d258cd4027c05ed8b18c2387cb4b34caf1/dom/bindings/WindowBinding.cpp:830
#34: mozilla::dom::IdleRequestCallback::Call, at 7504ad937ab2af8091bf0e0c954ec7adc799cd8602ff39fa3f70cd7ae5993ac494c3a096134bb0774db4421bf8aa4f5e736a1bdbf9f7641b28f0526ab7dfef19/dist/include/mozilla/dom/WindowBinding.h:635
#35: mozilla::dom::IdleRequest::IdleRun, at dom/base/IdleRequest.cpp:66
#36: nsGlobalWindow::RunIdleRequest, at dom/base/nsGlobalWindow.cpp:861
#37: nsGlobalWindow::ExecuteIdleRequest, at dom/base/nsGlobalWindow.cpp:891
#38: nsThread::ProcessNextEvent, at xpcom/threads/nsThread.cpp:1039
#39: NS_ProcessNextEvent, at xpcom/threads/nsThreadUtils.cpp:522
#40: mozilla::ipc::MessagePump::Run, at ipc/glue/MessagePump.cpp:97
#41: MessageLoop::RunInternal, at ipc/chromium/src/base/message_loop.cc:326
#42: MessageLoop::Run, at ipc/chromium/src/base/message_loop.cc:319
#43: nsBaseAppShell::Run, at widget/nsBaseAppShell.cpp:158
#44: XRE_RunAppShell, at toolkit/xre/nsEmbedFunctions.cpp:880
#45: mozilla::ipc::MessagePumpForChildProcess::Run, at ipc/glue/MessagePump.cpp:269
#46: MessageLoop::RunInternal, at ipc/chromium/src/base/message_loop.cc:326
#47: MessageLoop::Run, at ipc/chromium/src/base/message_loop.cc:319
#48: XRE_InitChildProcess, at toolkit/xre/nsEmbedFunctions.cpp:705
#49: content_process_main, at ipc/contentproc/plugin-container.cpp:63
#50: main, at browser/app/nsBrowserApp.cpp:285
#51: libc-2.26.so+0x20f6a
#52: MOZ_ReportAssertionFailure, at mfbt/Assertions.h:165

Comment 1

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jesse Schwartzentruber (:truber) from comment #0)
> Created attachment 8912762 [details]
> testcase.html
> 
> The attached testcase causes an assertion in m-c rev 20170926-7d15bc419c6c
> 
> It's not 100% reliable. Moving line 16 before line 5 makes it more reliable
> in m-c, but breaks 57.

I assume "breaks" == "doesn't assert"?

Comment 2

[Jesse Schwartzentruber (:truber)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> 
> I assume "breaks" == "doesn't assert"?

Correct.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Got this.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Actually, I know why the problem happens, but I'm not 100% sure it _should_ happen....

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1403615: Also follow the NODE_DESCENDANTS_NEED_FRAMES bit in ClearRestyleStateFromSubtree.

We don't follow this bit intentionally because we know that if it's not set,
there are no other restyle / change hints down the tree.

We rely on the frame constructor to clean the mess up, though, and it doesn't
really do a good work about it.

In particular, the case we're hitting on the test-case is:

<body descendant-need-frames change=reconstruct style="display: table-column-group">
  <div descendant-need-frames>
    <div descendant-need-frames>
      <span needs-frame></span>
    </div>
  </div>
</body>

When we see we need to reconstruct the body, we call
ClearRestyleStateFromSubtree, but that doesn't do much now, since we don't
follow the descendant-need-frames bits.

Then, when we reconstruct the content, we arrive at[1] when constructing the
first child <div>. The <div> flags have been cleared, but not the children's!

Then a text-node is inserted in a <div>, breaking all sorts of invariants.

This is the easiest fix. Other fixes include clearing the flags on
SetAsUndisplayedContent. But that implies not clearing them in
ShouldCreateItemsForChild, and doing that somewhere more sensible.

I suspect it's not too hard, but that's a slightly more risky change, will do it
if you prefer it.

[1]: http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/layout/base/nsCSSFrameConstructor.cpp#6092

Review commit: https://reviewboard.mozilla.org/r/184120/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184120/

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8912814 [details]
Bug 1403615: Also follow the NODE_DESCENDANTS_NEED_FRAMES bit in ClearRestyleStateFromSubtree.

https://reviewboard.mozilla.org/r/184120/#review189294

This change seems like the right thing and is nicely low risk. Thanks for the clear explanation!

::: commit-message-d0b2b:3
(Diff revision 1)
> +We don't follow this bit intentionally because we know that if it's not set,
> +there are no other restyle / change hints down the tree.

I think you mean "if it's set"?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8912814 [details]
Bug 1403615: Also follow the NODE_DESCENDANTS_NEED_FRAMES bit in ClearRestyleStateFromSubtree.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184120/diff/1-2/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8912814 [details]
Bug 1403615: Also follow the NODE_DESCENDANTS_NEED_FRAMES bit in ClearRestyleStateFromSubtree.

https://reviewboard.mozilla.org/r/184120/#review189294

> I think you mean "if it's set"?

Yeah, I meant specifically: "even if it's set, but none of the other bits are set". I reworded that bit, and added a crashtest.

Comment 9

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 7d02c9d7614a -d d4ccab1528bf: rebasing 422914:7d02c9d7614a "Bug 1403615: Also follow the NODE_DESCENDANTS_NEED_FRAMES bit in ClearRestyleStateFromSubtree. r=bholley" (tip)
merging layout/base/ServoRestyleManager.cpp
merging layout/style/crashtests/crashtests.list
warning: conflicts while merging layout/style/crashtests/crashtests.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/379e7e7bf80d
Also follow the NODE_DESCENDANTS_NEED_FRAMES bit in ClearRestyleStateFromSubtree. r=bholley

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing own crashtest:

https://hg.mozilla.org/integration/autoland/rev/dbf1351a429ae23910a9001da1ad0fd0969ab67a

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=379e7e7bf80dec2bf8f7df41a7021e4839d9b0ad&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=133634009&repo=autoland
> REFTEST TEST-UNEXPECTED-FAIL | file:///builds/worker/workspace/build/tests/reftest/tests/layout/style/crashtests/1403615.html | load failed: timed out waiting for reftest-wait to be removed

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Gah, of course I just verified it crashed without my patch, but not that it actually passed.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d4f4b54141a
Also follow the NODE_DESCENDANTS_NEED_FRAMES bit in ClearRestyleStateFromSubtree. r=bholley

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8d4f4b54141a

Comment 15

[Sylvestre Ledru [:Sylvestre]]

I guess we want to uplift that to 57? :)

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8912814 [details]
Bug 1403615: Also follow the NODE_DESCENDANTS_NEED_FRAMES bit in ClearRestyleStateFromSubtree.

Yes

Approval Request Comment
[Feature/Bug causing the regression]: stylo
[User impact if declined]: no impact known, but potentially wrong state in the tree.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: Just a one liner that makes us do a bit of extra cleanup after ourselves.
[String changes made/needed]: none

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8912814 [details]
Bug 1403615: Also follow the NODE_DESCENDANTS_NEED_FRAMES bit in ClearRestyleStateFromSubtree.

Fix an assert, taking it.
Should be in 57b5

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dade9951a461

Comment 19

[Andrei Vaida [:avaida]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #16)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: no
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Emilio's assessment on manual testing needs and the fact that this fix has automated coverage.