Summary: | [harfbuzz] Crash in harfbuzz because direction is not set | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Claudio Saavedra <csaavedra> | ||||
Component: | WebKitGTK | Assignee: | WebKit Security Group <webkit-security-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | bashi, behdad, cdumez, d-r, jshin, mrobinson, schenney, tony | ||||
Priority: | P2 | ||||||
Version: | 420+ | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Claudio Saavedra
2013-02-18 11:55:22 PST
Sorry, I think I should not be filing bugs when I haven't had a proper meal. Right... I should have expected this coming. The way webkit calls harfbuzz is buggy. I'm compiling webkitgtk now to try to fix this. I've got webkitgtk build here. Any easy way to reproduce the crash (ideally in gdb)? Run epiphany against your wk build (from master) and just start the browser. That should do it. This should do it, though, this area really needs more work. We should simply make direction available to HarfBuzz unconditionally. It's recipe for disaster otherwise. --- Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp.orig 2013-02-19 10:43:39.413756909 -0500 +++ Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp 2013-02-19 10:43:06.730116605 -0500 @@ -327,6 +327,9 @@ hb_buffer_set_script(harfBuzzBuffer.get(), currentRun->script()); if (shouldSetDirection) hb_buffer_set_direction(harfBuzzBuffer.get(), currentRun->rtl() ? HB_DIRECTION_RTL : HB_DIRECTION_LTR); + else + // Leaving direction to HarfBuzz to guess is *really* bad, but will do for now. + hb_buffer_guess_segment_properties (harfBuzzBuffer.get()); // Add a space as pre-context to the buffer. This prevents showing dotted-circle // for combining marks at the beginning of runs. FWIW, this was caused by the following HarfBuzz commit: commit c462b32dcb883a7aca066af24c4d28c7a2b7fa28 Author: Behdad Esfahbod <behdad@behdad.org> Date: Fri Feb 15 07:51:47 2013 -0500 Disable automatic segment properties guessing Before, if one called hb_shape() without setting script, language, and direction on the buffer, hb_shape() was calling hb_buffer_guess_segment_properties() on the user's behalf to guess these. This is very dangerous, since any serious user of HarfBuzz must set these properly (specially important is direction). So now, we don't guess properties by default. People not setting direction will get an abort() now. If the old behavior is desired (fragile, good for simple testing only), users can call hb_buffer_guess_segment_properties() on the buffer just before calling hb_shape(). Created attachment 189091 [details]
Patch
Who can I cc for a review on this area? Hi Tony, I was told you might be able to review this :) TIA Comment on attachment 189091 [details] Patch Attachment 189091 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16616727 Could we have a follow up bug for this to fix the underlying problem? According to comment #6 the real error is in WebKit failing to set all the properties on the text run correctly. Just hacking it in is not the right fix - we should be setting the properties. There is also no test. How are we to reproduce this without building for gtk and running some other piece of software? How will we detect if it fails again? It will crash with *any* complex script test, with recent-enough harfbuzz. The reason only GTK build noticed this is that Chrome runs a static version of harfbuzz. (In reply to comment #12) > It will crash with *any* complex script test, with recent-enough harfbuzz. The reason only GTK build noticed this is that Chrome runs a static version of harfbuzz. And the GTK+ bots are using an old version. (In reply to comment #12) > It will crash with *any* complex script test, with recent-enough harfbuzz. The reason only GTK build noticed this is that Chrome runs a static version of harfbuzz. Follow up bug then, so we fix this properly? Bug 110230 - [harfbuzz] Always pass correct text direction to HarfBuzz Comment on attachment 189091 [details] Patch Already landed on: http://trac.webkit.org/changeset/143337 Build fix in: http://trac.webkit.org/changeset/143345 Comment on attachment 189091 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189091&action=review > Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:334 > + else > + // Leaving direction to HarfBuzz to guess is *really* bad, but will do for now. > + hb_buffer_guess_segment_properties(harfBuzzBuffer.get()); Ouch. Is it correct without { } ? Comment on attachment 189091 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189091&action=review >> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:334 >> + hb_buffer_guess_segment_properties(harfBuzzBuffer.get()); > > Ouch. Is it correct without { } ? In any case, WebKit coding style recommends to use braces in this case: "One-line control clauses should not use braces unless comments are included or a single statement spans multiple lines." (In reply to comment #18) > In any case, WebKit coding style recommends to use braces in this case: > "One-line control clauses should not use braces unless comments are included or a single statement spans multiple lines." My bad. check-webkit-style didn't complain on this one. Do you want me to fix this? (In reply to comment #19) > (In reply to comment #18) > > > In any case, WebKit coding style recommends to use braces in this case: > > "One-line control clauses should not use braces unless comments are included or a single statement spans multiple lines." > > My bad. check-webkit-style didn't complain on this one. Do you want me to fix this? The code works and the patch already landed. I personally think it is fine to leave it as it is. I merely commented for future reference. |