Closed Bug 1384232 Opened 3 years ago Closed 3 years ago

stylo: Assertion failure: oldStyleContext->ComputedData() != newContext->ComputedData()

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- disabled
firefox57 --- fixed

People

(Reporter: truber, Assigned: emilio)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(6 files)

Attached file testcase.html
The attached testcases causes an assertion in m-c rev 07484bfdb96b with stylo enabled by pref.

Assertion failure: oldStyleContext->ComputedData() != newContext->ComputedData(), at /home/worker/workspace/build/src/layout07484bfdb96b/base/ServoRestyleManager.cpp:608
#01: mozilla::ServoRestyleManager::DoProcessPendingRestyles at layout/base/ServoRestyleManager.cpp:874
#02: mozilla::PresShell::DoFlushPendingNotifications at layout/base/PresShell.cpp:4195
#03: nsRefreshDriver::Tick at mfbt/RefPtr.h:284
#04: mozilla::RefreshDriverTimer::TickRefreshDrivers at layout/base/nsRefreshDriver.cpp:307
#05: mozilla::RefreshDriverTimer::Tick at layout/base/nsRefreshDriver.cpp:329
#06: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver at layout/base/nsRefreshDriver.cpp:684
#07: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync at layout/base/nsRefreshDriver.cpp:586
#08: mozilla::layout::VsyncChild::RecvNotify at layout/ipc/VsyncChild.cpp:70
#09: mozilla::layout::PVsyncChild::OnMessageReceived at obj-firefox/ipc/ipdl/PVsyncChild.cpp:155
#10: mozilla::ipc::MessageChannel::DispatchAsyncMessage at ipc/glue/MessageChannel.h:633
#11: mozilla::ipc::MessageChannel::DispatchMessage at ipc/glue/MessageChannel.cpp:2033
#12: mozilla::ipc::MessageChannel::RunMessage at ipc/glue/MessageChannel.cpp:1889
#13: mozilla::ipc::MessageChannel::MessageTask::Run at ipc/glue/MessageChannel.cpp:1922
#14: nsThread::ProcessNextEvent at mfbt/Maybe.h:445
#15: NS_ProcessNextEvent at xpcom/threads/nsThreadUtils.cpp:530
#16: mozilla::ipc::MessagePump::Run at ipc/glue/MessagePump.cpp:98
#17: MessageLoop::RunInternal at ipc/chromium/src/base/message_loop.cc:322
#18: MessageLoop::Run at ipc/chromium/src/base/message_loop.cc:294
#19: nsBaseAppShell::Run at widget/nsBaseAppShell.cpp:158
#20: XRE_RunAppShell at toolkit/xre/nsEmbedFunctions.cpp:882
#21: mozilla::ipc::MessagePumpForChildProcess::Run at ipc/glue/MessagePump.cpp:270
#22: MessageLoop::RunInternal at ipc/chromium/src/base/message_loop.cc:322
#23: MessageLoop::Run at ipc/chromium/src/base/message_loop.cc:294
#24: XRE_InitChildProcess at toolkit/xre/nsEmbedFunctions.cpp:703
#25: content_process_main at ipc/contentproc/plugin-container.cpp:66
#26: main at browser/app/nsBrowserApp.cpp:288
#27: __libc_start_main[/usr/lib/libc.so.6 +0x204ca]
#28: _start at 0x29
Flags: in-testsuite?
Priority: -- → P1
I think the assertion is not valid, since there are situations where we can get the same "new" style context from ResolveServoStyle() as the one that is currently on the frame, and that is when we reconstruct the frame before we get a chance to call Servo_TakeChangeHint.

So I guess two options:

1. Remove the assertion, and maybe dd an explicit check to see whether we really did get a new style context, to avoid doing some work.

2. Call Servo_TakeChangeHint to drop any restyle data when we reframe an element.

