Closed Bug 1412110 Opened 5 years ago Closed 5 years ago

Assertion failure: !item->GetAbove(), at /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:3151


(Core :: Web Painting, defect)

58 Branch
Not set



Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- fixed


(Reporter: jkratzer, Assigned: mattwoodrow)


(Blocks 1 open bug)


(Keywords: assertion, testcase)


(5 files)

Attached file trigger.html
Testcase found while fuzzing mozilla-central rev d734e6acf777.
Flags: in-testsuite?
Attached file log_minidump.txt
The first testcase reloads repeatedly, and I think it crashes when I click it a certain way (without needing the reloads).

I reduced it quite a bit to this testcase, removing a lot of the complexity -- this just combines position:sticky, a list bullet, and contenteditable.
The failing assertion is brand-new, as of bug 1405146.  mattwoodrow, can you take a look?
Blocks: 1405146
If it helps, here's a log from a brief "rr" debugging session, including a backtrace and a framedump.

As you can see, the "item" in question is a nsDisplayStickyPosition, and its GetAbove() method returns a nsDisplayCaret* (which is non-null, hence the assertion failure).

(I assume the nsDisplayCaret is the blinking insertion-point caret from contenteditable, or something like that.)
Flags: needinfo?(matt.woodrow)
Component: Layout → Layout: Web Painting
(57 is unaffected, per comment 3)
Has Regression Range: --- → yes
We have code to manually invalidate the frame when we become/stop being the caret frame, so we don't need to worry about the transition from no wrapper -> wrapper.
Assignee: nobody → matt.woodrow
Flags: needinfo?(matt.woodrow)
Attachment #8924475 - Flags: review?(mikokm)
Comment on attachment 8924475 [details] [diff] [review]
Make sure we create a wraplist when we build a caret item

Review of attachment 8924475 [details] [diff] [review]:

LGTM. It would be nice to have a comment to explain the logic behind canSkipWrapList and aCreatedContainerItem.

::: layout/generic/nsFrame.cpp
@@ +3505,5 @@
>        // return early.
>        aBuilder->AdjustWindowDraggingRegion(child);
>        child->BuildDisplayList(aBuilder, aLists);
> +      if (aBuilder->DisplayCaret(child, aLists.Content())) {

This change seems unnecessary since we return immediately?

::: layout/painting/nsDisplayList.h
@@ +719,5 @@
>    bool IsCompositingCheap() const { return mIsCompositingCheap; }
>    /**
>     * Display the caret if needed.
>     */
> +  bool DisplayCaret(nsIFrame* aFrame, nsDisplayList* aList) {

Since you touch the line anyway, could you move the brace on a new line?
Attachment #8924475 - Flags: review?(mikokm) → review+
Blocks: 1414816
Pushed by
Make sure we build a wrap list for the caret frame, since it will have multiple display items. r=miko
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.