Closed
Bug 1397398
Opened 7 years ago
Closed 7 years ago
stylo: Assertion failure: !mAssertWrapperRestyleLength || mPendingWrapperRestyles.Length() == mPendingWrapperRestyleOffset (Someone forgot to call ProcessWrapperRestyles!), at ServoRestyleManager.h:84
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | --- | fixed |
People
(Reporter: truber, Assigned: bzbarsky)
References
(Blocks 2 open bugs)
Details
(Keywords: assertion, testcase)
Attachments
(3 files, 1 obsolete file)
389 bytes,
text/html
|
Details | |
389 bytes,
text/html
|
Details | |
4.32 KB,
patch
|
Details | Diff | Splinter Review |
The attached testcase causes an assertion in m-c rev 20170906-c959327c6b75 with stylo enabled. Assertion failure: !mAssertWrapperRestyleLength || mPendingWrapperRestyles.Length() == mPendingWrapperRestyleOffset (Someone forgot to call ProcessWrapperRestyles!), at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ServoRestyleManager.h:84 #0: mozilla::ServoRestyleState::~ServoRestyleState, at layout/base/ServoRestyleManager.h:82 #1: mozilla::ServoRestyleManager::DoProcessPendingRestyles, at layout/base/ServoRestyleManager.cpp:1117 #2: mozilla::PresShell::DoFlushPendingNotifications, at layout/base/PresShell.cpp:4191 #3: mozilla::PresShell::DoFlushPendingNotifications, at layout/base/PresShell.cpp:4064 #4: mozilla::EventStateManager::PreHandleEvent, at dom/events/EventStateManager.cpp:737 #5: mozilla::PresShell::HandleEventInternal, at layout/base/PresShell.cpp:8154 #6: mozilla::PresShell::HandlePositionedEvent, at layout/base/PresShell.cpp:7951 #7: mozilla::PresShell::HandleEvent, at layout/base/PresShell.cpp:7749 #8: nsViewManager::DispatchEvent, at view/nsViewManager.cpp:803 #9: nsView::HandleEvent, at view/nsView.cpp:1137 #10: mozilla::widget::PuppetWidget::DispatchEvent, at widget/PuppetWidget.cpp:395 #11: mozilla::layers::APZCCallbackHelper::DispatchWidgetEvent, at gfx/layers/apz/util/APZCCallbackHelper.cpp:498 #12: mozilla::dom::TabChild::RecvRealMouseButtonEvent, at dom/ipc/TabChild.cpp:1701 #13: mozilla::dom::TabChild::RecvSynthMouseMoveEvent, at dom/ipc/TabChild.cpp:1652 #14: mozilla::dom::TabChild::RecvNormalPrioritySynthMouseMoveEvent, at dom/ipc/TabChild.cpp:1663 #15: mozilla::dom::PBrowserChild::OnMessageReceived, at ed13da5492fa1612139759a7da5d71861d287c5f7879313536c4c209b4ae2aa436c1de4e982d8900a9e1ff213e4950db7242ce838935efe17693805d87ce26e4/ipc/ipdl/PBrowserChild.cpp:3522
Flags: in-testsuite?
Updated•7 years ago
|
Priority: -- → P2
Updated•7 years ago
|
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 1•7 years ago
|
||
This doesn't seem to be reproducible on tip with the attached testcase. Bisect says: Due to skipped revisions, the first good revision could be any of: changeset: 379191:c4a95bccc94f parent: 379189:7b74ef52385d user: Emilio Cobos Álvarez <emilio@crisal.io> date: Wed Sep 06 07:55:08 2017 -0500 summary: servo: Merge #18391 - style: Stop the cascade when only reset structs change (from emilio:cascade-stoph); r=heycam changeset: 379192:1e12a866239c user: Henrik Skupin <mail@hskupin.info> date: Mon Aug 28 17:46:06 2017 +0200 summary: Bug 1394381 - Add logging output for Marionette connection attempts. r=maja_zf changeset: 379193:adba302ab807 user: Emilio Cobos Álvarez <emilio@crisal.io> date: Mon Sep 04 14:50:13 2017 +0200 summary: Bug 1395227: Stop the cascade when only reset structs change. r=heycam I guess we might not be reaching the relevant code now... I'll try to debug on an ancestor of those changesets.
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
So this is moderately exciting. I thought I understood the invariants here, wrote the code with them in mind, and then they changed out from under me. ;) The main change that happened is that we no longer start at the document root when restyling. We start at doc->GetServoRestyleRoot(). In this case we basically have markup like this: <span id="x" style="display: contents"> <span style="display: table-caption"> </span> <script> document.body.offsetWidth; x.style.color = "green"; </script> (and in fact that's a minimal testcase for this bug). The restyle starts at "x", with the "root" ServoRestyleState that ServoRestyleManager::DoProcessPendingRestyles creates and then goes down the DOM tree. Since "x" has no frame, we use the provided ServoRestyleState for handling the kids instead of creating our own. When we consider x's kid, we put the table wrapper frame into the list on the restyle state. Then we don't call ProcessWrapperRestyles, because we don't own the restyle state, so we keep unwinding, and then the restyle state goes off the stack and asserts that it has unprocessed restyles. Now the thing is... the reason we process restyles for wrapper anon boxes when the ServoRestyleState for their ancestor _frame_ comes off the stack is that this is where they inherit styles from. In this case, the frame for the table inherits its style from the parent of "x", not from "x". So a restyle rooted at "x" can't change its style. In practice, what this means is that we don't even need to worry about adding pending wrapper restyles to an owner-less ServoRestyleState. But mOwner is debug-only, so it's simpler to just loosen the assert here to not assert if !mOwner, because in that case we can have pending restyles hanging around.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8906160 -
Flags: review?(emilio)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment 5•7 years ago
|
||
Comment on attachment 8906160 [details] [diff] [review] Make sure to process wrapper restyles even if our restyle root has no frame of its own Review of attachment 8906160 [details] [diff] [review]: ----------------------------------------------------------------- The commit message could be clearer. This doesn't make sure we process the restyles, right? Just that we don't assert when not processing them, because it's ok. Maybe it could be reworded? ::: layout/base/crashtests/1397398-3.html @@ +3,5 @@ > + <span style="display: table-caption"> > +</span> > +<script> > + document.body.offsetWidth; > + x.style.color = "green"; This is a much more obvious test-case :)
Attachment #8906160 -
Flags: review?(emilio) → review+
Assignee | ||
Comment 6•7 years ago
|
||
Er, yeah, I wrote the commit message before the patch, and actually before I was done debugging the testcase. I'll rewrite it to reflect reality. Good catch!
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8906160 -
Attachment is obsolete: true
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e5dd43e9452d Don't assert about unprocessed wrapper anonymous box restyles in situations in which the styles of those wrapper anonymous boxes can't have changed anyway. r=emilio
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e5dd43e9452d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•