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)
Core
CSS Parsing and Computation
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.
Review commit: https://reviewboard.mozilla.org/r/46771/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46771/
Comment 2•8 years ago
|
||
(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
Comment 3•8 years ago
|
||
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)
Comment 4•8 years ago
|
||
(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 hidden (spam) |
Comment hidden (spam) |
Comment 8•8 years ago
|
||
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 9•8 years ago
|
||
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) |
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae1879ede4f0 https://hg.mozilla.org/integration/mozilla-inbound/rev/aba943f47963
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ae1879ede4f0 https://hg.mozilla.org/mozilla-central/rev/aba943f47963
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 18•8 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.
Description
•