Closed Bug 1636256 Opened 4 years ago Closed 4 years ago

Abort pending importJSONDump tasks in the worker at shutdown

Categories

(Firefox :: Remote Settings Client, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 78
Tracking Status
firefox77 --- fixed
firefox78 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

The beta numbers in https://bugzilla.mozilla.org/show_bug.cgi?id=1605973 and the last few comments there suggest that the worker is starting off significant indexeddb tasks that we then can't finish in time.

We should rewrite the importJSONDump code so its work happens in one transaction, and then abort that transaction when we realize shutdown is happening.

To use a single transaction for importJSONDump, this commit:

  • changes IDBHelpers' executeIDB method to take either a string or array
    pointing to objectStores that the caller wants to use.
  • uses that from RemoteSettingsWorker to start a single transaction using both
    the records and the timestamps store
  • updates bulkOperationHelper to take an optional completion callback, in
    addition to the rejection callback, to be called when all the bulk
    operations are complete
  • uses that optional argument from RemoteSettingsWorker's importDumpIDB
    (the actual implementation of IDB access from importJSONDump) to first
    bulk-import the actual records, and then update the timestamp stored for
    that remote settings collection.

Then to abort that single transaction, this commit:

  • stores pending transactions in a set, similar to what Database.jsm already
    does, and removes items from that set when the promise from executeIDB
    either resolves or rejects.
  • adds a prepareShutdown action on the RemoteSettingsWorker's Agent class,
    to be called by the jsm side of the worker manager when shutdown happens.
    When called, it iterates over the pending transactions and aborts all of
    them.
    This also sets a gShutdown flag.
  • ensures that where code awaits in the middle of an operation, it stops
    (throws) immediately if gShutdown has been set.
  • adds a test to test_shutdown_handling.js to verify that this mechanism now
    stops pending import tasks in the worker.

Finally, as a driveby, fixes an oversight in test_remote_settings_worker.js
where the second .get() call wasn't actually testing whether the
importJSONDump call in the worker had succeeded, because if the collection
was empty it would do the import itself, which I realized when I used similar
code in the shutdown test...

Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/52e5c30dc235
abort importJSONDump tasks in the remote settings worker at shutdown, r=leplatrem,asuth
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 78

Comment on attachment 9146592 [details]
Bug 1636256 - abort importJSONDump tasks in the remote settings worker at shutdown, r?leplatrem!,asuth!

Beta/Release Uplift Approval Request

  • User impact if declined: Shutdown hangs (ie shutdown crashes)
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: Bug 1635408
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is a relatively small JS-only change that improves the shutdown handling of the remote settings worker existing shutdown blocker. We're seeing hundreds of these hangs/crashes on release, and I'd like us to nip those in the bud (and, if there remain significant numbers of 77 crashes, to have time to address them in 78 given it's an ESR cycle). I'd like this to land together with the patch in bug 1635408. Since the changes in this bug and bug 1635408, the crashes on nightly have reduced to 0, but it's hard to be sure how strong the effect will be on beta/release given the difference in number of users.
  • String changes made/needed: nope
Attachment #9146592 - Flags: approval-mozilla-beta?
Depends on: 1635408

Comment on attachment 9146592 [details]
Bug 1636256 - abort importJSONDump tasks in the remote settings worker at shutdown, r?leplatrem!,asuth!

Approve for 77 beta 7, thanks.

Attachment #9146592 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: