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

RESOLVED FIXED in Firefox 48

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: u459114, Assigned: u459114)

Tracking

({assertion})

unspecified
mozilla48
assertion
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8741813 [details]
MozReview Request: Bug 1264949 - Ensure that the display list does not contain any background-image/background-color display item;

Review commit: https://reviewboard.mozilla.org/r/46771/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46771/
(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.)
(Assignee)

Comment 5

2 years ago
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 hidden (spam)
Comment hidden (spam)
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+
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Blocks: 1265633
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1265633

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ae1879ede4f0
https://hg.mozilla.org/mozilla-central/rev/aba943f47963
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1270795

Comment 18

2 years ago
WebKit and Blink there are similar problems.
You need to log in before you can comment on or make changes to this bug.