Last Comment Bug 782908 - Update harfbuzz to current version
: Update harfbuzz to current version
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics: Text (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla17
Assigned To: Jacek Caban
:
Mentors:
Depends on: 758236 780409
Blocks: 789687
  Show dependency treegraph
 
Reported: 2012-08-15 03:28 PDT by Jacek Caban
Modified: 2016-04-07 18:19 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1.0 (184.32 KB, patch)
2012-08-15 03:28 PDT, Jacek Caban
jfkthame: review-
Details | Diff | Review
patch (187.58 KB, patch)
2012-08-17 01:46 PDT, Jacek Caban
jfkthame: review+
Details | Diff | Review

Description Jacek Caban 2012-08-15 03:28:17 PDT
Created attachment 652058 [details] [diff] [review]
fix v1.0

I originally intended to fix mingw-w64 compilation breakage, which was one-line change that I fixed upstream:

https://bugs.freedesktop.org/show_bug.cgi?id=53233

Meantime quite a lot changes have been made to harfbuzz and it's usually a good idea to update sooner rather than later. The attached patch updates Mozilla code to upstream 4ac4c6f2e12ddc8bf5e750671321458218b6e0c8 commit.

Here is try commit (looks green so far):

https://tbpl.mozilla.org/?tree=Try&rev=eb338dc01bdb
Comment 1 Jonathan Kew (:jfkthame) 2012-08-15 05:00:19 PDT
Comment on attachment 652058 [details] [diff] [review]
fix v1.0

Looks fine. I was hoping to take another harfbuzz refresh soon, so thanks for doing this.

r=me, assuming the remaining tests come up green on tryserver (still waiting for Windows and Android at this point).
Comment 2 Jonathan Kew (:jfkthame) 2012-08-15 07:51:27 PDT
Comment on attachment 652058 [details] [diff] [review]
fix v1.0

Unfortunately, there's a reftest failure (that looks like it is indeed harfbuzz-related) on WinXP, in layout/reftests/text/arial-bold-lam-alef-1.html. So I'm retracting my r+, at least until we've investigated that. :(
Comment 3 Jonathan Kew (:jfkthame) 2012-08-16 07:53:38 PDT
After a bunch of re-triggers, that WinXP Opt reftest failed on 2 out of 10 runs. So it seems to be intermittent, but it wasn't just a one-off stray cosmic ray.

I'm not aware that this test has been failing before now, and so I suspect it's showing a genuine issue in the harfbuzz update (or an issue in our related code that happens to get exposed by the update).

The failure looks like either the U+FEFB character being mapped to the wrong glyph, or the 'fina' feature being (inappropriately) applied to U+FEFB in isolation. The fact that it's intermittent makes me wonder if perhaps there's an uninitialized value somewhere. Maybe running under valgrind would reveal a culprit, but I don't have time to try that just now.
Comment 4 Behdad Esfahbod 2012-08-16 08:37:01 PDT
Ouch.  OOB.  Fixed upstream.

commit bd08d5d126aa878d1dbf7bfd4b1a764c170cd9ad
Author: Behdad Esfahbod <behdad@behdad.org>
Date:   Thu Aug 16 11:35:50 2012 -0400

    [OT] Fix Arabic shaper OOB access
    
    https://bugzilla.mozilla.org/show_bug.cgi?id=782908

diff --git a/src/hb-ot-shape-complex-arabic.cc b/src/hb-ot-shape-complex-arabic.cc
index e1a2791..3e61e69 100644
--- a/src/hb-ot-shape-complex-arabic.cc
+++ b/src/hb-ot-shape-complex-arabic.cc
@@ -202,7 +202,7 @@ struct arabic_shape_plan_t
   ASSERT_POD ();
 
   bool do_fallback;
-  hb_mask_t mask_array[ARABIC_NUM_FEATURES];
+  hb_mask_t mask_array[ARABIC_NUM_FEATURES + 1];
 };
Comment 5 Jonathan Kew (:jfkthame) 2012-08-16 08:49:46 PDT
Thanks for the quick investigation and fix, Behdad.

Jacek, if you could pull the new HB code including this fix, and put up a fresh patch and tryserver push, that'd be great - thanks!
Comment 6 Jacek Caban 2012-08-17 01:46:51 PDT
Created attachment 652702 [details] [diff] [review]
patch

Thanks for the quick fix! I've attached the updated patch, here is try push:

https://tbpl.mozilla.org/?tree=Try&rev=ada79909d964
Comment 7 Jonathan Kew (:jfkthame) 2012-08-17 10:08:44 PDT
Comment on attachment 652702 [details] [diff] [review]
patch

r=jfkthame, as the patch looks fine and tryserver is looking happy as well... please wait for those last couple of Windows reftests to come in green before actually landing it, though. (The Linux orange is unrelated.)

FTR, this will bring us to bd08d5d126aa878d1dbf7bfd4b1a764c170cd9ad in the upstream repo.
Comment 8 Jacek Caban 2012-08-18 01:21:25 PDT
Thanks for the review before even requesting, you own the record of my review times:) Try finished and it looks good, pushed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dc6b1b9db8e
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-08-18 16:19:55 PDT
https://hg.mozilla.org/mozilla-central/rev/9dc6b1b9db8e

Note You need to log in before you can comment on or make changes to this bug.