MozReview Request: Bug 1269407 - Return early from all lifecycle methods if not supported system. r=sebastian
58 bytes, text/x-review-board-request
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 : searchEngineManager.getEngine(new UploadTelemetryCallback(BrowserApp.this)); So searchEngineManager must be null. searchEngineManager is constructed in onCreate – unless we return early from onCreate for !isSupportedSystem . 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? : http://mxr.mozilla.org/mozilla-beta/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#1076 : I checked in a separate application & onStart is still called after onCreate returns early
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. :)
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! :)
https://hg.mozilla.org/integration/fx-team/rev/ea0021eed8cd7658fe0e247cacb4022506b6968d Bug 1269407 - Return early from all lifecycle methods if not supported system. r=sebastian
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 on attachment 8751974 [details] MozReview Request: Bug 1269407 - Return early from all lifecycle methods if not supported system. r=sebastian Crash fix, Aurora48+, Beta47+
Hitting conflicts uplifting to aurora.
Branch patch also applies to 47: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=2253bf1d43ce Aurora is closed atm.
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!
tracking-fennec: ? → 48+
tracking-fennec: 48+ → 47+
(In reply to Michael Comella (:mcomella) from comment #15) > https://hg.mozilla.org/releases/mozilla-aurora/rev/a9a6da099c59 setting flags
You need to log in before you can comment on or make changes to this bug.