Emilio, which of these sounds best to you?
Flags: needinfo?(emilio+bugs)
(In reply to Cameron McCormack (:heycam) from comment #1)
> I think the assertion is not valid, since there are situations where we can
> get the same "new" style context from ResolveServoStyle() as the one that is
> currently on the frame, and that is when we reconstruct the frame before we
> get a chance to call Servo_TakeChangeHint.

Hm, but isn't that what the ClearRestyleStateFromSubtree bit is supposed to handle?

http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/layout/base/ServoRestyleManager.cpp#564

(Note that I'm hoping to move that handling directly into the traversal at some point, but in the mean time I'd think we shouldn't reach the assertion in reconstruct-self-or-ancestor cases).
Ah, and if we end up reconstructing a parent frame for whatever reason the frame constructor decides, we won't end up clearing that data?
Flags: needinfo?(cam)
Priority: P1 → --
Priority: -- → P2
It seems to me the assertion doesn't happen anymore?
Flags: needinfo?(jschwartzentruber)
Still happens for me on m-c 20170817-a6a1f5c1d971. Attaching the log & prefs in case anything is relevant there.
Flags: needinfo?(jschwartzentruber)
Attached file log.txt
Attached file prefs.js
(In reply to Jesse Schwartzentruber (:truber) from comment #5)
> Still happens for me on m-c 20170817-a6a1f5c1d971. Attaching the log & prefs
> in case anything is relevant there.

Yes. And the call stack is a little bit different each time I tried. All are the same assertion from DoFlushPendingNotifications, but Sometimes it is from RefreshDriverTimer::Tick(), and sometimes it is from nsDocLoader::DocLoaderIsEmpty(bool), or from mozilla::PresShell::HandlePositionedEvent... :(
(In reply to Boris Chiou [:boris] from comment #8)
> (In reply to Jesse Schwartzentruber (:truber) from comment #5)
> > Still happens for me on m-c 20170817-a6a1f5c1d971. Attaching the log & prefs
> > in case anything is relevant there.
> 
> Yes. And the call stack is a little bit different each time I tried. All are
> the same assertion from DoFlushPendingNotifications, but Sometimes it is
> from RefreshDriverTimer::Tick(), and sometimes it is from
> nsDocLoader::DocLoaderIsEmpty(bool), or from
> mozilla::PresShell::HandlePositionedEvent... :(

Yeah, that's just because this failures is happening in a style flush and there are lots of different things that can trigger a style flush. You can force a flush with |window.getComputedStyle(document.body).color|, which might make the testcase more consistent.
Assignee: nobody → bobbyholley
Flags: needinfo?(emilio)
Flags: needinfo?(cam)
So Bobby and I took a look at this, and this is because we load the XBL binding for the text area when processing restyles.

It ends up loading the stylesheets synchronously, and because we don't differentiate between the main ServoStyleSet and one for an XBL binding, we happen to post a restyle to the root element from change hint processing.

I think the fix here is to properly differentiate when a ServoStyleSet is for an XBL binding, and avoid doing any kind of invalidation (passing nullptr as the doc element) for those, since we only compute it once when loading the bindings.

This setup will need to somehow play well with bug 1382078, but I think it does, since that makes us post a restyle to the root anyway. Paging in TYLin just in case.

I can write a patch, or Bobby can if he has the cycles, I guess.
Emilio is going to take this. See also bug 1343362, where we grudgingly added support for these kinds of reentrant restyle hints.
Assignee: bobbyholley → emilio
Blocks: 1390726
Comment on attachment 8903320 [details]
Bug 1384232: Let ServoStyleset know what its purpose is.

https://reviewboard.mozilla.org/r/175116/#review180266

I was thinking about either doing this or making a specialized class inheriting from ServoStyleSet for XBL purpose while working on bug 1382078. Thanks for fixing this. This looks good to me except some C++ nits.

::: layout/style/ServoStyleSet.h:104
(Diff revision 1)
>  
> -  ServoStyleSet();
> +  // The kind of styleset we have.
> +  //
> +  // We use ServoStyleSet also from XBL bindings, and some stuff needs to be
> +  // different between them.
> +  enum class Kind {

Let's make this `enum class Kind : uint8_t` to save some memory.

::: layout/style/ServoStyleSet.h:114
(Diff revision 1)
> +
> +    // A StyleSet for XBL, which is owned by a given XBL binding.
> +    ForXBL,
> +  };
> +
> +  ServoStyleSet(Kind aKind);

Single-argument constructor should annotate as `explicit` per our coding style, so this needs to be `explicit ServoStyleSet(Kind aKind);`

::: layout/style/ServoStyleSet.h:565
(Diff revision 1)
>                           ServoStyleSheet* aBeforeSheet);
>  
>    void RemoveSheetOfType(SheetType aType,
>                           ServoStyleSheet* aSheet);
>  
> +  Kind mKind;

Nit: This is set in the constructor, and shouldn't need to be changed after that, so let's make it `const`
Attachment #8903320 - Flags: review?(tlin) → review+
Comment on attachment 8903321 [details]
Bug 1384232: Don't post restyles when loading XBL stylesheets.

https://reviewboard.mozilla.org/r/175118/#review180272

::: layout/style/ServoStyleSet.cpp:1397
(Diff revision 1)
>  ServoStyleSet::UpdateStylist()
>  {
>    MOZ_ASSERT(StylistNeedsUpdate());
> -  Element* root = mPresContext->Document()->GetDocumentElement();
> +
> +  // There's no need to compute invalidations and such for an XBL styleset,
> +  // since they are loaded and unloaded synchronously, and they don't have to

FWIW, non-chrome XBL stylesheets are loaded asyncronously in http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/dom/xbl/nsXBLResourceLoader.cpp#153-157

It's true that XBL stylesets don't have to deal with dynamic changes, and should be recomputed whenever needed, so I think this is OK.
Attachment #8903321 - Flags: review?(tlin) → review+
Comment on attachment 8903322 [details]
Bug 1384232: Crashtest.

https://reviewboard.mozilla.org/r/175120/#review180274

::: layout/style/crashtests/crashtests.list:211
(Diff revision 1)
>  load 1391577.html
>  load 1393189.html
>  load 1393580.html
>  load 1389645.html
>  load 1393791.html
> +load 1384232.html

I feel that we try very hard to keep file (nearly) sorted, but not a bit deal.
Attachment #8903322 - Flags: review?(tlin) → review+
Comment on attachment 8903321 [details]
Bug 1384232: Don't post restyles when loading XBL stylesheets.

https://reviewboard.mozilla.org/r/175118/#review180272

> FWIW, non-chrome XBL stylesheets are loaded asyncronously in http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/dom/xbl/nsXBLResourceLoader.cpp#153-157
> 
> It's true that XBL stylesets don't have to deal with dynamic changes, and should be recomputed whenever needed, so I think this is OK.

Yeah, I know. The intention of this comment is to say that we load the stylesheets synchronously before the binding is bound to a document, not about how the binding is loaded per se.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ffd346281fb9
Let ServoStyleset know what its purpose is. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/492969ffe3c8
Don't post restyles when loading XBL stylesheets. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/27603653cb27
Crashtest. r=TYLin
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e066d2f8f62d
Reduce expected assertions in test_bug381167.xhtml. r=me
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/eb024541c2cf
Same for test_xultree_animation.xhtml. r=me
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.