isSupportedSystem early-return breaks initialization assumptions - Crash in java.lang.NullPointerException: at org.mozilla.gecko.BrowserApp.onStart(BrowserApp.java)

RESOLVED FIXED in Firefox 47

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

({crash})

unspecified
Firefox 49
Points:
---

Firefox Tracking Flags

(firefox47 fixed, firefox48 fixed, firefox49 fixed, fennec47+)

Details

(crash signature)

Attachments

(2 attachments)

[Tracking Requested - why for this release]:

This bug was filed from the Socorro interface and is 
report bp-867dba12-9b84-4ee6-a6b8-d93782160501.
=============================================================
This crash appears to happen at [1]:
  searchEngineManager.getEngine(new UploadTelemetryCallback(BrowserApp.this));

So searchEngineManager must be null. searchEngineManager is constructed in onCreate – unless we return early from onCreate for !isSupportedSystem [2].

That means that isSupportedSystem's early return breaks assumptions in other lifecycle methods (e.g. onStart/onResume & presumably their shutdown methods) about which members should already be initialized.

Caveat: this crash only occurs two build IDs (20160425004018 & 20160425174457) so maybe it's not actually a problem, but the theory seems sound to me.

Some solutions:
 1) Have !isSupportedSystem call finish but not return early and we can explicitly guard any code that will crash if !isSupportedSystem (this requires us to know which code will crash if !isSupportedSystem – the method currently checks API level & cpu architecture – but these values may change, leaving the any existing checks potentially out of date – but that might be okay).
 2) Fix this specific case and catch future errors in crash-stats like this.

I prefer 1, but we'll have to figure out what crashes on unsupported API levels and architectures.

Snorp, I imagine the architecture crashes are from native code – is that true? Would we be able to prevent crashes on unsupported architectures just by stopping Gecko from loading?

[1]: http://mxr.mozilla.org/mozilla-beta/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#1076
[2]: I checked in a separate application & onStart is still called after onCreate returns early
Flags: needinfo?(snorp)
I tracked 47 for the telemetry changes (which added searchEngineManager) but this may affect other versions. Let's add this to triage to raise awareness.
tracking-fennec: 47+ → ?
Depends on: 1119915
(In reply to Michael Comella (:mcomella) from comment #0)
> I prefer 1, but we'll have to figure out what crashes on unsupported API
> levels and architectures.

I wonder why? Why do we want to guard our code very fine-grained and then run /some/ code, just to tear down the whole app again after that? Let's bail out of all life-cycle methods early and be done with it. There's only a limited number of lifecycle methods (it seems like we use more now; back then I only guarded the minimum needed) but an infinite number of variations of the other code that's always changing.
(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> I wonder why? Why do we want to guard our code very fine-grained and then
> run /some/ code, just to tear down the whole app again after that?

I didn't consider this case:

> bail out of all life-cycle methods early and be done with it.

Emphasis on *all* life-cycle methods. I thought my preference was the better of two evils but you've pointed out a good alternative. :)
Flags: needinfo?(snorp)
This avoids a problem where a lifecycle method may assume a previous lifecycle
method initialized some values but we returned early (e.g. on a not supported
system) so these values were never initialized and the application may crash.

I tested this patch by forcing HardwareUtils.isSupportedSystem to return either
true or false (but both were in a supported device configuration).

Review commit: https://reviewboard.mozilla.org/r/52339/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52339/
Attachment #8751974 - Flags: review?(s.kaspari)
Attachment #8751974 - Flags: review?(s.kaspari) → review+
Comment on attachment 8751974 [details]
MozReview Request: Bug 1269407 - Return early from all lifecycle methods if not supported system. r=sebastian

https://reviewboard.mozilla.org/r/52339/#review49359

Awesome! Thanks for writing this patch.

The usage of mIsAbortingAppLaunch is a good idea! :)
Comment on attachment 8751974 [details]
MozReview Request: Bug 1269407 - Return early from all lifecycle methods if not supported system. r=sebastian

Approval Request Comment
[Feature/regressing bug #]: bug 1119915
[User impact if declined]: Users using unsupported configurations may crash instead of being presented with a helpful prompt telling them their configuration is not supported.

[Describe test coverage new/current, TreeHerder]: Tested locally.
[Risks and why]: Low – there are 7 essential callbacks to the Android lifecycle and we make sure to return early from them so we don't accidentally use resources which were not initialized in other callbacks. We've already done this in one callback and it worked correctly until another patch added used some values which were expected to be initialized sooner so we can extrapolate that this is likely to work in all callbacks.
[String/UUID change made/needed]: None
Attachment #8751974 - Flags: approval-mozilla-beta?
Attachment #8751974 - Flags: approval-mozilla-aurora?

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ea0021eed8cd
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment on attachment 8751974 [details]
MozReview Request: Bug 1269407 - Return early from all lifecycle methods if not supported system. r=sebastian

Crash fix, Aurora48+, Beta47+
Attachment #8751974 - Flags: approval-mozilla-beta?
Attachment #8751974 - Flags: approval-mozilla-beta+
Attachment #8751974 - Flags: approval-mozilla-aurora?
Attachment #8751974 - Flags: approval-mozilla-aurora+
Hitting conflicts uplifting to aurora.
Flags: needinfo?(michael.l.comella)
Based on comment 12, this was committed to Beta47.
Hi Wes, I think we only need to land the aurora patch for this one as Mike committed the beta patch to m-b yesterday. Thanks!
Flags: needinfo?(wkocher)
tracking-fennec: ? → 48+
tracking-fennec: 48+ → 47+
https://hg.mozilla.org/releases/mozilla-aurora/rev/a9a6da099c59
Flags: needinfo?(wkocher)
Flags: needinfo?(michael.l.comella)
You need to log in before you can comment on or make changes to this bug.