1369985 - A part of long content with tabs and full-width characters is not rendered

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[make.tar.gz]

Attachment: sample.htm

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce:

Tried to render long contents.
Three conditions are required to repro:
1. written in full-width Japanese characters.
2. paragraphs contains contains <br> tags.
3. paragraphs are separated by consecutive tabs.
4. must be long more than (More than 30,000 characters or so)
Please refer to the attached file.

This issue is happened on Firefox which version is 53-55 regardless the OS.


Actual results:

The bottom of content is rendered as empty area without characters.
* We can see "END OF FILE" at the bottom of attached contents on the browsers other than FF.


Expected results:

All of content should be rendered.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=84f4a96a130b7b1c51e4dd13a9eb342517bc9dc3&tochange=ad208e73ab6cd3fb26734bf4a9c19004b9675b27

Regressed by: Bug 1316482

Comment 2

[Jeremy Chen ([e-mail])]

Hmmm... at first, I though this is something about buffer boundary issue, cause it seems to me that the white space processing works fine with the exact same pattern at other different places in the same test file. However, after I added couple more characters, the issue still sustained...:/

I'll take a deeper look later, so keep the ni as a reminder.

Comment 3

[make.tar.gz]

Attachment: 1369985.zip

I'm not sure whether the samples are useful for you, but attached some additional ones.

a. simplified.htm
 Simplified by eliminated tabs, and put the contents from line 2 to 4. The issue still sustained.

b. join2-3.htm
 joined line 2 and 3 of simplified.htm, but the issue still sustained.

c. join3-4.htm
 joined line 3 and 4, so the issue is SOLVED.

d. no_br.htm
 Eliminated <br> in the contents, so the issue is SOLVED.

e. with_half_width.htm
 Put half width characters at the top of each line in the HTML. The issue is SOLVED.

Comment 4

[Jonathan Kew [:jfkthame]]

I notice that the rendering of the testcase cuts off at exactly 200 lines (in the sense of the "hard" line-breaks created by <br>, ignoring "soft" line wrapping). This leads me to suspect there's a connection with the NUM_LINES_TO_BUILD_TEXT_RUNS constant (200) used by BuildTextRuns() in nsTextFrame.cpp.[1]

