Closed
Bug 1401739
Opened 7 years ago
Closed 7 years ago
stylo: Assertion failure: nsLayoutUtils::FirstContinuationOrIBSplitSibling(parent) == this (This should only be used for children!)
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | + | fixed |
firefox58 | --- | fixed |
People
(Reporter: truber, Assigned: bzbarsky)
References
(Blocks 2 open bugs)
Details
(Keywords: assertion, testcase, Whiteboard: [fuzzblocker])
Attachments
(2 files, 1 obsolete file)
348 bytes,
text/html
|
Details | |
2.94 KB,
patch
|
emilio
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
The attached testcase causes an assertion in m-c rev 20170920-469eb992a9d1
Assertion failure: nsLayoutUtils::FirstContinuationOrIBSplitSibling(parent) == this (This should only be used for children!), at /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:10234
#0: nsIFrame::UpdateStyleOfChildAnonBox, at layout/generic/nsFrame.cpp:10236
#1: mozilla::ServoRestyleState::ProcessMaybeNestedWrapperRestyle, at layout/base/ServoRestyleManager.cpp:227
#2: mozilla::ServoRestyleState::ProcessWrapperRestyles, at layout/base/ServoRestyleManager.cpp:169
#3: mozilla::ServoRestyleManager::ProcessPostTraversal, at layout/base/ServoRestyleManager.cpp:938
#4: mozilla::ServoRestyleManager::DoProcessPendingRestyles, at layout/base/ServoRestyleManager.cpp:1121
#5: mozilla::PresShell::DoFlushPendingNotifications, at layout/base/PresShell.cpp:4170
#6: mozilla::PresShell::DoFlushPendingNotifications, at layout/base/PresShell.cpp:4043
#7: nsDocument::FlushPendingNotifications, at dom/base/nsDocument.cpp:8345
#8: nsDocLoader::DocLoaderIsEmpty, at uriloader/base/nsDocLoader.cpp:703
#9: nsDocLoader::OnStopRequest, at uriloader/base/nsDocLoader.cpp:632
#10: mozilla::net::nsLoadGroup::RemoveRequest, at netwerk/base/nsLoadGroup.cpp:629
#11: nsDocument::DoUnblockOnload, at dom/base/nsDocument.cpp:9173
#12: nsDocument::UnblockOnload, at dom/base/nsDocument.cpp:9095
#13: nsDocument::DispatchContentLoadedEvents, at dom/base/nsDocument.cpp:5599
#14: mozilla::detail::RunnableMethodImpl<nsDocument*, void (nsDocument::*)(), true, (mozilla::RunnableKind)0u>::Run, at xpcom/threads/nsThreadUtils.h:1142
#15: mozilla::SchedulerGroup::Runnable::Run, at xpcom/threads/SchedulerGroup.cpp:396
#16: nsThread::ProcessNextEvent, at xpcom/threads/nsThread.cpp:1039
#17: NS_ProcessNextEvent, at xpcom/threads/nsThreadUtils.cpp:521
#18: mozilla::ipc::MessagePump::Run, at ipc/glue/MessagePump.cpp:97
#19: MessageLoop::RunInternal, at ipc/chromium/src/base/message_loop.cc:326
#20: MessageLoop::Run, at ipc/chromium/src/base/message_loop.cc:319
#21: nsBaseAppShell::Run, at widget/nsBaseAppShell.cpp:158
#22: XRE_RunAppShell, at toolkit/xre/nsEmbedFunctions.cpp:880
#23: mozilla::ipc::MessagePumpForChildProcess::Run, at ipc/glue/MessagePump.cpp:269
#24: MessageLoop::RunInternal, at ipc/chromium/src/base/message_loop.cc:326
#25: MessageLoop::Run, at ipc/chromium/src/base/message_loop.cc:319
#26: XRE_InitChildProcess, at toolkit/xre/nsEmbedFunctions.cpp:705
#27: content_process_main, at ipc/contentproc/plugin-container.cpp:63
#28: main, at browser/app/nsBrowserApp.cpp:285
#29: libc-2.26.so+0x20f6a
#30: MOZ_ReportAssertionFailure, at mfbt/Assertions.h:165
Flags: in-testsuite?
Comment 1•7 years ago
|
||
aChildFrame in this function is a table frame, and thus the parent here ends up being a table wrapper frame. "this" points to the parent of the table wrapper frame (which is a block frame).
Updated•7 years ago
|
Priority: -- → P2
Comment 2•7 years ago
|
||
Requesting tracking on all outstanding p2 stylo bugs.
tracking-firefox57:
--- → ?
Comment 3•7 years ago
|
||
Oh, hmm, I was wrong. parent at that moment does point to the block frame which is the parent of the table wrapper frame.
The problem here is that, the block frame "parent" points to is not the first continuation.
I'm not sure what is the correct fix here. I guess either the assertion is invalid, or "parentForRestyle" in ServoRestyleState::ProcessMaybeNestedWrapperRestyle should be assigned to nsLayoutUtils::FirstContinuationOrIBSplitSibling(parent) rather than parent at ServoRestyleManager.cpp:219.
Assignee | ||
Comment 4•7 years ago
|
||
I think we should loosen the assert at the beginning of nsIFrame::UpdateStyleOfChildAnonBox. Using the first continuation for parentForRestyle isn't right, because of first-line bits.
I'll post a patch with some comments.
Assignee | ||
Comment 5•7 years ago
|
||
> Using the first continuation for parentForRestyle isn't right, because of first-line bits.
No, that's not true. Let me poke at this some more.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Assignee | ||
Comment 6•7 years ago
|
||
MozReview-Commit-ID: KhfvBuCeoex
Attachment #8911323 -
Flags: review?(xidorn+moz)
Updated•7 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•7 years ago
|
||
MozReview-Commit-ID: KhfvBuCeoex
Attachment #8911337 -
Flags: review?(xidorn+moz)
Assignee | ||
Updated•7 years ago
|
Attachment #8911323 -
Attachment is obsolete: true
Attachment #8911323 -
Flags: review?(xidorn+moz)
Comment 8•7 years ago
|
||
Comment on attachment 8911337 [details] [diff] [review]
Make sure to always call UpdateStyleOfChildAnonBox with the first continuation as "this"
Review of attachment 8911337 [details] [diff] [review]:
-----------------------------------------------------------------
This looks like a reasonable fix, but emilio should be a better reviewer here given he reviewed bug 1390389 (where you incorrectly marked r=me).
I'm confused that, wouldn't this make a unrelated frame inherit something which has taken :first-line into account? Or :first-line fixup always happens after this?
Attachment #8911337 -
Flags: review?(xidorn+moz) → review?(emilio)
Comment 9•7 years ago
|
||
Comment on attachment 8911337 [details] [diff] [review]
Make sure to always call UpdateStyleOfChildAnonBox with the first continuation as "this"
Review of attachment 8911337 [details] [diff] [review]:
-----------------------------------------------------------------
This looks ok, given it happens before the first line reparenting stuff. r=me
Attachment #8911337 -
Flags: review?(emilio) → review+
Assignee | ||
Comment 10•7 years ago
|
||
> Or :first-line fixup always happens after this?
Exactly. At this point, the ::first-line styles are not in play yet.
Updated•7 years ago
|
status-firefox58:
--- → affected
Comment 11•7 years ago
|
||
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b97d4e53e02
Make sure to always call UpdateStyleOfChildAnonBox with the first continuation as "this". r=emilio
Assignee | ||
Comment 12•7 years ago
|
||
Comment on attachment 8911337 [details] [diff] [review]
Make sure to always call UpdateStyleOfChildAnonBox with the first continuation as "this"
Should probably backport this to 57.
Approval Request Comment
[Feature/Bug causing the regression]: Stylo.
[User impact if declined]: No immediate impact, but interferes with fuzzing.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]:
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: In my opinion, no.
[Why is the change risky/not risky?]: It just uses a different frame with the
same style, to satisfy an assertion.
[String changes made/needed]: None.
Attachment #8911337 -
Flags: approval-mozilla-beta?
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Comment 13•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 14•7 years ago
|
||
Comment on attachment 8911337 [details] [diff] [review]
Make sure to always call UpdateStyleOfChildAnonBox with the first continuation as "this"
Fix some assert issue, taking it.
Should be in 57b4
Attachment #8911337 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•7 years ago
|
Comment 15•7 years ago
|
||
bugherder uplift |
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•