kMaxBrokeredLen is set to a small value in shipping builds
Categories
(Core :: Layout: Text and Fonts, defect)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox111 | --- | fixed |
People
(Reporter: gstoll, Assigned: bobowen)
References
Details
Attachments
(1 file, 1 obsolete file)
In security/sandbox/chromium-shim/sandbox/win/src/line_break_common.h, the constant kMaxBrokeredLen is set to 50 if ENABLE_TESTS is defined, with the comment
// Set a low max brokered length for testing to exercise the chunking code.
However, ENABLE_TESTS is defined in almost all builds of Firefox, including shipping builds in Nightly, Beta, and Release channels. I have verified with the 108 Release build that this constant is set to 50 instead of the larger, more efficient value.
I'm not sure how much of a perf impact this has, but it might be worth seeing if we can only set this to 50 if tests are active.
|
Reporter | |
Comment 1•1 year ago
|
||
After talking to :bobowen about this, we can just add a && defined(MOZ_DEBUG), and the tests should still pass in debug and release.
|
|
Reporter | |
Comment 2•1 year ago
|
||
Per the linked bug, this was getting set to a low value in released shipping builds, which may have a performance impact. As a side benefit this means we run tests with the low and the high value, which might catch more problems.
Pushed by gstoll@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bf3bdd2f1384 only set kMaxBrokeredLen to low value in debug mode r=bobowen
|
||
Comment 4•1 year ago
|
||
Backed out for causing for causing wpt failures
Backout link: https://hg.mozilla.org/integration/autoland/rev/e388fe5e76eedb080503b56c054312a88040e71d
INFO - PID 1512 | 1673470812421 Marionette INFO No differences allowed
[task 2023-01-11T21:00:12.513Z] 21:00:12 INFO - TEST-UNEXPECTED-FAIL | /css/css-text/line-breaking/line-breaking-025.html | Testing http://web-platform.test:8000/css/css-text/line-breaking/line-breaking-025.html != http://web-platform.test:8000/css/css-text/line-breaking/reference/line-breaking-025-ref.html
[task 2023-01-11T21:00:12.523Z] 21:00:12 INFO - mismatch reftest has no differences
|
|
Reporter | |
Comment 5•1 year ago
|
||
Hmm, maybe the test does care about this value?
Going to pass this to :bobowen to figure out what to do with this.
|
||
Comment 6•1 year ago
|
||
Is current nightly actually breaking the blue text in http://wpt.live/css/css-text/line-breaking/line-breaking-025.html? Chances are the test was "passing" due to a minor difference in text rendering, rather than actually passing. Specially since it was failing on windows-ccov?
On Linux we don't seem to line-break the blue text.
|
Assignee | |
Comment 7•1 year ago
•
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)
Is current nightly actually breaking the blue text in http://wpt.live/css/css-text/line-breaking/line-breaking-025.html? Chances are the test was "passing" due to a minor difference in text rendering, rather than actually passing. Specially since it was failing on windows-ccov?
On Linux we don't seem to line-break the blue text.
Yes, we are breaking in Nightly at the moment, because of the way we chunk up the text and the fact that kMaxBrokeredLen is mistakenly being shipped with the shorter test length.
As I was writing the patch for this, I think I recall us discussing this and the fact that (with the proper maximum brokered length) chunking would be fairly rare and we believed not finding a break within the 32 chars we search within would also be rare, so we would just not complicate the code further.
This bug seems to show that perhaps the second of those doesn't hold in all languages with Uniscribe, so we need to address it.
Currently, when we have to chunk the text, we go back 32 chars from the end of the chunk and search forwards for breaks to use as a safe starting point for the next chunk. We do this instead of simply searching backwards from the end, because the chunking itself can cause false breaks near the end of the chunk by splitting off characters that change the "meaning" of previous characters.
If we didn't find a break then we start the next chunk from character after this chunk. The problem is Uniscribe seems to often (perhaps always) insert a break at the start of the buffer and we can't safely just set that back to false, because it could be a real break.
So, I propose that in the case where we don't find a break we again jump back 32 chars and start from there (checking that we don't split a surrogate pair). We already know that there are no false breaks in this section and this should guard against causing any at the start of the next chunk.
This will mean that in these (hopefully fairly rare) cases that we may make some more calls, but given that the maximum buffer size is 297 and we've been shipping with a buffer size of 50 anyway, I don't think this will cause a problem.
I have a patch and I'll add a test using the text from the test that highlighted this.
|
|
Assignee | |
Comment 8•1 year ago
|
||
|
|
Assignee | |
Comment 9•1 year ago
|
||
This means that we start from a known non-break and that we shouldn't be in any
danger of causing false breaks once Uniscribe gets to unprocessed characters.
This also makes the crash tests manual and debug only.
Manual because now that the win32k pref is default on and not dynamic the tests
will not run on try any more.
Debug only so that we don't include code in opt builds that is only for manual
tests.
|
||
Updated•1 year ago
|
|
||
Comment 10•1 year ago
|
||
Pushed by bobowencode@gmail.com: https://hg.mozilla.org/integration/autoland/rev/3c7123ec7398 Start next line break chunk from start of previous search when no breaks found. r=jfkthame
|
||
Comment 11•1 year ago
|
||
| bugherder | ||
Description
•