And indeed, a local build with that value changed shows different behavior (reduce it to 20, and the rendering cuts off earlier -- actually, at 40 lines; increase it to 2000 and there's no longer any truncation).

I haven't totally understood what's going on here, and why particular cases fail while some slight variations don't, but it may be a clue to start from....


[1] https://dxr.mozilla.org/mozilla-central/rev/cad53f061da634a16ea75887558301b77f65745d/layout/generic/nsTextFrame.cpp#1360

Comment 5

[Jonathan Kew [:jfkthame]]

I don't believe this is a regression from bug 1316482 after all (probably not a recent regression at all). Although the original testcase appears to show a regression there, the "simplified" testcase from attachment 8874701 [details] exhibits the same problem in Firefox 52, which predates the landing of bug 1316482.

The problem seems to arise when the EnsureTextRun() call in nsTextFrame::TrimTrailingWhiteSpace fails to create the textrun for the current frame; when the bug occurs, a debug build will issue warnings from the NS_ENSURE_TRUE(mTextRun, result) call there. I'm not sure why this failure happens but I don't think it is really caused by bug 1316482; that change just happened to shift some of the precise edge conditions involved.

Comment 6

[Alice0775 White]

Regression window using the "simplified" testcase from attachment 8874701 [details] :
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e48bc2dc52895542e7988b0eafdda2535f68e639&tochange=be7d9851343b6612a322dfa15b11ec099cd94c18

Regressed by: Bug 1081858

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

That one is probably not the true regression either as it is pretty much the same kind of change as bug 1316482 that just moves the boundary of this issue a bit.

Comment 8

[Alice0775 White]

Via local build.
Last Good: e48bc2dc5289
First Bad: 8d6560b363d2

Regressed by: 
	8d6560b363d2	Kan-Ru Chen — Bug 1081858 - Part 1. Fix aText off-by-one indexing. r=jfkthame

Comment 9

[Jonathan Kew [:jfkthame]]

(In reply to Alice0775 White from comment #8)
> Via local build.
> Last Good: e48bc2dc5289
> First Bad: 8d6560b363d2
> 
> Regressed by: 
> 	8d6560b363d2	Kan-Ru Chen — Bug 1081858 - Part 1. Fix aText off-by-one
> indexing. r=jfkthame

That's very interesting; I was suspecting this may be related to the perf regression in bug 1371351, and it looks like that same changeset may be involved in both cases.

Comment 10

[Alice0775 White]

Hey @kchen

Could you consider backing Bug 1081858 out?
Because, Chrome and Edge behave like Firefox51(i.e before landing in bug 1081858)(tested with attachment 8809207 [details]).
So, web compatibility point of view, Bug 1081858 should be backed out.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

We probably cannot backout bug 1081858 cleanly. We may need to backout bug 1316482 as well, but I'd hope we can fix this rather than backing out those changes...

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Attachment: simplified testcase

So it seems that other content doesn't really matter.

It seems to me:
* This happens when there is a point the new transform rule is applied. Content other than that doesn't really matter.
* The issue is 200 lines aligned, which means, if the above point falls in the first 200 lines, text after 200 lines would not be rendered. If the point falls in the second 200 lines, text after 400 lines would not be rendered.

So there must be some magic number involves somewhere.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Oh it looks like jfkthame had found that magic number in comment 9.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

I don't quite have time to look into this deeper currently... Hopefully jfkthame has some time frame for this.

Comment 15

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #13)
> Oh it looks like jfkthame had found that magic number in comment 9.

Actually it's comment 4

The magic number is NUM_LINES_TO_BUILD_TEXT_RUNS added in bug 397510

It looks like the cut-off is intended. We should rebuild the textrun when the area is scrolled into view but somehow that didn't happen.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Attachment: poc patch

Inspired by bug 1373586 comment 2, I tried to dig a bit deeper into the code around TextContainsLineBreakerWhiteSpace.

I think the reason of this is the inconsistency between BuildTextRunsScanner::FindBoundaries and BuildTextRunsScanner::BuildTextRunForFrames.

In BuildTextRunForFrames, we check TextContainsLineBreakerWhiteSpace on a transformed text, while in FindBoundaries, we use that on the original text. In the past, it isn't a problem, because we never remove breaker whitespace completely (we would at least leave one whitespace), but with bug 1081858 fixed (actually, with its part 1), that can happen when it is between two "segment break skip char"s. This leads to an inconsistency that, FindBoundaries thinks there is a valid textrun boundary, but BuildTextRunForFrames fails to find any, thus it keeps setting mSkipIncompleteTextRuns to true and refuse to build textrun.

The attached file is a poc patch which seems to fix the very-simplified testcase in comment 12. It wouldn't work on some other cases, and this may not be the right place to put this fix at all. But it at least provides some proof to the analysis above.

I don't have a clear idea about what the actual solution should be.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Maybe we should just transform text in FindBoundaries... Not sure what that would mean for performance.

Comment 18

[Kan-Ru Chen [:kanru] (UTC+9)]

I also thought about the approach in the poc patch. I think it is correct that TextContainsLineBreakerWhiteSpace is only valid for transformed text. The poc patch will miss some cases because the white space skipping rule is more complex. So either BuildTextRunsScanner::FindBoundaries should operate on transformed text or it need to transform the text by itself.

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

OK, I have a patch which should fix this issue in principle. At least it seems to fix all of this bug, bug 1371351, and bug 1373586.

I'm not sure whether it would introduce performance regression. Probably not a big deal... We can always deal with it later if that shows up in any profile, I guess.

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1369985 - Look for text run boundary based on transformed text when needed.

Review commit: https://reviewboard.mozilla.org/r/150106/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150106/

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8878850 [details]
Bug 1369985 - Look for text run boundary based on transformed text when needed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150106/diff/1-2/

Comment 22

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #19)
> OK, I have a patch which should fix this issue in principle. At least it
> seems to fix all of this bug, bug 1371351, and bug 1373586.
> 
> I'm not sure whether it would introduce performance regression. Probably not
> a big deal... We can always deal with it later if that shows up in any
> profile, I guess.

Yeah, I'm a little concerned about the added call to TransformText, as it represents another pass over the text (doing a significant amount of work as it goes). And it seems wasteful to create a transformed copy of the whole text, when TextContainsLineBreakerWhiteSpace is only going to look as far as the first whitespace character and then the transformed text will be thrown away.

Maybe we could refactor the logic from TransformText such that TextContainsLineBreakerWhiteSpace could (optionally) apply the transformation on the fly, and return as soon as it has an answer, rather than having to always transform the entire buffer. But I think it makes sense to get this landed first, and verify that it fixes the current issues (and see if it appears in profiles at all) before we tackle further optimization.

Comment 23

[Jonathan Kew [:jfkthame]]

Comment on attachment 8878850 [details]
Bug 1369985 - Look for text run boundary based on transformed text when needed.

https://reviewboard.mozilla.org/r/150106/#review155272

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/999889ae6e62
Look for text run boundary based on transformed text when needed. r=jfkthame

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8878850 [details]
Bug 1369985 - Look for text run boundary based on transformed text when needed.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1081858 (introduced this regression) and bug 1316482 (make it a little more likely to happen)
[User impact if declined]: for some Chinese/Japanese pages, there could be rendering issue (this bug), performance issue (bug 1371351), and crash (bug 1373586)
[Is this code covered by automated tests?]: There are tests added for the rendering issue and the crash.
[Has the fix been verified in Nightly?]: Just landed, so no.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: Moderately risky in terms of performance.
[Why is the change risky/not risky?]: This change adds some significant work in some cases, which may be in a hot path. Pages containing non-ASCII characters could be affected.
[String changes made/needed]: n/a

Comment 26

[Ekanan Ketunuti]

(In reply to Pulsebot from comment #24)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/autoland/rev/999889ae6e62
> Look for text run boundary based on transformed text when needed. r=jfkthame

It didn't fix crash bug 1373586, I'm afraid. bp-f6b549cd-4ca4-4986-8961-92a6b1170620

Sadly, non-Nightly won't have crash symbols.

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Is that crash report from a local build? If you have a local debug build, could you share the crash stack in that bug? (You should be able to see the stack if you disable sandbox or run the build with "./mach run --disable-e10s".)

This patch does fix the crash for me locally.

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

It would also be good to know if your local build can pass the two new tests.

Comment 29

[Ekanan Ketunuti]

No, it's from autoland changeset 999889ae6e62 that was built here https://archive.mozilla.org/pub/firefox/tinderbox-builds/autoland-win32/1497914543/.

Once it landed in Nightly I'll retest again.

Comment 30

[Ekanan Ketunuti]

FWIW, It didn't crash test attachment 8878858 [details].

Comment 31

[Kan-Ru Chen [:kanru] (UTC+9)]

FWIW, my local build of 999889ae6e62 does not crash with the link in bug 1373586

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/999889ae6e62

Comment 33

[Ryan VanderMeulen [:RyanVM]]

We should keep this on the radar for ESR52 too.

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

Is this something we might want to uplift to 55? Sounds like it helps with the perf regression in bug 1371351 also.

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #34)
> Is this something we might want to uplift to 55? Sounds like it helps with
> the perf regression in bug 1371351 also.

I think so, and that's why I requested beta uplift in comment 25. It helps all three bugs to some extent. There are other cases some problems may still exist, though.

Comment 36

[Julien Cristau [:jcristau]]

Comment on attachment 8878850 [details]
Bug 1369985 - Look for text run boundary based on transformed text when needed.

this seems worth taking in beta55.  Thanks Xidorn.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6dfc682e06d6

Comment 38

[Ryan VanderMeulen [:RyanVM]]

This seems like a pretty bad issue for users in affected locales. Did we want to consider it for ESR52 as well? Note that it'll need a rebased patch if the answer is yes.

Comment 39

[Xidorn Quan [:xidorn] UTC+10]

If we don't want to uplift bug 1374625 to ESR52, I don't think we should uplift this either. Basically, bug 1374625 has zero risk, and it protects a kind of crash, while this patch has moderate risk (as I mentioned in comment 25), and doesn't always avoid crash. (The crash may happen less frequent than this rendering issue, though)