1384646 - 8,300 instances of "called for non-cluster boundary" emitted from layout/generic/nsTextFrame.cpp during linux64 debug testing

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Eric Rahm [:erahm]]

> 8328 WARNING: called for non-cluster boundary: file layout/generic/nsTextFrame.cpp, line 7710

This warning [1] shows up in the following test suites:

>   4164 - test-linux64/debug-web-platform-tests-3 wpt3
>   4164 - test-linux64/debug-web-platform-tests-e10s-3 wpt3

It shows up in 8 tests. A few of the most prevalent:

>   2082 - [e10s] /css/css-writing-modes-3/text-orientation-script-001.html
>   2082 -        /css/css-writing-modes-3/text-orientation-script-001.html
>   1488 -        /css/css-writing-modes-3/text-orientation-script-001j.html
>   1488 - [e10s] /css/css-writing-modes-3/text-orientation-script-001j.html
>    582 -        /css/css-writing-modes-3/text-orientation-script-001l.html
>    582 - [e10s] /css/css-writing-modes-3/text-orientation-script-001l.html
>     12 - [e10s] /css/css-writing-modes-3/text-orientation-script-001e.html
>     12 -        /css/css-writing-modes-3/text-orientation-script-001e.html

[1] https://hg.mozilla.org/mozilla-central/annotate/e8400551c2e3/layout/generic/nsTextFrame.cpp#l7710

Comment 1

[Jet Villegas (inactive)]

Kato-san: does it make sense that this warning fires for the cases listed? Thx!

Comment 2

[Eric Rahm [:erahm]]

This is currently the #7 most verbose warning during testing.

Comment 3

[Eric Rahm [:erahm]]

Attachment: Remove verbose non-cluster boundary warning

Comment 4

[Eric Rahm [:erahm]]

Looks like Jet ni'd the wrong person, Kato-san can you take a look?

Comment 5

[Makoto Kato [:m_kato]]

Why isn't iterator or inOffset cluster start?

Comment 6

[Jonathan Kew [:jfkthame]]

These testcases try to query getBoundingClientRect() for each individual Unicode character in the test repertoire; but some of those characters are cluster-continuations, so we end up calling nsTextFrame::GetCharacterRectsInRange for a range that does not start/end on cluster boundaries.

It's unclear to me whether the cluster-boundary check in nsTextFrame::UpdateIteratorFromOffset is actually necessary, or if it could simply be removed. This is used by GetPointFromOffset and GetCharacterRectsInRange; would behavior be significantly broken if these methods did not force the input offsets onto cluster boundaries?

Comment 7

[Jonathan Kew [:jfkthame]]

FWIW, I've pushed a try run with this code disabled, just to see if anything obvious breaks....
https://treeherder.mozilla.org/#/jobs?repo=try&revision=04c95b264aa62f1212e8d7c985415aa6baec2f2e

Comment 8

[Eric Rahm [:erahm]]

Jonathan, do you want to take this? FWIW your try push looks okay.

Comment 9

[Eric Rahm [:erahm]]

Also note I confirmed the warning is gone on your try push:

> (venv) erahm@shetland:~/dev/tools/log-spam-hell/logs$ log_spam report --repo try --warning-count 500 04c95b264aa6 | grep 'called for non-cluster boundary'
> (venv) erahm@shetland:~/dev/tools/log-spam-hell/logs$

Comment 10

[Jonathan Kew [:jfkthame]]

I'd like to see what :m_kato thinks about comment 6 before we decide on the way forward here. I believe this functionality may be used by E Asian input methods, but it's not clear to me whether removing the cluster-boundary check would have any bad effects there in practice.

Comment 11

[Makoto Kato [:m_kato]]

(In reply to Jonathan Kew (:jfkthame) from comment #6)
> These testcases try to query getBoundingClientRect() for each individual
> Unicode character in the test repertoire; but some of those characters are
> cluster-continuations, so we end up calling
> nsTextFrame::GetCharacterRectsInRange for a range that does not start/end on
> cluster boundaries.
> 
> It's unclear to me whether the cluster-boundary check in
> nsTextFrame::UpdateIteratorFromOffset is actually necessary, or if it could
> simply be removed. This is used by GetPointFromOffset and
> GetCharacterRectsInRange; would behavior be significantly broken if these
> methods did not force the input offsets onto cluster boundaries?

Hmm, as warning, it is correct...  But I don't hit this warning when debugging cluster issue.. So It is OK to remove this if this becomes noise.

If GetCharacterRectsInRange hits this warning, OS API passes invalid offset, so this warning isn't helpful on this case.

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 8951764 [details] [diff] [review]
Remove verbose non-cluster boundary warning

Review of attachment 8951764 [details] [diff] [review]:
-----------------------------------------------------------------

OK, let's just go ahead and remove the warning here, it doesn't seem to provide any real value.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:erahm, could you have a look please?

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18514e973e55
Remove verbose non-cluster boundary warning. r=jfkthame

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/18514e973e55