Last Comment Bug 628829 - Updater crashes when localized strings excess 200 utf-8 bytes, as bn-IN, kn, ml, and te do
: Updater crashes when localized strings excess 200 utf-8 bytes, as bn-IN, kn, ...
Status: VERIFIED FIXED
[4b10][mozmill][hardblocker]
: crash
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: Trunk
: All Mac OS X
: -- critical (vote)
: mozilla2.0b11
Assigned To: Robert Strong [:rstrong] (use needinfo to contact me)
:
Mentors:
: 646058 (view as bug list)
Depends on: 629256
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-25 15:53 PST by Marcia Knous [:marcia - use ni]
Modified: 2011-03-29 12:16 PDT (History)
13 users (show)
robert.strong.bugs: in‑testsuite-
robert.strong.bugs: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.17+
.17-fixed
.19+
.19-fixed


Attachments
increase the size of MAX_TEXT_LEN (680 bytes, patch)
2011-01-25 20:20 PST, Robert Strong [:rstrong] (use needinfo to contact me)
l10n: feedback+
Details | Diff | Splinter Review
patch rev2 - increase the size of MAX_TEXT_LEN (pushed) (680 bytes, patch)
2011-01-25 20:50 PST, Robert Strong [:rstrong] (use needinfo to contact me)
dtownsend: review+
christian: approval1.9.2.17+
christian: approval1.9.1.19+
Details | Diff | Splinter Review
win7 upgrader window (4.23 KB, image/png)
2011-01-26 02:58 PST, Zibi Braniecki [:gandalf][:zibi]
no flags Details
patch part 2 (pushed) (929 bytes, patch)
2011-01-26 22:39 PST, Robert Strong [:rstrong] (use needinfo to contact me)
dtownsend: review+
christian: approval1.9.2.17-
christian: approval1.9.1.19-
Details | Diff | Splinter Review

Description Marcia Knous [:marcia - use ni] 2011-01-25 15:53:14 PST
Seen while running spot checks for Beta 10. Occurs on 10.5 as well as 10.6. I don't have the fonts installed on my system so I am not sure if that makes a difference.

STR:
1. Download the build from the releases directory
2. Attempt to update to B10

Result: Crash while updater is running. Apple crash reporter is generated.
Comment 1 juan becerra [:juanb] 2011-01-25 15:54:29 PST
If you run the beta 10 installer, does the browser launch?
Comment 2 Henrik Skupin (:whimboo) 2011-01-25 17:42:36 PST
Clearly an updater bug. I have seen the same while running the partial+fallback update tests against all locales for b9->b10.

