1567036 - nsBidiPresUtils line iterator can get off due to mPrevFrame being out of sync due to ResolveParagraphWithinBlock

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I need nsBidiPresUtils's line iterator to be accurate to fix bug 1300293, and an assertion I wrote caught that it was not accurate while loading https://html.spec.whatwg.org/ . (No need to view source, like in that bug!)

The patch will explain what the problem was.

(It's... not the only such problem and I haven't gotten the patch working yet, but it does fix the assertion included in this patch.)

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: debugging session #1

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: debugging session #2

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: debugging session #3

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: Bug 1567036 - Clear BidiParagraphData::mPrevFrame when ResolveParagraphWithinBlock creates a new continuation, which invalidates its relationship to the line iterator.

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

That said, it's possible I should instead change this in BidiParagraphData::ResetData, whose only caller is nsBidiPresUtils::ResolveParagraphWithinBlock. But then it would happen even if resolving the inner paragraph doesn't create new continuations. I'm not sure what you think about that tradeoff.

Also, here's a try run.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Interestingly, there was a bug I missed in the first version (the addition of the second null assignment that I've now added related to SplitInlineAncestors) that showed up by my assertion firing while doing view-source of the HTML spec... but didn't show up anywhere in our testsuite. So it seems like I should probably figure out how to add a test for that...

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Here's a new try run.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c1c5d8615e5
Clear BidiParagraphData::mPrevFrame when ResolveParagraphWithinBlock creates a new continuation, which invalidates its relationship to the line iterator. r=jfkthame

Comment 9

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/0c1c5d8615e5