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)

defect

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)

Attached file testcase.html
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?
Priority: -- → P2
Flags: needinfo?(bzbarsky)
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.
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: nobody → bzbarsky
Status: NEW → ASSIGNED
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+
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!
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
https://hg.mozilla.org/mozilla-central/rev/e5dd43e9452d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.