Closed Bug 1445462 Opened 2 years ago Closed 2 years ago

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

Categories

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

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 61
Iteration:
60.4 - Mar 12
Tracking Status
firefox60 + fixed
firefox61 --- fixed

People

(Reporter: Grisha, Assigned: Grisha)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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 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?
Attachment #8958630 - Flags: review?(rnewman) → review+
(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.
(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 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
Attachment #8958912 - Flags: review?(rnewman) → review+
Pushed by gkruglov@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/53395fa976d3
https://hg.mozilla.org/mozilla-central/rev/30867952db10
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Nightly crash stats look good. This should be uplifted to 60 if possible.
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.
Attachment #8958912 - Flags: approval-mozilla-beta?
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
Attachment #8958912 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1442351
You need to log in before you can comment on or make changes to this bug.