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

RESOLVED FIXED in Firefox 58

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jkratzer, Assigned: mattwoodrow)

Tracking

(Blocks 1 bug, {assertion, testcase})

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

Firefox Tracking Flags

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

Details

Attachments

(5 attachments)

Reporter

Description

2 years ago
Posted file trigger.html
Testcase found while fuzzing mozilla-central rev d734e6acf777.
Flags: in-testsuite?
Reporter

Comment 1

2 years ago
Posted 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
Assignee

Comment 6

2 years ago
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+

Updated

2 years ago
Blocks: 1414816

Comment 8

2 years ago
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c87b4385113
Make sure we build a wrap list for the caret frame, since it will have multiple display items. r=miko

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7c87b4385113
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58

Updated

2 years ago
Duplicate of this bug: 1414816
You need to log in before you can comment on or make changes to this bug.