1445462 - Uploader may try to schedule a task on a queue that's been shutdown

Status: RESOLVED FIXED

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

Description

[:Grisha Kruglov]

Bug 1408710 serialized flow of records at the RecordsChannel level, but forgot to account for some of the failure conditions that may occur while BatchingUploader is processing and uploading records.

Specifically, it didn't account for the fact that 'store' is delegate driven; 'storeDone' would get called after a failure might have occurred during 'process', and might try to schedule a task on a queue that's been shutdown (as a by-product of aborting a flow due to upload's store failure).

A patch is to fix this is relatively straightforward; changing this code so that it's not super awkward is the same as continuing the work Bug 1408710 and friends started - further simplifying 'store' and 'fetch' delegate chains, etc.

Comment 1

[:Grisha Kruglov]

Attachment: Bug 1445462 - Pre: Clean up "ignore records on batch failure" code

This should have been a part of Bug 1408710, but alas, here we are.

Patch changes two things:
- serializes process failures in BatchingUploader if record-to-be-uploaded fails sanity checks and server requirements
-- this helps us short-circuit flow in RecordsChannel
- avoids performing any work in ServerSession's storeDone if flow has been aborted

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

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8958630 [details]
Bug 1445462 - Pre: Clean up "ignore records on batch failure" code

https://reviewboard.mozilla.org/r/227542/#review233344

It'd be nice if `store(record)` returned failure, no?

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploader.java:226
(Diff revision 1)
>      }
>  
>      public void noMoreRecordsToUpload() {
> +        // It's possible that we've hit a failure during an upload.
> +        // If that's the case, bail out. Our delegate chain has already been notified.
> +        if (payloadDispatcher.storeFailed.get() || aborted) {

Reverse these conditions. Not only should the bool check be faster, but I suspect that it will always be true if we get here, no?

Comment 3

[:Grisha Kruglov]

(In reply to Richard Newman [:rnewman] from comment #2)
> It'd be nice if `store(record)` returned failure, no?

Yes! I'm explicitly taking this path to make this very easily upliftable into 60. Otherwise, we'll be looking at a very churny patch.

Comment 4

[:Grisha Kruglov]

Attachment: Bug 1445462 - Ensure tasks aren't scheduled during upload after flow has been aborted

This should have been a part of Bug 1408710, but alas, here we are.

Patch changes two things:
- serializes process failures in BatchingUploader if record-to-be-uploaded fails sanity checks and server requirements
-- this helps us short-circuit flow in RecordsChannel
- avoids performing any work in ServerSession's storeDone if flow has been aborted

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

Comment 5

[:Grisha Kruglov]

Comment on attachment 8958630 [details]
Bug 1445462 - Pre: Clean up "ignore records on batch failure" code

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

Comment 6

[:Grisha Kruglov]

(In reply to Richard Newman [:rnewman] from comment #2)
> Reverse these conditions. Not only should the bool check be faster, but I
> suspect that it will always be true if we get here, no?

Not the case; hopefully the new "pre" cleanup commit makes this clearer.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8958912 [details]
Bug 1445462 - Ensure tasks aren't scheduled during upload after flow has been aborted

https://reviewboard.mozilla.org/r/227780/#review233566

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53395fa976d3
Pre: Clean up "ignore records on batch failure" code r=rnewman
https://hg.mozilla.org/integration/autoland/rev/30867952db10
Ensure tasks aren't scheduled during upload after flow has been aborted r=rnewman

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/53395fa976d3
https://hg.mozilla.org/mozilla-central/rev/30867952db10

Comment 10

[:Grisha Kruglov]

Nightly crash stats look good. This should be uplifted to 60 if possible.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8958912 [details]
Bug 1445462 - Ensure tasks aren't scheduled during upload after flow has been aborted

Approval Request Comment
[Feature/Bug causing the regression]:
  Bug 1408710

[User impact if declined]:
  Crashes.

[Is this code covered by automated tests?]:
  Yes.

[Has the fix been verified in Nightly?]:
  Yes. Crash rate looks good.

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

[List of other uplifts needed for the feature/fix]:
  Just the two commits in this bug:
  https://hg.mozilla.org/mozilla-central/rev/53395fa976d3
  https://hg.mozilla.org/mozilla-central/rev/30867952db10

[Is the change risky?]:
  No.

[Why is the change risky/not risky?]:
  It's relatively simple, with good root cause analysis, and measurable elimination of a crash that was introduced recently.

[String changes made/needed]:
  None.

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 8958912 [details]
Bug 1445462 - Ensure tasks aren't scheduled during upload after flow has been aborted

fix fennec crashes related to sync, beta60+, should be in 60.0b7

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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