1156227 - <br> trips assert about suppressing line breaks in ruby

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

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

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 4

[Daniel Holbert [: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."

Comment 5

[Daniel Holbert [:dholbert]]

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

[Xidorn Quan [:xidorn] UTC+11]

Sorry about that. I should have noticed that you are probably not familiar with the code here.

Comment 7

[Daniel Holbert [:dholbert]]

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

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Should have been fixed by bug 1157011.