Closed
Bug 1269407
Opened 9 years ago
Closed 9 years ago
isSupportedSystem early-return breaks initialization assumptions - Crash in java.lang.NullPointerException: at org.mozilla.gecko.BrowserApp.onStart(BrowserApp.java)
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(firefox47 fixed, firefox48 fixed, firefox49 fixed, fennec47+)
RESOLVED
FIXED
Firefox 49
People
(Reporter: mcomella, Assigned: mcomella)
References
Details
(Keywords: crash)
Crash Data
Attachments
(2 files)
58 bytes,
text/x-review-board-request
|
sebastian
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details |
9.65 KB,
patch
|
Details | Diff | Splinter Review |
[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)
Assignee | ||
Comment 1•9 years ago
|
||
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
Comment 2•9 years ago
|
||
(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.
Assignee | ||
Comment 3•9 years ago
|
||
(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)
Assignee | ||
Comment 4•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8751974 -
Flags: review?(s.kaspari) → review+
Comment 5•9 years ago
|
||
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! :)
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/ea0021eed8cd7658fe0e247cacb4022506b6968d
Bug 1269407 - Return early from all lifecycle methods if not supported system. r=sebastian
Assignee | ||
Comment 7•9 years ago
|
||
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•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
status-firefox48:
--- → affected
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)
Assignee | ||
Comment 11•9 years ago
|
||
Assignee | ||
Comment 12•9 years ago
|
||
Branch patch also applies to 47:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=2253bf1d43ce
Aurora is closed atm.
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+
Assignee | ||
Comment 15•9 years ago
|
||
Flags: needinfo?(wkocher)
Flags: needinfo?(michael.l.comella)
Comment 16•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #15)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/a9a6da099c59
setting flags
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•