Closed Bug 1386027 Opened 3 years ago Closed 3 years ago

Crash in java.lang.NullPointerException: Attempt to invoke virtual method ''java.lang.Class java.lang.Object.getClass()'' on a null object reference at org.mozilla.gecko.sync.telemetry.TelemetryCollector.setError(TelemetryCollector.java)

Categories

(Firefox for Android :: Android Sync, defect, P1, critical)

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
relnote-firefox --- 55+
firefox54 --- unaffected
firefox55 blocking fixed
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: calixte, Assigned: Grisha)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(3 files)

This bug was filed from the Socorro interface and is 
report bp-56028983-ddf6-4bfe-9c36-b9fd60170731.
=============================================================

There are 5 crashes in nightly 56 with buildids 20170731100320 and 20170730100245.
:Grisha Kruglov, could you investigate please ?
Flags: needinfo?(gkruglov)
I also see crashes in 55, which is to be expected, since Bug 1308337 landed in 55.
Assignee: nobody → gkruglov
Status: NEW → ASSIGNED
Flags: needinfo?(gkruglov)
Priority: -- → P1
This is the top Android crash in Nightly 20170804100335.
Comment on attachment 8892675 [details]
Bug 1386027 - Simplify handleError interfaces for SessionCallback and TelemetryCollector

https://reviewboard.mozilla.org/r/163666/#review170956

Consider uplift.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:191
(Diff revision 1)
>      public void handleAborted(GlobalSession globalSession, String reason) {
>        super.handleAborted(globalSession, reason);
>        // Note to future maintainers: while there are reasons, other than 'backoff', this method
>        // might be called, in practice that _is_ the only reason it gets called at the moment of
>        // writing this. If this changes, please do expand this telemetry handling.
> -      this.telemetryCollector.setError(TelemetryCollector.KEY_ERROR_INTERNAL, "backoff");
> +      this.telemetryCollector.setError(TelemetryCollector.KEY_ERROR_INTERNAL, new BackoffException());

Why not also pass `reason` here, as on line 179?
Attachment #8892675 - Flags: review?(rnewman) → review+
Pushed by gkruglov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2b09cdaae71c
Simplify handleError interfaces for SessionCallback and TelemetryCollector r=rnewman
https://hg.mozilla.org/mozilla-central/rev/2b09cdaae71c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Should we uplift?
Flags: needinfo?(gkruglov)
Yup, I was waiting for a nice soft landing on nightly.
Crash Signature: [@ java.lang.NullPointerException: Attempt to invoke virtual method ''java.lang.Class java.lang.Object.getClass()'' on a null object reference at org.mozilla.gecko.sync.telemetry.TelemetryCollector.setError(TelemetryCollector.java)] → [@ java.lang.NullPointerException: Attempt to invoke virtual method ''java.lang.Class java.lang.Object.getClass()'' on a null object reference at org.mozilla.gecko.sync.telemetry.TelemetryCollector.setError(TelemetryCollector.java)] [@ java.lang.NullPoin…
Flags: needinfo?(gkruglov)
Approval Request Comment

[Feature/Bug causing the regression]: Bug 1308337 or one of its follow-ups

[User impact if declined]: Crash happens when sync fails in certain ways

[Is this code covered by automated tests?]: Somewhat; there are tests that exercise most of this code, just not under every condition.

[Has the fix been verified in Nightly?]: Should be in the next nightly.

[Needs manual test from QE? If yes, steps to reproduce]: Not necessary.

[List of other uplifts needed for the feature/fix]: N/A

[Is the change risky?]: Not really.

[Why is the change risky/not risky?]: Patch is straightforward: it simplifies sync telemetry error handling interface, and ensures its consistent use, getting its consumers to stop passing around nulls.

[String changes made/needed]: N/A
Attachment #8895422 - Flags: approval-mozilla-beta?
Comment on attachment 8895422 [details] [diff] [review]
simplify-telemetry-error-handling-beta.patch

Fix for crashes from telemetry changes in 55, let's uplift to beta 2. 
Includes tests.
Attachment #8895422 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Probably too late for 55.
Tracking as it seems that it is skipping
Should I request an uplift to 55 as well? I was under the impression that it was too late; I'm not sure what "it is skipping" means.
Flags: needinfo?(sledru)
I meant "spiking" :(
please fill the uplift request! 
thanks
Flags: needinfo?(sledru)
Flags: needinfo?(gkruglov)
(same as for beta uplift)

[Feature/Bug causing the regression]: Bug 1308337 or one of its follow-ups

[User impact if declined]: Crash happens when sync fails in certain ways

[Is this code covered by automated tests?]: Somewhat; there are tests that exercise most of this code, just not under every condition.

[Has the fix been verified in Nightly?]: No recorded crashes in Nightly after this landed a few days ago, as far as I can tell. 

[Needs manual test from QE? If yes, steps to reproduce]: Not necessary.

[List of other uplifts needed for the feature/fix]: N/A

[Is the change risky?]: Not really.

[Why is the change risky/not risky?]: Patch is straightforward: it simplifies sync telemetry error handling interface, and ensures its consistent use, getting its consumers to stop passing around nulls.

[String changes made/needed]: N/A
Flags: needinfo?(gkruglov)
Attachment #8896420 - Flags: approval-mozilla-release?
Comment on attachment 8896420 [details] [diff] [review]
simplify-telemetry-error-release.patch

Not really happy about the size of the patch but we will use nightly & beta as testing during the week end to see if it is really that safe.
Attachment #8896420 - Flags: approval-mozilla-release? → approval-mozilla-release+
Added to the 55 release notes:
	Fix a crash with Telemetry
Product: Android Background Services → Firefox for Android
You need to log in before you can comment on or make changes to this bug.