1817235 - Partial Test For `nsBlockFrame::IsLastLine(BlockReflowState& aState, LineIterator aLine)`

Status: RESOLVED FIXED

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

Description

[Carolin Brandt]

Attachment: test.html

---

This report is part of a research collaboration between Mozilla and the TU Delft.
If you want to help us understand whether this report is helpful,
please answer a few questions before you start addressing the report.

If you would rather talk to us or show your process on screen, you can schedule a call (write Carolin at [e-mail]) or upload a screen recording.

---

We created a test case that executes the not-yet-tested code block at https://searchfox.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp#5002.
However, the test is missing a functional check (is(..) or ok(..)) to check that the behavior of the code block is correct.

Please complete the test and add it to the test suite, if you think it is worth to do so.

In the attachments we provide the generated test (test.html).
It reaches the targeted code block through the stacktrace in stacktrace.txt.
We also provide some additional generated tests that target the same location (alternative_test_N.html).

Comment 1

[Carolin Brandt]

Attachment: stacktrace.txt

Comment 2

[Carolin Brandt]

Attachment: alternative_test_1.html

Comment 3

[Carolin Brandt]

Attachment: alternative_test_2.html

Comment 4

[Carolin Brandt]

Attachment: alternative_test_3.html

Comment 5

[Carolin Brandt]

Recent changes made the original searchfox link invalid,
here is a permalink to the code targeted by our test case: https://searchfox.org/mozilla-central/rev/00ea1649b59d5f427979e2d6ba42be96f62d6e82/layout/generic/nsBlockFrame.cpp#5027

Comment 6

[Emilio Cobos Álvarez (:emilio)]

This looks potentially interesting. This seems to affect line alignment of blocks split across columns. My guess is that some of the test-cases could be cleaned up and landed, but we should make sure our behavior actually makes sense.

Ting-Yu, do you happen to have the cycles to poke at this?

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Unfortunately, none of the testcases exercises this while-loop in IsLastLine().

However, I've crafted a testcase to exercise the loop, and our behavior is the same as other engines. If the while-loop is removed, the rendering is changed, which is an evidence that the while-loop is required. I'll attach a patch with the testcase shortly.

Comment 8

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1817235 - Add a web-platform test to exercise text-align-last for a block split in multicol.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf609827a2dc
Add a web-platform test to exercise text-align-last for a block split in multicol. r=layout-reviewers,jfkthame

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/39210 for changes under testing/web-platform/tests

Comment 11

[Carolin Brandt]

Thanks for adding the test Ting-Yu!
I have two questions to help me improve the testcases I propose :)

How did you determine that the testcases I attached did not execute the loop?
How did they influence you when crafting your testcase?

Comment 12

[Emilio Cobos Álvarez (:emilio)]

I assume Ting-Yu meant that none of the current test-cases exercised it (and yours did), but maybe I read that backwards :)

Comment 13

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/cf609827a2dc

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 15

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Thanks for raising the questions!

How did you determine that the testcases I attached did not execute the loop?

I manually added printf in the loop, and I don't see any log being printed. Also, this while-loop is checking next-in-flow, which can only be reachable in multi-column layout with at least two columns or in printing environment with at least two pages. I guess maybe something has been changed since you file this bug that makes your testcase invalid?

How did they influence you when crafting your testcase?

All of your testcases use text-align-last, and we check text-align and text-align-last before calling IsLastLine() https://searchfox.org/mozilla-central/rev/736eb58cd30da3afc0310b58232a1e4d0dcc86a4/layout/generic/nsBlockFrame.cpp#5176-5177. Both are great hints when I write my own testcase.

I'm also interested in how you produced the stacktrace in comment 1.

Comment 16

[Carolin Brandt]

We inserted a MOZ_RELEASE_ASSERT in the uncovered blocks of the while loop, and used a fuzzer to generate possible testcases until one hit this assertion. The crash from hitting the release assertion produced the stacktrace and we saved the testcases that created it (the ones I attached).
Indeed, it could be file changes between me generating the testcases and now that made it invalid :)

Thanks for explaining your steps, great to hear what the testcases made it easier for you!