1386027 - 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)

Status: RESOLVED FIXED

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

Description

[Calixte Denizet (:calixte)]

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 ?

Comment 1

[:Grisha Kruglov]

I also see crashes in 55, which is to be expected, since Bug 1308337 landed in 55.

Comment 2

[:Grisha Kruglov]

Attachment: Bug 1386027 - Simplify handleError interfaces for SessionCallback and TelemetryCollector

Error reporting, and especially the split between per-stage and global session errors,
are a bit of a mess; at the very least, this patch unifies things a little bit, and
ensures we're not passing around nulls unexpectedly.

Review commit: https://reviewboard.mozilla.org/r/163666/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163666/

Comment 3

[Nicholas Nethercote [inactive]]

This is the top Android crash in Nightly 20170804100335.

Comment 4

[Richard Newman [:rnewman]]

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?

Comment 5

[:Grisha Kruglov]

Comment on attachment 8892675 [details]
Bug 1386027 - Simplify handleError interfaces for SessionCallback and TelemetryCollector

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163666/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b09cdaae71c
Simplify handleError interfaces for SessionCallback and TelemetryCollector r=rnewman

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b09cdaae71c

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Should we uplift?

Comment 9

[:Grisha Kruglov]

Yup, I was waiting for a nice soft landing on nightly.

Comment 10

[:Grisha Kruglov]

Attachment: simplify-telemetry-error-handling-beta.patch

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

Comment 11

[Liz Henry (:lizzard)]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2eabe4c12b9a

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Probably too late for 55.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Tracking as it seems that it is skipping

Comment 15

[:Grisha Kruglov]

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.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

I meant "spiking" :(
please fill the uplift request! 
thanks

Comment 17

[:Grisha Kruglov]

Attachment: simplify-telemetry-error-release.patch

(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

Comment 18

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/361d734f9891

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Added to the 55 release notes:
	Fix a crash with Telemetry