This bug was filed from the Socorro interface and is report bp-589b7799-eb31-40a4-bd7d-c44e30180313. ============================================================= Seen while looking at early 59 Mac crash stats: http://bit.ly/2FIb1zx. This is a startup crash which has 17 crashes/17 installs so far. Although there appears to have been a similar signature in 58.0.2, the stack does look different. All of the crashes have the same crash reason: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS I guessed on the component - perhaps someone can bucket this in a better place if it doesn't belong here. Top 10 frames of crashing thread: 0 XUL doLoadFromCommonData intl/icu/source/common/udata.cpp:952 1 XUL doOpenChoice intl/icu/source/common/udata.cpp:1330 2 XUL icu_60::LoadedNormalizer2Impl::load intl/icu/source/common/loadednormalizer2impl.cpp:80 3 XUL icu_60::Normalizer2::getInstance intl/icu/source/common/loadednormalizer2impl.cpp:126 4 XUL uidna_openUTS46_60 intl/icu/source/common/uts46.cpp:219 5 XUL nsIDNService::nsIDNService netwerk/dns/nsIDNService.cpp:153 6 XUL nsIDNServiceConstructor netwerk/build/nsNetModule.cpp:381 7 XUL nsComponentManagerImpl::CreateInstanceByContractID xpcom/components/nsComponentManager.cpp:1086 8 XUL nsComponentManagerImpl::GetServiceByContractID xpcom/components/nsComponentManager.cpp:1446 9 XUL nsGetServiceByContractID::operator const xpcom/components/nsComponentManagerUtils.cpp:67 =============================================================

This is a startup crash - 90% of crashes had an uptime less than one minute.

Hey Andre, this looks like a regression in the icu update. Can you take a look?

The ICU60 changes for the cpp files on the stack [1,2,3] all seem pretty unlikely to cause these crashes. It looks like ICU mmaps its data, maybe the data file itself is corrupted and things go downhill when interpreting bogus data? [1] https://hg.mozilla.org/mozilla-central/diff/00c94696d5a3/intl/icu/source/common/udata.cpp [2] https://hg.mozilla.org/mozilla-central/diff/00c94696d5a3/intl/icu/source/common/loadednormalizer2impl.cpp [3] https://hg.mozilla.org/mozilla-central/diff/00c94696d5a3/intl/icu/source/common/uts46.cpp > Although there appears to have been a similar signature in 58.0.2, the stack does look different. The stack looks similar enough for me. (ICU's main loading function is |doLoadFromCommonData|, everything else calls into this function, so basically only the top two frames are of interest. And these two frames match in the 59.0 and 58.0.2 reports.)

Thanks Andre. Currently the Mac volume is quite a bit higher than it was on 58.0.2. I also just noticed that one crash has Crash Reason: EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE which is different from the others that have EXC_BAD_ACCESS / KERN_INVALID_ADDRESS - https://crash-stats.mozilla.com/report/index/e5c33792-9b5f-4f53-8800-33c260180314.

Adding waldo in case he has any ideas.

I have no insight into why this is happening. However. The reason we read ICU data from a separate file at all, is because OS X universal binaries (originally PPC/x86, later x86/x86-64) wanted it so they wouldn't have doubled ICU data for universal builds -- universal builds that are gone as of Firefox 53 (bug 1295375). (News to me! Shows how closely I watch broader Mozilla happenings any more. :-| ) There's no reason to ship this data outside the binary any more, and it's only a few-line change to always embed the data. Whatever reason this crash occurs, this change would certainly eliminate it. The fix would be to unconditionally set MOZ_ICU_DATA_ARCHIVE= in build/autoconf/icu.m4. This change is definitely worth doing on trunk (followed by removing a bunch of dead code for when it's non-empty). I'm not sure whether it's a backportable change. Line-wise it's small, and we'd exercise code paths used on other platforms, but it nonetheless is a large ripple effect. Anyway, it's the weekend, just wrapping up from a late-night try-push of this https://treeherder.mozilla.org/#/jobs?repo=try&revision=65b769c2a0c853832e06c9da3767a223ba39f596 so I'll post this and figure out whether to do anything with this come Monday.

Comment on attachment 8959889 [details] [diff] [review] Always embed ICU data directly into the library, not storing it in a separate file in the file system Review of attachment 8959889 [details] [diff] [review]: ----------------------------------------------------------------- It is Monday. I think we can move forward with this on trunk, at an absolute minimum.

Comment on attachment 8959889 [details] [diff] [review] Always embed ICU data directly into the library, not storing it in a separate file in the file system Review of attachment 8959889 [details] [diff] [review]: ----------------------------------------------------------------- I think you're right in that universal binaries were the primary motivation behind this and they're no longer relevant. File a follow-up bug to remove all the conditionals around this? No sense in carrying around extra branches for a feature we're not using.

Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/1164d9b6127a Always embed ICU data directly into the library, not storing it in a separate file in the file system. r=ted

Looks like a scary thing to uplift for a 59 dot release, but an uplift to Beta for 60 seems like it would be a good idea?

Comment on attachment 8959889 [details] [diff] [review] Always embed ICU data directly into the library, not storing it in a separate file in the file system Approval Request Comment [Feature/Bug causing the regression]: ¯\_(ツ)_/¯ [User impact if declined]: Some people will hit crashes for unclear reasons. [Is this code covered by automated tests?]: Well, every startup runs doLoadFromCommonData, so I guess sort of? [Has the fix been verified in Nightly?]: Waiting to watch the crashes go away now. [Needs manual test from QE? If yes, steps to reproduce]: We don't got none, but if you can start up a browser, that will demonstrate this issue not existing *as to a particular installation*. You would need an installation hitting this crash, that you then updated to include this patch, to properly reproduce this as a fix. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: [Why is the change risky/not risky?]: The change made here, pushes Mac builds into a well-used path used by Windows and Android. Still, it remains a notable change. But IMO, it is *risky* mostly only on FUD grounds that are unrebuttable and unprovable and anecdotal at best. Where does that leave things? I think this is something to take on beta, assuming a couple weeks beta-time for experience. And if we have any evidence of crashing on ESR, it's probably worth taking there. But if we don't have such evidence -- which is quite possible, because this could be something that's not likely to affect more locked-down enterprise systems that would be running ESR. [String changes made/needed]: N/A [Approval Request Comment] Fix Landed on Version: Landing on trunk now, will land on beta soonish likely. Risk to taking this patch (and alternatives if risky): See above for risk. No reasonable alternatives to this if deemed risky. String or UUID changes made by this patch: N/A

"But if we don't have such evidence -- which is quite possible, because this could be something that's not likely to affect more locked-down enterprise systems that would be running ESR." ...-- then I'd say don't bother on ESR. (Also don't bother on ESR if that particular ESR is going to go away Real Soon, which maybe it is, I don't pay much attention to release scheduling these days to know.)

Comment on attachment 8959889 [details] [diff] [review] Always embed ICU data directly into the library, not storing it in a separate file in the file system ok let's get this on beta. we don't seem to have any crashes there, only on release, though, so no way to verify :/

Comment on attachment 8959889 [details] [diff] [review] Always embed ICU data directly into the library, not storing it in a separate file in the file system ESR52 is EOL in August and this bug doesn't meet the criteria for uplifting there anyway.