2011-01-26 02:35:34.020 updater[4725:903] *** Assertion failure in -[NSTextFieldCell _objectValue:forString:errorDescription:], /SourceCache/AppKit/AppKit-1038.35/AppKit.subproj/NSCell.m:1531
2011-01-26 02:35:34.046 updater[4725:903] An uncaught exception was raised
2011-01-26 02:35:34.069 updater[4725:903] Invalid parameter not satisfying: aString != nil
2011-01-26 02:35:34.250 updater[4725:903] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Invalid parameter not satisfying: aString != nil'
*** Call stack at first throw:
(
	0   CoreFoundation                      0x00007fff800b47b4 __exceptionPreprocess + 180
	1   libobjc.A.dylib                     0x00007fff8907c0f3 objc_exception_throw + 45
	2   CoreFoundation                      0x00007fff800b45d7 +[NSException raise:format:arguments:] + 103
	3   Foundation                          0x00007fff85e2d87a -[NSAssertionHandler handleFailureInMethod:object:file:lineNumber:description:] + 198
	4   AppKit                              0x00007fff8042c7b6 -[NSCell _objectValue:forString:errorDescription:] + 168
	5   AppKit                              0x00007fff8042c67d -[NSCell setStringValue:] + 45
	6   AppKit                              0x00007fff804e7881 -[NSControl setStringValue:] + 115
	7   updater                             0x0000000100004858 start + 11544
	8   AppKit                              0x00007fff80405657 -[NSIBObjectData nibInstantiateWithOwner:topLevelObjects:] + 1445
	9   AppKit                              0x00007fff8040388d loadNib + 226
	10  AppKit                              0x00007fff80402f9d +[NSBundle(NSNibLoading) _loadNibFile:nameTable:withZone:ownerBundle:] + 763
	11  AppKit                              0x00007fff80402bd2 +[NSBundle(NSNibLoading) loadNibNamed:owner:] + 326
	12  updater                             0x00000001000049d4 start + 11924
)
terminate called after throwing an instance of 'NSException'
Comment 3 christian 2011-01-25 17:45:27 PST
I'm going to mark this as a final hardblocker for now.
Comment 4 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-25 17:47:39 PST
Josh, could you provide some help or insight as to what is going on here?
Comment 5 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-25 17:53:49 PST
Might be helpful to see the update.log from the crash... Henrik or Marcia, could you attach one? Thanks
Comment 6 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-25 17:54:37 PST
btw: that would be the one in the updates/0 directory immediately after the crash
Comment 7 Henrik Skupin (:whimboo) 2011-01-25 17:58:55 PST
It's quite short. We seem to crash right after we start with the preparation:

SOURCE DIRECTORY /Volumes/data/Firefox.app/Contents/MacOS/updates/0
DESTINATION DIRECTORY /Volumes/data/Firefox.app
PREPARE PATCH Contents/Info.plist
PREPARE PATCH Contents/MacOS/XUL
Comment 8 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-25 18:01:36 PST
Henrik, did you narrow this down to being caused by "system doesn't have supported fonts installed" as well per the summary?
Comment 9 Henrik Skupin (:whimboo) 2011-01-25 18:06:24 PST
I'm running the tests against all locales right now and only bn-IN is causing this problem for me on both machines. None of the other locales are affected. And only for the bn-IN locale there are no supported fonts pre-installed. You should be clearly able to reproduce it with:

ftp://ftp.mozilla.org/pub/firefox/releases/4.0b9/mac/bn-IN/

Steps:
1. Unpack
2. Start Firefox and download update
3. Restart Firefox

The crash will happen right away after the updater appears.
Comment 10 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-25 18:09:57 PST
Marcia, does this also only happen to you with bn-IN?

Henrik, I'd like to confirm whether you narrowed this down to being caused by "system doesn't have supported fonts installed" as well per the summary in case they have different root causes.
Comment 11 Nick Thomas [:nthomas] 2011-01-25 18:13:24 PST
FWIW, the repo for bn-IN didn't change between b9 and b10, but did between b8 and b9. Since we're exercising the updater.app that shipped with b9 the change in
  http://hg.mozilla.org/l10n-central/bn-IN/rev/8cb56ecca0c3
