Closed Bug 1401739 Opened 3 years ago Closed 3 years ago

stylo: Assertion failure: nsLayoutUtils::FirstContinuationOrIBSplitSibling(parent) == this (This should only be used for children!)

Categories

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

defect

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)

Attached file testcase.html
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?
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).
Priority: -- → P2
Requesting tracking on all outstanding p2 stylo bugs.
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.
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.
> 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: nobody → bzbarsky
MozReview-Commit-ID: KhfvBuCeoex
Attachment #8911323 - Flags: review?(xidorn+moz)
Status: NEW → ASSIGNED
MozReview-Commit-ID: KhfvBuCeoex
Attachment #8911337 - Flags: review?(xidorn+moz)
Attachment #8911323 - Attachment is obsolete: true
Attachment #8911323 - Flags: review?(xidorn+moz)
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 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+
> Or :first-line fixup always happens after this?

Exactly.  At this point, the ::first-line styles are not in play yet.
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
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?
https://hg.mozilla.org/mozilla-central/rev/4b97d4e53e02
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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+
You need to log in before you can comment on or make changes to this bug.