118.71 KB, text/html
4.74 KB, application/zip
2.74 KB, text/html; charset=UTF-8
1014 bytes, patch
|Details | Diff | Splinter Review|
59 bytes, text/x-review-board-request
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.
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
Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=84f4a96a130b7b1c51e4dd13a9eb342517bc9dc3&tochange=ad208e73ab6cd3fb26734bf4a9c19004b9675b27 Regressed by: Bug 1316482
Severity: normal → major
Has Regression Range: --- → yes
Has STR: --- → yes
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.
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. 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....  https://dxr.mozilla.org/mozilla-central/rev/cad53f061da634a16ea75887558301b77f65745d/layout/generic/nsTextFrame.cpp#1360
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
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
status-firefox-esr52: unaffected → wontfix
Version: 53 Branch → 52 Branch
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.
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.
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.
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...
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.
Oh it looks like jfkthame had found that magic number in comment 9.
Attachment #8878021 - Attachment mime type: text/html → text/html; charset=UTF-8
I don't quite have time to look into this deeper currently... Hopefully jfkthame has some time frame for this.
(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.
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.
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.
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
(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 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+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/999889ae6e62 Look for text run boundary based on transformed text when needed. r=jfkthame
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?
(In reply to Pulsebot from comment #24) > Pushed by email@example.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.
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.
It would also be good to know if your local build can pass the two new tests.
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.
FWIW, my local build of 999889ae6e62 does not crash with the link in bug 1373586
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.
(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 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+
status-firefox55: affected → fixed
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.
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)
You need to log in before you can comment on or make changes to this bug.