628829 - Updater crashes when localized strings excess 200 utf-8 bytes, as bn-IN, kn, ml, and te do

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

[Marcia Knous [:marcia]]

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]]

If you run the beta 10 installer, does the browser launch?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[LegNeato]

I'm going to mark this as a final hardblocker for now.

Comment 4

[Robert Strong (they/them - no direct email)]

Josh, could you provide some help or insight as to what is going on here?

Comment 5

[Robert Strong (they/them - no direct email)]

Might be helpful to see the update.log from the crash... Henrik or Marcia, could you attach one? Thanks

Comment 6

[Robert Strong (they/them - no direct email)]

btw: that would be the one in the updates/0 directory immediately after the crash

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 (they/them - no direct email)]

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][⌚️UTC+1]]

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 (they/them - no direct email)]

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] (UTC+12)]

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 (they/them - no direct email)]

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] (UTC+12)]

Should see the same crash from b8 too then.

Comment 14

[Robert Strong (they/them - no direct email)]

I did. :(

Comment 15

[Robert Strong (they/them - no direct email)]

Verified that backing out http://hg.mozilla.org/l10n-central/bn-IN/rev/6c0c535a310b fixes this for bi-IN

Comment 16

[Nick Thomas [:nthomas] (UTC+12)]

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 [:Pike]]

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 (they/them - no direct email)]

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 (they/them - no direct email)]

Just tried with the font installed and it failed with the same error. :(

Comment 20

[Axel Hecht [:Pike]]

Suspect: readstrings.h defines max-length to be 200, and the bn-IN strings have more utf-8 chars.

Comment 21

[Robert Strong (they/them - no direct email)]

(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 (they/them - no direct email)]

Attachment: 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 [:Pike]]

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 (they/them - no direct email)]

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 (they/them - no direct email)]

Attachment: patch rev2 - increase the size of MAX_TEXT_LEN (pushed)

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The crash also happened for kn. Axel can you please confirm that this locale is also affected?

Comment 27

[Axel Hecht [:Pike]]

line length is 277, should, yeah.

Comment 28

[Zibi Braniecki [:zbraniecki][:gandalf]]

cannot make it crash on Windows 7 (via VirtualBox)

Comment 29

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: win7 upgrader window

Win7 upgrader window

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 [:Pike]]

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][⌚️UTC+1]]

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][⌚️UTC+1]]

Interestingly there is no crash on Linux:
http://mozmill-archive.brasstacks.mozilla.com/#/update/report/9ab243441dd5e74d70de6904a805597a

Comment 34

[Axel Hecht [:Pike]]

(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][⌚️UTC+1]]

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 (they/them - no direct email)]

Henrik,

Comment 37

[Robert Strong (they/them - no direct email)]

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][⌚️UTC+1]]

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] (UTC+12)]

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 (they/them - no direct email)]

(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 (they/them - no direct email)]

btw: either Henrik or myself will let you know about 3.5

Comment 42

[Robert Strong (they/them - no direct email)]

3.5 is also affected for ml and te

Comment 43

[Robert Strong (they/them - no direct email)]

Now if I only had a time machine to go back and fix this everything would be just grand.

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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 [:Pike]]

(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 (they/them - no direct email)]

(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 [:Pike]]

(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] (UTC+12)]

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] (UTC+12)]

I mean mac-only in comment #48.

Comment 50

[Robert Strong (they/them - no direct email)]

It does... kn on 3.6 doesn't have localized strings.

Comment 51

[Robert Strong (they/them - no direct email)]

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 (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/d72773c698eb

Comment 53

[Robert Strong (they/them - no direct email)]

(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 (they/them - no direct email)]

Attachment: 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 (they/them - no direct email)]

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

[LegNeato]

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 (they/them - no direct email)]

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][⌚️UTC+1]]

Verified fixed with latest updates on trunk for bn-IN, kn, ml, and te on OS X.

Comment 59

[Daniel Veditz [:dveditz]]

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.