may be relevant.
Comment 12 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-25 18:21:20 PST
Those are in app changes... I suspect the following changeset
http://hg.mozilla.org/l10n-central/bn-IN/rev/6c0c535a310b
Comment 13 Nick Thomas [:nthomas] 2011-01-25 18:23:32 PST
Should see the same crash from b8 too then.
Comment 14 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-25 18:43:23 PST
I did. :(
Comment 15 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-25 19:04:12 PST
Verified that backing out http://hg.mozilla.org/l10n-central/bn-IN/rev/6c0c535a310b fixes this for bi-IN
Comment 16 Nick Thomas [:nthomas] 2011-01-25 19:06:47 PST
Updates have been disabled for b8 & b9 mac bn-IN on beta channel.

[RelEng: in /opt/aus2/incoming/3/Firefox, chmod -v 0 4.0b{8,9}/Darwin*/*/bn-IN/beta/. Both MPT and PHX. The original dir mode was 775]
Comment 17 Axel Hecht [pto-Aug-30][:Pike] 2011-01-25 19:08:26 PST
Fonts for bn-IN are available on http://ekushey.org/?page/mac_download, fyi.

Did anyone try to update b8 to b9 yet?
Comment 18 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-25 19:09:52 PST
btw: I suspect that everything is fine if the font is installed and I highly doubt anyone is running bn-IN without the font installed since it is impossible to read the ui.

I tried both b8 and b9 and both are broken when the font isn't installed. I haven't verified if it works if the font is installed yet.
Comment 19 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-25 19:18:03 PST
Just tried with the font installed and it failed with the same error. :(
Comment 20 Axel Hecht [pto-Aug-30][:Pike] 2011-01-25 19:29:19 PST
Suspect: readstrings.h defines max-length to be 200, and the bn-IN strings have more utf-8 chars.
Comment 21 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-25 20:08:23 PST
(In reply to comment #20)
> Suspect: readstrings.h defines max-length to be 200, and the bn-IN strings have
> more utf-8 chars.
Yes, that is the cause. Thanks!
Comment 22 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-25 20:20:48 PST
Created attachment 507026 [details] [diff] [review]
increase the size of MAX_TEXT_LEN

Axel, at least for Firefox 4.0 I think the value should just be increased. Are you ok with that and do you think we should have other checks in place to protect against this happening again? Perhaps a build time check would be simplest.
Comment 23 Axel Hecht [pto-Aug-30][:Pike] 2011-01-25 20:43:39 PST
Comment on attachment 507026 [details] [diff] [review]
increase the size of MAX_TEXT_LEN

I think you're on the right track for fx4, but according to my grep, Malayalam should be affected by this bug, too, and has an even longer Info string. Including all, the line in the source is 331, the result probably busts 300. That's the longest we have so far, though.

For the record, this is what I'm doing on my local checkouts:

for f in */*/updater/updater.ini;do printf $f" "; egrep ^InfoTe $f|wc -c; done

I'd just bump to 400.
Comment 24 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-25 20:48:53 PST
Can I say damn!!! ;)

The string length has been 200 since this code was implemented.

I'll increase it to 400.
Comment 25 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-25 20:50:10 PST
Created attachment 507032 [details] [diff] [review]
patch rev2 - increase the size of MAX_TEXT_LEN (pushed)
Comment 26 Henrik Skupin (:whimboo) 2011-01-25 23:42:02 PST
The crash also happened for kn. Axel can you please confirm that this locale is also affected?
Comment 27 Axel Hecht [pto-Aug-30][:Pike] 2011-01-26 00:58:03 PST
line length is 277, should, yeah.
Comment 28 Zibi Braniecki [:gandalf][:zibi] 2011-01-26 02:38:36 PST
cannot make it crash on Windows 7 (via VirtualBox)
Comment 29 Zibi Braniecki [:gandalf][:zibi] 2011-01-26 02:58:28 PST
Created attachment 507073 [details]
win7 upgrader window

Win7 upgrader window
Comment 30 Henrik Skupin (:whimboo) 2011-01-26 03:05:00 PST
Axel, would it be helpful to also run our Mozmill update test for all locales on Windows and Linux? I could start those right after Mac is ready.
Comment 31 Axel Hecht [pto-Aug-30][:Pike] 2011-01-26 03:12:07 PST
I think we can do focused tests on bn-IN, kn, ml. gandalf did win, which shows a broken unicode glyph, I suspect that glyph is what osx crashes on. Linux may do either, so seeing what the updater dialog is on linux would be helpful.

Warning, I expect some of this to affect 3.6 and older, too, as we have long strings on the branches, too.
Comment 32 Henrik Skupin (:whimboo) 2011-01-26 03:31:49 PST
Ok, will check linux for those locales right after. The same crash also happens for te.

(In reply to comment #31)
> Warning, I expect some of this to affect 3.6 and older, too, as we have long
> strings on the branches, too.

If anything helps I can trigger a testrun if wanted. No manual work involved here.
Comment 33 Henrik Skupin (:whimboo) 2011-01-26 03:43:22 PST
Interestingly there is no crash on Linux:
http://mozmill-archive.brasstacks.mozilla.com/#/update/report/9ab243441dd5e74d70de6904a805597a
Comment 34 Axel Hecht [pto-Aug-30][:Pike] 2011-01-26 03:53:25 PST
(In reply to comment #33)
> Interestingly there is no crash on Linux:
> http://mozmill-archive.brasstacks.mozilla.com/#/update/report/9ab243441dd5e74d70de6904a805597a

A screenshot of the update progress UI would be helpful to assess the damage.
Comment 35 Henrik Skupin (:whimboo) 2011-01-26 05:15:36 PST
So in the log I find the following message for Linux:

(updater:19475): Pango-WARNING **: Invalid UTF-8 string passed to pango_layout_set_text()

The fonts are looking good on all the named locales. None of them are missing on Ubuntu. So I believe we are fine here.

Anything to do on 3.6?
Comment 36 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-26 12:28:41 PST
Henrik,
Comment 37 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-26 12:33:56 PST
Henrik, 

bn-IN and kn have en-US string on 3.6 and should be fine.

ml and te have localized strings on 3.6 and should be tested

Thanks,
Robert
Comment 38 Henrik Skupin (:whimboo) 2011-01-26 12:43:58 PST
Yes, I'm already testing the 3.6.13 updates. For results see:
http://mozmill-archive.brasstacks.mozilla.com/#/update/reports?branch=3.6&platform=Mac

As Robert already mentioned the same crash happens on 3.6.13 for the ml and te locales. I will run update tests for 3.5.16 right after. Will report back.
Comment 39 Nick Thomas [:nthomas] 2011-01-26 12:52:04 PST
Do we want to disable updates on these other locales ? I will remove the bn-IN ones that are already disabled, since the chmod 0 is causing us issues with a backup.
Comment 40 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-26 12:54:57 PST
(In reply to comment #39)
> Do we want to disable updates on these other locales ? I will remove the bn-IN
> ones that are already disabled, since the chmod 0 is causing us issues with a
> backup.
Yes, the updates for ml and te should be remove from 3.6
Comment 41 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-26 12:55:41 PST
btw: either Henrik or myself will let you know about 3.5
Comment 42 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-26 13:06:46 PST
3.5 is also affected for ml and te
Comment 43 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-26 13:12:56 PST
Now if I only had a time machine to go back and fix this everything would be just grand.
Comment 44 Henrik Skupin (:whimboo) 2011-01-26 14:44:46 PST
(In reply to comment #42)
> 3.5 is also affected for ml and te

The testrun on 3.5 has been completed:
http://mozmill-archive.brasstacks.mozilla.com/#/update/reports?branch=3.5&platform=All

Locales which expose this crash are: kn, ml, and te
Comment 45 Axel Hecht [pto-Aug-30][:Pike] 2011-01-26 14:59:42 PST
(In reply to comment #22)
> Created attachment 507026 [details] [diff] [review]
> increase the size of MAX_TEXT_LEN
> 
> Axel, at least for Firefox 4.0 I think the value should just be increased. Are
> you ok with that and do you think we should have other checks in place to
> protect against this happening again? Perhaps a build time check would be
> simplest.

Now that we have the past in check, post 4.0, I'd favour that code to just not rely on hard-coded string lengths at all.
Comment 46 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-26 15:03:50 PST
(In reply to comment #45)
> (In reply to comment #22)
> > Created attachment 507026 [details] [diff] [review]
> > increase the size of MAX_TEXT_LEN
> > 
> > Axel, at least for Firefox 4.0 I think the value should just be increased. Are
> > you ok with that and do you think we should have other checks in place to
> > protect against this happening again? Perhaps a build time check would be
> > simplest.
> 
> Now that we have the past in check, post 4.0, I'd favour that code to just not
> rely on hard-coded string lengths at all.
I'm for that after Firefox 4.0. I'd also still to enforce a sane length for this string during the locale check though... any reason not to do so?
Comment 47 Axel Hecht [pto-Aug-30][:Pike] 2011-01-26 15:17:50 PST
(In reply to comment #46)
> I'm for that after Firefox 4.0. I'd also still to enforce a sane length for
> this string during the locale check though... any reason not to do so?

I wouldn't know anything "sane". A build-time check isn't really a safe guard these days, as nobody looks at the build results. I don't know anything compelling to hack into compare-locales. OTH, I doubt that I'll forget this anytime soon, and I look at string changes anyway.

I really think we found the corners of the corner cases, investing in infrastructure sounds like a waste of time.
Comment 48 Nick Thomas [:nthomas] 2011-01-26 17:30:51 PST
So the updates to disable are
*) For 4.0
  * bn-IN, for b8,b9 -> b10
  * kn, ml, te for all 4.0
*) For 3.6: ml, te for all versions on branch  (comment #38)
*) For 3.5: kn, ml, te for all version on branch (comment #44)

Does that sound right ?
Comment 49 Nick Thomas [:nthomas] 2011-01-26 17:33:13 PST
I mean mac-only in comment #48.
Comment 50 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-26 17:35:11 PST
It does... kn on 3.6 doesn't have localized strings.
Comment 51 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-26 18:26:08 PST
If we want to make an attempt to re-patriot the affected systems would might be able to get away with a patch that just updates the updater.ini. This is a possibility because there is a call to usleep(500000) prior to showing the ui and it returns early if the update is 50% done after the usleep.
Comment 52 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-26 21:18:18 PST
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/d72773c698eb
Comment 53 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-26 21:27:27 PST
(In reply to comment #51)
> If we want to make an attempt to re-patriot the affected systems would might be
> able to get away with a patch that just updates the updater.ini. This is a
> possibility because there is a call to usleep(500000) prior to showing the ui
> and it returns early if the update is 50% done after the usleep.
If I am reading the data correctly the total ADU's for all platforms for these locales combined is under 1200 so I doubt it will be worth the effort to do this.
Comment 54 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-26 22:39:16 PST
Created attachment 507368 [details] [diff] [review]
patch part 2 (pushed)

I think this extra precaution patch would be a good thing to do. If either of the strings are larger than MAX_TEXT_LEN just continue the update without showing the UI which also prevents the crash.
Comment 55 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-27 13:36:11 PST
Comment on attachment 507368 [details] [diff] [review]
patch part 2 (pushed)

patch part 2 pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/db5d6ac59163
Comment 56 christian 2011-01-27 16:41:49 PST
Comment on attachment 507368 [details] [diff] [review]
patch part 2 (pushed)

Let's just take the space increase on branches, not the UI hiding.
Comment 57 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-28 10:46:41 PST
Pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a84535e18534

Pushed to mozilla-1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b6efde698e76
Comment 58 Henrik Skupin (:whimboo) 2011-02-10 07:18:21 PST
Verified fixed with latest updates on trunk for bn-IN, kn, ml, and te on OS X.
Comment 59 Daniel Veditz [:dveditz] 2011-03-04 10:15:04 PST
The "3.6.15" we're releasing today does not fix this bug, the release containing this bug fix has been renamed to "3.6.16" and the bugzilla flags will be updated to reflect that soon. Today's release is a re-release of 3.6.14 plus a fix for a bug that prevented many Java applets from starting up.
Comment 60 Robert Strong [:rstrong] (use needinfo to contact me) 2011-03-29 12:16:17 PDT
*** Bug 646058 has been marked as a duplicate of this bug. ***

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