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

Status: RESOLVED FIXED

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

Description

[Jesse Schwartzentruber (:truber)]

Attachment: 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

Comment 1

[Cameron McCormack (:heycam)]

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?

Comment 2

[Bobby Holley (:bholley)]

(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).

Comment 3

[Cameron McCormack (:heycam)]

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?

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

It seems to me the assertion doesn't happen anymore?

Comment 5

[Jesse Schwartzentruber (:truber)]

Still happens for me on m-c 20170817-a6a1f5c1d971. Attaching the log & prefs in case anything is relevant there.

Comment 6

[Jesse Schwartzentruber (:truber)]

Attachment: log.txt

Comment 7

[Jesse Schwartzentruber (:truber)]

Attachment: prefs.js

Comment 8

[Boris Chiou [:boris]]

(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... :(

Comment 9

[Bobby Holley (:bholley)]

(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.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 11

[Bobby Holley (:bholley)]

Emilio is going to take this. See also bug 1343362, where we grudgingly added support for these kinds of reentrant restyle hints.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1384232: Let ServoStyleset know what its purpose is.

Review commit: https://reviewboard.mozilla.org/r/175116/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175116/

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1384232: Don't post restyles when loading XBL stylesheets.

Review commit: https://reviewboard.mozilla.org/r/175118/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175118/

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1384232: Crashtest.

Review commit: https://reviewboard.mozilla.org/r/175120/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175120/

Comment 15

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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`

Comment 16

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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.

Comment 17

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8903320 [details]
Bug 1384232: Let ServoStyleset know what its purpose is.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175116/diff/1-2/

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8903321 [details]
Bug 1384232: Don't post restyles when loading XBL stylesheets.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175118/diff/1-2/

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8903322 [details]
Bug 1384232: Crashtest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175120/diff/1-2/

Comment 21

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e066d2f8f62d
Reduce expected assertions in test_bug381167.xhtml. r=me

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb024541c2cf
Same for test_xultree_animation.xhtml. r=me

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ffd346281fb9
https://hg.mozilla.org/mozilla-central/rev/492969ffe3c8
https://hg.mozilla.org/mozilla-central/rev/27603653cb27
https://hg.mozilla.org/mozilla-central/rev/e066d2f8f62d
https://hg.mozilla.org/mozilla-central/rev/eb024541c2cf