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

RESOLVED FIXED in Firefox 58

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: tsmith, Assigned: emilio)

Tracking

(Blocks 3 bugs, {assertion, testcase})

58 Branch
mozilla59
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57 unaffected, firefox58 fixed, firefox59 fixed)

Details

Attachments

(6 attachments)

(Reporter)

Description

2 years ago
Posted file 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)
Flags: in-testsuite?
(Reporter)

Comment 1

2 years ago
Posted file prefs.js
(Assignee)

Updated

2 years ago
Flags: needinfo?(emilio)
(Assignee)

Comment 2

2 years ago
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...
(Assignee)

Updated

2 years ago
Flags: needinfo?(emilio)
(Assignee)

Updated

2 years ago
Assignee: nobody → emilio
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 years ago
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...
Attachment #8925876 - Flags: review?(bzbarsky)
Blocks: 1409079
Has Regression Range: --- → yes

Comment 7

2 years ago
mozreview-review
Comment on attachment 8925875 [details]
Bug 1415013: Devirtualize PresShell::IsSafeToFlush.

https://reviewboard.mozilla.org/r/197094/#review202710
Attachment #8925875 - Flags: review?(bzbarsky) → review+

Comment 8

2 years ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/adcb5a4c7591
Devirtualize PresShell::IsSafeToFlush. r=bz
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Updated

2 years ago
Blocks: 1415237
(Assignee)

Comment 9

2 years ago
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.
Attachment #8926875 - Flags: review?(bzbarsky)
(Assignee)

Comment 11

2 years ago
This is a slightly easier patch than what I was trying to do... Though I still think the other refactor is worth it too.
Attachment #8926955 - Flags: review?(bzbarsky)
(Assignee)

Updated

2 years ago
Blocks: 1414941
Priority: -- → P3
Attachment #8926875 - Flags: review?(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.
Attachment #8926955 - Flags: review?(bzbarsky) → review+

Comment 13

2 years ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8067f9bf9a7
Clear servo data on flattened tree changes. r=bz
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Comment 15

2 years ago
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.
Please request Beta approval on this when you get a chance.
Flags: needinfo?(emilio)
Keywords: leave-open
Target Milestone: --- → mozilla59
(Assignee)

Comment 17

2 years ago
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
Flags: needinfo?(emilio)
Attachment #8925876 - Flags: approval-mozilla-beta?
Depends on: 1418059
No longer depends on: 1418059
Comment on attachment 8925876 [details]
Bug 1415013: Clear servo data from the flattened tree in DestroyFramesForAndRestyle.

Fix a stylo issue. Beta58+.
Attachment #8925876 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1417561
(Reporter)

Updated

2 years ago
Group: layout-core-security
See Also: → 1417561
Group: layout-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.