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

RESOLVED FIXED in Firefox 57

Status

()

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: truber, Assigned: emilio)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Trunk
mozilla58
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 fixed, firefox58 fixed)

Details

Attachments

(2 attachments)

Posted 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?
(Assignee)

Comment 1

2 years ago
(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)
(Reporter)

Comment 2

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

Correct.
Flags: needinfo?(jschwartzentruber)
(Assignee)

Comment 3

2 years ago
Got this.
Assignee: nobody → emilio
(Assignee)

Comment 4

2 years ago
Actually, I know why the problem happens, but I'm not 100% sure it _should_ happen....
(Assignee)

Updated

2 years ago
Summary: stylo: Assertion failure: !parent->HasAnyOfFlags(Element::kAllServoDescendantBits) → stylo: Assertion failure: !element->HasAnyOfFlags(Element::kAllServoDescendantBits)
Priority: -- → P2
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-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

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 hidden (mozreview-request)
(Assignee)

Comment 8

2 years ago
mozreview-review-reply
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

2 years ago
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

2 years ago
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
(Assignee)

Comment 12

2 years ago
Gah, of course I just verified it crashed without my patch, but not that it actually passed.
Flags: needinfo?(emilio)

Comment 13

2 years ago
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

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8d4f4b54141a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
I guess we want to uplift that to 57? :)
Flags: needinfo?(emilio)
(Assignee)

Comment 16

2 years ago
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?
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
status-firefox-esr52: --- → unaffected
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+

Comment 18

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/dade9951a461
status-firefox57: affected → fixed
(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.