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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

[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

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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. :)

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1269407 - Return early from all lifecycle methods if not supported system. r=sebastian

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/

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

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 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/ea0021eed8cd7658fe0e247cacb4022506b6968d
Bug 1269407 - Return early from all lifecycle methods if not supported system. r=sebastian

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ea0021eed8cd

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Hitting conflicts uplifting to aurora.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 48 branch patch

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Branch patch also applies to 47:
  https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=2253bf1d43ce

Aurora is closed atm.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Based on comment 12, this was committed to Beta47.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a9a6da099c59

Comment 16

[Carsten Book [:Tomcat]]

(In reply to Michael Comella (:mcomella) from comment #15)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/a9a6da099c59

setting flags