Closed Bug 1445524 Opened 2 years ago Closed 2 years ago

Crash in doLoadFromCommonData

Categories

(Core :: JavaScript: Internationalization API, defect, P1, critical)

Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: marcia, Assigned: Waldo)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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?
Flags: needinfo?(andrebargull)
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.)
Flags: needinfo?(andrebargull)
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.
Flags: needinfo?(jwalden+bmo)
See Also: → 1446113
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.
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(jwalden+bmo)
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.
Attachment #8959889 - Flags: review?(ted)
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.
Attachment #8959889 - Flags: review?(ted) → review+
Priority: -- → P1
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
https://hg.mozilla.org/mozilla-central/rev/1164d9b6127a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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?
Flags: needinfo?(jwalden+bmo)
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
Flags: needinfo?(jwalden+bmo)
Attachment #8959889 - Flags: approval-mozilla-esr52?
Attachment #8959889 - Flags: approval-mozilla-beta?
"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 :/
Attachment #8959889 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Attachment #8959889 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52-
Duplicate of this bug: 1446113
You need to log in before you can comment on or make changes to this bug.