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

RESOLVED FIXED in Firefox 55

Status

()

--
major
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: make.tar.gz, Assigned: xidorn)

Tracking

(Blocks: 1 bug, {jp-critical, regression})

52 Branch
mozilla56
Unspecified
All
jp-critical, regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox-esr52 wontfix, firefox53 wontfix, firefox54 wontfix, firefox55 fixed, firefox56 fixed)

Details

Attachments

(5 attachments)

(Reporter)

Description

2 years ago
Created attachment 8874093 [details]
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.

Updated

2 years ago
Status: UNCONFIRMED → NEW
status-firefox53: --- → wontfix
status-firefox54: --- → wontfix
status-firefox55: --- → affected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → unaffected
Component: Untriaged → Layout
Ever confirmed: true
Keywords: jp-critical, regression
OS: Unspecified → All
Product: Firefox → Core

Comment 1

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

Regressed by: Bug 1316482
Blocks: 1316482
Severity: normal → major
Has Regression Range: --- → yes
Has STR: --- → yes
Flags: needinfo?(jfkthame)
Flags: needinfo?(jeremychen)
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.
(Reporter)

Comment 3

2 years ago
Created attachment 8874701 [details]
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.
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
Flags: needinfo?(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.
No longer blocks: 1316482
Keywords: regression

Comment 6

2 years ago
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
Blocks: 1081858
status-firefox-esr52: unaffected → wontfix
Flags: needinfo?(kchen)
Keywords: regression
Version: 53 Branch → 52 Branch
Flags: needinfo?(jeremychen)
(Assignee)

Comment 7

2 years ago
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

2 years ago
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
(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

2 years ago
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.
(Assignee)

Comment 11

2 years ago
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...
(Assignee)

Comment 12

2 years ago
Created attachment 8878021 [details]
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.
(Assignee)

Comment 13

2 years ago
Oh it looks like jfkthame had found that magic number in comment 9.
(Assignee)

Updated

2 years ago
Attachment #8878021 - Attachment mime type: text/html → text/html; charset=UTF-8
(Assignee)

Comment 14

2 years ago
I don't quite have time to look into this deeper currently... Hopefully jfkthame has some time frame for this.
(Assignee)

Updated

2 years ago
See Also: → bug 1371351
(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.
Flags: needinfo?(kchen)
(Assignee)

Updated

2 years ago
See Also: → bug 1373586
(Assignee)

Comment 16

2 years ago
Created attachment 8878746 [details] [diff] [review]
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.
(Assignee)

Comment 17

2 years ago
Maybe we should just transform text in FindBoundaries... Not sure what that would mean for performance.
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.
(Assignee)

Comment 19

2 years ago
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.
Assignee: nobody → xidorn+moz
(Assignee)

Updated

2 years ago
Blocks: 1371351
(Assignee)

Updated

2 years ago
Blocks: 1373586
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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

2 years ago
mozreview-review
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
Attachment #8878850 - Flags: review?(jfkthame) → review+

Comment 24

2 years ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/999889ae6e62
Look for text run boundary based on transformed text when needed. r=jfkthame
(Assignee)

Comment 25

2 years ago
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
Attachment #8878850 - Flags: approval-mozilla-beta?

Comment 26

2 years ago
(In reply to Pulsebot from comment #24)
> Pushed by xquan@mozilla.com:
> 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.
(Assignee)

Comment 27

2 years ago
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.
Flags: needinfo?(ananuti)
(Assignee)

Comment 28

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

Comment 29

2 years ago
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.
Flags: needinfo?(ananuti)

Comment 30

2 years ago
FWIW, It didn't crash test attachment 8878858 [details].
FWIW, my local build of 999889ae6e62 does not crash with the link in bug 1373586

Comment 32

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/999889ae6e62
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
We should keep this on the radar for ESR52 too.
status-firefox-esr52: wontfix → affected
Is this something we might want to uplift to 55? Sounds like it helps with the perf regression in bug 1371351 also.
Flags: needinfo?(xidorn+moz)
(Assignee)

Comment 35

2 years ago
(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.
Flags: needinfo?(xidorn+moz)
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.
Attachment #8878850 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 37

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/6dfc682e06d6
status-firefox55: affected → fixed
Flags: in-testsuite+
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.
Flags: needinfo?(xidorn+moz)
(Assignee)

Comment 39

a year ago
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)
Flags: needinfo?(xidorn+moz)
status-firefox-esr52: affected → wontfix
You need to log in before you can comment on or make changes to this bug.