Some floating point intricacies here...patch follows.
Created attachment 154644 [details] Avoiding assertion failure.
Comment on attachment 154644 [details] Avoiding assertion failure. I think at the very least this deserves a comment in the code. But it is not clear from the ChangeLog why the sum of glyph advances would not be cleanly representable in float? Are you suggesting that the advances are all zero? Could the reason for that be a differen one, i.e. lack of glyphs present in the given font for a given string?
Can you add a LayoutTest that triggers the ASSERT? That may make it more clear what this change is doing.
Comment on attachment 154644 [details] Avoiding assertion failure. View in context: https://bugs.webkit.org/attachment.cgi?id=154644&action=review > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:351 > + int nextX = currentX + static_cast<int>(m_harfbuzzRuns[i]->width()); Couldn't we use float instead casting int?
Comment on attachment 154644 [details] Avoiding assertion failure. I think that there is consensus that this needs either a test or more information how exactly this bug is triggered.
(In reply to comment #5) > (From update of attachment 154644 [details]) > I think that there is consensus that this needs either a test or more information how exactly this bug is triggered. Well, I can trigger this if I select text like crazy back and forth with the mouse on a page that has complex text. Isn't that a great reproduction? ;-) What i see in this case is a mismatch of for example 85.999975 vs. 86 when entering the function. I will try to come up with a test case - not sure if I will succeed. (In reply to comment #4) > (From update of attachment 154644 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154644&action=review > > > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:351 > > + int nextX = currentX + static_cast<int>(m_harfbuzzRuns[i]->width()); > > Couldn't we use float instead casting int? For nextX and currentX you mean? Sure can, but I thought you had a perfomance reason or something for using int's all over the place here. Maybe that assumption was wrong?
(In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 154644 [details] [details]) > > I think that there is consensus that this needs either a test or more information how exactly this bug is triggered. > > Well, I can trigger this if I select text like crazy back and forth with the mouse on a page that has complex text. Isn't that a great reproduction? ;-) What i see in this case is a mismatch of for example 85.999975 vs. 86 when entering the function. I will try to come up with a test case - not sure if I will succeed. Perhaps you can figure out which sequence of fonts and characters causes this with excessive debug output right before the ASSERT?
(In reply to comment #6) > (In reply to comment #5) > For nextX and currentX you mean? Sure can, but I thought you had a perfomance reason or something for using int's all over the place here. Maybe that assumption was wrong? Yes. Sorry for confusion, but when I started writing the code, I used ComplexControllerHarfBuzz::offsetForPosition() as a reference. I prefer accuracy than performance here. As Tony and Simon suggest, please consider adding a test to the patch.
Created attachment 154941 [details] Mouse selection fuzzing test. So, this is the best I could come up with. It fuzzes the mouse selection in a brute force way. This test crashes 100% reliably on my machine before the DRT timeout. Requires the Telugu Lohi font to be installed into the EFL DRT fonts directory like: cp /usr/share/fonts/truetype/ttf-telugu-fonts/lohit_te.ttf WebKitBuild/Dependencies/Source/webkitgtk-test-fonts-0.0.3 Unfortunately, this doesn't really work as a regression test eventually. I also tried replaying exactly the same sequence of random numbers to reproduce the crash, but that doesn't work. Suggestions on how to exercise this code path in a different way are welcome. Can we agree that there is a problem and change the types to float here - which makes sense not only for the assertion failure issue, but in general, for reasons of precision?
(In reply to comment #9) > Can we agree that there is a problem and change the types to float here - which makes sense not only for the assertion failure issue, but in general, for reasons of precision? I have no objection if you describe why adding a test is difficult in ChangeLog.
(In reply to comment #10) > (In reply to comment #9) > > > Can we agree that there is a problem and change the types to float here - which makes sense not only for the assertion failure issue, but in general, for reasons of precision? > > I have no objection if you describe why adding a test is difficult in ChangeLog. It would also be OK to add this to WebKit/ManualTests (maybe with harfbuzz in the test name?).
Created attachment 155236 [details] Fix for assertion being hit, test case.
Comment on attachment 155236 [details] Fix for assertion being hit, test case. View in context: https://bugs.webkit.org/attachment.cgi?id=155236&action=review > ManualTests/harfbuzz-mouse-selection-crash.html:14 > +layoutTestController.waitUntilDone(); if (testRunner) testRunner.waitUntilDone(); ? How does this test finish successfully? > ManualTests/harfbuzz-mouse-selection-crash.html:18 > + var body = document.body; We prefer 4-spaces indentation. > ManualTests/harfbuzz-mouse-selection-crash.html:24 > + eventSender.mouseMoveTo(xStart, yStart); Please ensure evenSender exists. > ManualTests/harfbuzz-mouse-selection-crash.html:29 > +/* console.log("{x:"+randomX+",y:"+randomY+"} "); */ Please remove this. > ManualTests/harfbuzz-mouse-selection-crash.html:40 > +<body onload="mouseSelection()"><!-- It would be better to include the following description as a part of the test (not as comment).
Comment on attachment 155236 [details] Fix for assertion being hit, test case. View in context: https://bugs.webkit.org/attachment.cgi?id=155236&action=review >> ManualTests/harfbuzz-mouse-selection-crash.html:14 >> +layoutTestController.waitUntilDone(); > > if (testRunner) > testRunner.waitUntilDone(); > > ? > > How does this test finish successfully? if (testRunner) --> if (window.testRunner)
Created attachment 155253 [details] Fix for assertion being hit, test case, v2.
(In reply to comment #13) > (From update of attachment 155236 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155236&action=review > > > ManualTests/harfbuzz-mouse-selection-crash.html:14 > > +layoutTestController.waitUntilDone(); > > if (testRunner) > testRunner.waitUntilDone(); Changed to if (window.testRunner) testRunner.waitUntilDone(); > How does this test finish successfully? Changed, now using a large enough number of iterations to reliably crash. Completes successfully if it doesn't crash. Note added in the description. > > ManualTests/harfbuzz-mouse-selection-crash.html:18 > > + var body = document.body; > > We prefer 4-spaces indentation. Done. > > ManualTests/harfbuzz-mouse-selection-crash.html:24 > > + eventSender.mouseMoveTo(xStart, yStart); > > Please ensure evenSender exists. Done, checking for window.eventSender > > ManualTests/harfbuzz-mouse-selection-crash.html:29 > > +/* console.log("{x:"+randomX+",y:"+randomY+"} "); */ > > Please remove this. Done. > > ManualTests/harfbuzz-mouse-selection-crash.html:40 > > +<body onload="mouseSelection()"><!-- > > It would be better to include the following description as a part of the test (not as comment). Changed.
Comment on attachment 155253 [details] Fix for assertion being hit, test case, v2. This code change seems fine. In the test, would it be possible to run the test until it crashes while recording the mouseMoveTo positions, then use the mouse positions to make a reliable ASSERT? You could take the minimum number of mouse positions to hit the ASSERT and create an automated layout test for it. It would also be OK to run such a layout test on all ports because it should still pass on all ports. Hmm, I guess you need a certain font installed. Does it repro with either lohit_hi.ttf, lohit_ta.ttf, or MuktiNarrow.ttf from ttf-indic-fonts-core? We already use those fonts on Chromium Linux. This manual test also seems OK.
Tony, thanks for the r+ - comments below. CC'ing Raphael, cq+? Kiitos paljon! :-) (In reply to comment #17) > (From update of attachment 155253 [details]) > This code change seems fine. > > In the test, would it be possible to run the test until it crashes while recording the mouseMoveTo positions, then use the mouse positions to make a reliable ASSERT? You could take the minimum number of mouse positions to hit the ASSERT and create an automated layout test for it. As mentioned in comment 9, I tried recording and replaying, but that didn't do it for some unknown reason. :-( > It would also be OK to run such a layout test on all ports because it should still pass on all ports. > > Hmm, I guess you need a certain font installed. Does it repro with either lohit_hi.ttf, lohit_ta.ttf, or MuktiNarrow.ttf from ttf-indic-fonts-core? We already use those fonts on Chromium Linux. I can give this a try, with a different text and these fonts and see what happens.
Comment on attachment 155253 [details] Fix for assertion being hit, test case, v2. Clearing flags on attachment: 155253 Committed r124111: <http://trac.webkit.org/changeset/124111>
All reviewed patches have been landed. Closing bug.
*** Bug 40673 has been marked as a duplicate of this bug. ***