Closed Bug 1403615 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: truber, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(2 files)

Attached file 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
Flags: in-testsuite?
(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"?
Flags: needinfo?(jschwartzentruber)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #1) > > I assume "breaks" == "doesn't assert"? Correct.
Flags: needinfo?(jschwartzentruber)
Got this.
Assignee: nobody → emilio
Actually, I know why the problem happens, but I'm not 100% sure it _should_ happen....
Summary: stylo: Assertion failure: !parent->HasAnyOfFlags(Element::kAllServoDescendantBits) → stylo: Assertion failure: !element->HasAnyOfFlags(Element::kAllServoDescendantBits)
Priority: -- → P2
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"?
Attachment #8912814 - Flags: review?(bobbyholley) → review+
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.
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)
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/379e7e7bf80d Also follow the NODE_DESCENDANTS_NEED_FRAMES bit in ClearRestyleStateFromSubtree. r=bholley
Gah, of course I just verified it crashed without my patch, but not that it actually passed.
Flags: needinfo?(emilio)
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/8d4f4b54141a Also follow the NODE_DESCENDANTS_NEED_FRAMES bit in ClearRestyleStateFromSubtree. r=bholley
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
I guess we want to uplift that to 57? :)
Flags: needinfo?(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
Flags: needinfo?(emilio)
Attachment #8912814 - Flags: approval-mozilla-beta?
Flags: in-testsuite? → in-testsuite+
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
Attachment #8912814 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: