<br> trips assert about suppressing line breaks in ruby




4 years ago
4 years ago


(Reporter: jruderman, Assigned: xidorn)


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

Mac OS X
assertion, testcase
Dependency tree / graph

Firefox Tracking Flags

(firefox40 affected)



(3 attachments)



4 years ago
Created attachment 8594689 [details]

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')

Comment 1

4 years ago
Created attachment 8594690 [details]

Comment 2

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

Comment 3

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

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. :)

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

Comment 8

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