<br> trips assert about suppressing line breaks in ruby

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jruderman, Assigned: xidorn)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
x86_64
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 affected)

Details

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
Created attachment 8594689 [details]
testcase

Assertion failure: !(0 != ((reflowStatus) & 0x0100)) && !pushedFrame (Any line break inside ruby box should has been suppressed), at layout/generic/nsRubyBaseContainerFrame.cpp:777

(Btw, there's a typo in the assertion: 'has' should be 'have')
(Reporter)

Comment 1

4 years ago
Created attachment 8594690 [details]
stack
(Assignee)

Comment 2

4 years ago
Created attachment 8595633 [details] [diff] [review]
patch
Assignee: nobody → quanxunzhen
Attachment #8595633 - Flags: review?(dholbert)
(Assignee)

Comment 3

4 years ago
Comment on attachment 8595633 [details] [diff] [review]
patch

I'd like to fix this with bug 1157011. The testcase in that bug should certainly trigger this assertion if it is not crashed first.
Attachment #8595633 - Flags: review?(dholbert)
For future reference, RE this bug & bug 1156222:

It's helpful, for review purposes, if you could briefly explain what's going wrong & causing the assertion-failure (or whatever the bug is) in a bug comment, before (or around when) you request review.

It's not always exactly clear to a reviewer (or to someone looking at this bug in the future) how the patch maps to the actual bug symptoms, and it takes a bit of extra time to infer that when reviewing. In my case, I'm unfamiliar with the assertion in comment 0, and I haven't looked at BRFrame recently, and I wasn't aware that ruby had this line-break-suppressing behavior [though it makes sense]. I'm not suggesting that you cover all of that content preemptively, but at least a quick summary of what's causing the problem would be helpful in getting the reviewer on the right track to understanding the patch quickly.

e.g. something like "Ah, so: We have a check to see if the BR frame is inside of ruby content (via ShouldSuppressLineBreak), and we suppress the line-break if so.  But with this testcase, the BR frame's custom display value is making that check fail, so we break when we shouldn't."
Er, sorry, meant to post that on bug 1157011.  Anyway, just wanted to give you a heads-up, for faster/easier/happier reviews. :)
(Assignee)

Comment 6

4 years ago
Sorry about that. I should have noticed that you are probably not familiar with the code here.
No worries -- though, even when the reviewer's more familiar with the code than I was in this case, I'd still suggest including a brief summary of the issue that you've identified & are fixing, even if it's just a paraphrased English version of the code in the patch.  It helps jog the memory, and it clarifies the point of the patch -- and it's also nice for the benefit of other people looking at the bug to have a chance at figuring out what's going on. :)
Depends on: 1157011
(Assignee)

Comment 8

4 years ago
Should have been fixed by bug 1157011.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.