Closed Bug 1264949 Opened 8 years ago Closed 8 years ago

Debug-only abort (from fatal assertion) when we have background-clip:text and text-overflow: ellipsis

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: u459114, Assigned: u459114)

References

Details

(Keywords: assertion)

Attachments

(2 files)

Patches in bug 759568 did not handle nsDisplayTextOverflowMarker::Paint, so we can't see the shape of overflow marker. It is not a blocker of turning on bg-clip:text, since the behavior of both chrome & safari is the same with us.

Real blocker here is this assertion crash
ClipBackgroundByText(nsIFrame* aFrame, nsRenderingContext* aContext,
                     const gfxRect& aFillRect)
{
//..
#ifdef DEBUG
  for (nsDisplayItem* i = list.GetBottom(); i; i = i->GetAbove()) {
    MOZ_ASSERT(nsDisplayItem::TYPE_TEXT == i->GetType());
  }
#endif
}

nsDisplayTextOverflowMarker, type == TYPE_TEXT_OVERFLOW, is put into list, follow by an assertion crash.
(Clarifying summary, since this is an intentional abort rather than a crash, and it's debug-only)
(Also: do make sure to include an automated testcase before this lands.)
Flags: in-testsuite?
Keywords: assertion
Summary: Assertion crash when we have background-clip:text and text-overflow: ellipsis → Debug-only abort (from fatal assertion) when we have background-clip:text and text-overflow: ellipsis
Two issues that I'm noticing with the patch here, when playing with this feature today:
 (1) Typo in commit message -- "assetion" is missing an "r"

 (2) I don't think this patch is sufficient. I'm hitting this fatal assertion when the page from bug 1248644 *, and the patch here doesn't help. In my case (with this patch applied) I'm still aborting because i->GetType() is nsDisplayItem::TYPE_OPACITY.  (not TEXT or TEXT_OVERFLOW)


* That page is:
http://www.bloomberg.com/news/features/2016-02-15/missing-malaysia-jet-mh370-weeks-away-from-keeping-secrets-forever
Flags: needinfo?(cku)
(Also: while you're modifying this MOZ_ASSERT, please add an explanatory message to it, indicating why we have reason to expect that the assertion should hold.)

(jfkthame should also probably review this, since he knows text and display-list code better than I do. I'm just posting some driveby feedback as I play around with this feature & test this patch.)
I added this assertion to ensure that we don't generate and put useless display items, such as image or background image display items, into nsDisplayList.

To correct the assertion here, I should list all display item types that can be generated by nsTextFrame directly or _indirectly_, which is difficult.

Another thought is I may just ensure that there is no TYPE_BCCKGROUND and TYPE_BCCKGROUND_COLOR display item stored in this display list, since it will cause infinite ClipBackgroundByText recursive call.
Flags: needinfo?(cku)
Comment on attachment 8741813 [details]
MozReview Request: Bug 1264949 - Ensure that the display list does not contain any background-image/background-color display item;

https://reviewboard.mozilla.org/r/46771/#review43697

This seems reasonable; I'd just suggest a slight re-wording for the comment.

::: layout/base/nsDisplayList.cpp:518
(Diff revision 2)
>                                        &list);
>    builder.LeavePresShell(aFrame);
>  
>  #ifdef DEBUG
> +  // ClipBackgroundByText is called by nsDisplayBackgroundImage::Paint or
> +  // nsDisplayBackgroundColor::Paint, have this assertion here to ensure that,

For readability, please split this into two sentences (perhaps even start a new line):

    // ClipBackgroundByText is called by nsDisplayBackgroundImage::Paint or
    // nsDisplayBackgroundColor::Paint.
    // Assert that we do not generate and put nsDisplayBackgroundImage or
    // nsDisplayBackgroundColor into the list again, which would lead to
    // infinite recursion.
Attachment #8741813 - Flags: review?(jfkthame) → review+
Comment on attachment 8742222 [details]
MozReview Request: Bug 1264949 - crash test;

https://reviewboard.mozilla.org/r/46991/#review43699
Attachment #8742222 - Flags: review?(jfkthame) → review+
Blocks: 1265633
https://hg.mozilla.org/mozilla-central/rev/ae1879ede4f0
https://hg.mozilla.org/mozilla-central/rev/aba943f47963
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
WebKit and Blink there are similar problems.
You need to log in before you can comment on or make changes to this bug.