1415013 - Assertion failure: !parent->HasAnyOfFlags(Element::kAllServoDescendantBits), at /src/layout/style/ServoStyleSet.cpp:995

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Assertion failure: !parent->HasAnyOfFlags(Element::kAllServoDescendantBits), at /src/layout/style/ServoStyleSet.cpp:995

==33207==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f421bf15934 bp 0x7ffc6f72ad50 sp 0x7ffc6f72ab80 T0)
==33207==The signal is caused by a WRITE memory access.
==33207==Hint: address points to the zero page.
    #0 0x7f421bf15933 in mozilla::ServoStyleSet::StyleDocument(mozilla::ServoTraversalFlags) /src/layout/style/ServoStyleSet.cpp:1003:7
    #1 0x7f421c1a9915 in mozilla::ServoRestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:1127:20
    #2 0x7f421c1773cb in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /src/layout/base/PresShell.cpp:4186:41
    #3 0x7f421c10b493 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:1882:18
    #4 0x7f421c11491e in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /src/layout/base/nsRefreshDriver.cpp:306:7
    #5 0x7f421c1146f4 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:327:5
    #6 0x7f421c117bd5 in mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:769:5
    #7 0x7f421c116c76 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:682:35
    #8 0x7f421c112df7 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() /src/layout/base/nsRefreshDriver.cpp:528:20
    #9 0x7f4216463a9f in nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1037:14
    #10 0x7f42164846b0 in NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:513:10
    #11 0x7f4217020b55 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21
    #12 0x7f4216f72f57 in MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10
    #13 0x7f4216f72de9 in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3
    #14 0x7f421bbfe50a in nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:158:27
    #15 0x7f421efc1031 in nsAppStartup::Run() /src/toolkit/components/startup/nsAppStartup.cpp:288:30
    #16 0x7f421f135758 in XREMain::XRE_mainRun() /src/toolkit/xre/nsAppRunner.cpp:4675:22
    #17 0x7f421f13737a in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4837:8
    #18 0x7f421f1382a9 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4932:21
    #19 0x4ed558 in do_main(int, char**, char**) /src/browser/app/nsBrowserApp.cpp:231:22
    #20 0x4ece7b in main /src/browser/app/nsBrowserApp.cpp:304:16
    #21 0x7f42354b982f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
    #22 0x41ebe4 in _start (/home/user/workspace/browsers/m-c-1509629439-asan-debug/firefox+0x41ebe4)

Comment 1

[Tyson Smith [:tsmith]]

Attachment: prefs.js

Comment 2

[Emilio Cobos Álvarez (:emilio)]

So I took a look at this. The reason this happens is that we're posting a restyle for something that claims to be part of the composed doc, but it's not part of the flattened tree. 

Since we don't know it, we start propagating flags up the tree, and then arrive to the shadow tree boundary, and GetFlattenedTreeParentNodeForStyle() returns null there, so we assume we're in a different restyle root.

Of course we're not a restyle root, so we mess up and leave that state around... I think the fastest way to make this work is removing style data when an element goes out of the flattened tree, I'm looking into how to do this properly...

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1415013: Devirtualize PresShell::IsSafeToFlush.

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1415013: Clear servo data from the flattened tree in DestroyFramesForAndRestyle.

Otherwise we may end up leaving style Servo data on nodes that no longer are on
the flattened tree, and thus incorrectly processing (or trying to process)
restyles in them.

Also devirtualize the function while at it.

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

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8925876 [details]
Bug 1415013: Clear servo data from the flattened tree in DestroyFramesForAndRestyle.

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

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8925876 [details]
Bug 1415013: Clear servo data from the flattened tree in DestroyFramesForAndRestyle.

Gah, this is not ready for review yet, since the ClearServoDataFromSubtree means also that we won't store the reframe hint...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8925875 [details]
Bug 1415013: Devirtualize PresShell::IsSafeToFlush.

https://reviewboard.mozilla.org/r/197094/#review202710

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/adcb5a4c7591
Devirtualize PresShell::IsSafeToFlush. r=bz

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Some cleanup that allows me to fix this more easily.

I want to do the correct, less-footgunny fix for this, which is making no previous style data imply frame construction. That solves the problem of us not storing change hints for elements without style data, which is what the problem the patch here has.

However that requires some cleanup that can land separately, and this is it.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/adcb5a4c7591

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Clear servo data on flattened tree changes.

This is a slightly easier patch than what I was trying to do... Though I still think the other refactor is worth it too.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8926955 [details] [diff] [review]
Clear servo data on flattened tree changes.

Can you please document on Element::mServoData when it needs to be cleared or not?

>+      // Clear the style data from all the flattened tree descendants.

I think this comment should point out explicitly that it's not clearing servo data from aElement and why it's not doing that.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8067f9bf9a7
Clear servo data on flattened tree changes. r=bz

Comment 14

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/b8067f9bf9a7

Comment 15

[Emilio Cobos Álvarez (:emilio)]

There are existing tests for this provided I manage to land bug 1414999 properly... Otherwise I'll cycle back and land the crashtest or something.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8925876 [details]
Bug 1415013: Clear servo data from the flattened tree in DestroyFramesForAndRestyle.

Approval Request Comment
[Feature/Bug causing the regression]: stylo
[User impact if declined]: inconsistent state, leading to general badness
[Is this code covered by automated tests?]: not yet, need to also fix bug 1414999.
[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 clears some state that we don't really want to leave around.
[String changes made/needed]: none

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8925876 [details]
Bug 1415013: Clear servo data from the flattened tree in DestroyFramesForAndRestyle.

Fix a stylo issue. Beta58+.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fb8050fb78aa