Closed Bug 1415013 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect, P3)

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: tsmith, Assigned: emilio)

References

(Blocks 3 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(6 files)

Attached file testcase.html (deleted) —
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?
Attached file prefs.js (deleted) —
Flags: needinfo?(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...
Flags: needinfo?(emilio)
Assignee: nobody → 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...
Attachment #8925876 - Flags: review?(bzbarsky)
Blocks: 1409079
Has Regression Range: --- → yes
Attachment #8925875 - Flags: review?(bzbarsky) → review+
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/adcb5a4c7591 Devirtualize PresShell::IsSafeToFlush. r=bz
Keywords: leave-open
Blocks: 1415237
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)
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)
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+
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b8067f9bf9a7 Clear servo data on flattened tree changes. r=bz
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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
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+
Group: layout-core-security
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.

Attachment

General

Created:
Updated:
Size: