1912893 - Extracting the attachments bundle leads to shutdown crash

Status: RESOLVED FIXED

(Firefox :: Remote Settings Client, defect)

Description

[Mathieu Leplatre [:leplatrem]]

The security-state/intermediates collection leads to a lot of crashes on Beta after we landed Bug 1889617

crash stats

It seems that the code is faulty, and that a promise is not awaited during the bundle extraction, which leads to massive number of parallel write on IndexedDB, and crashing in some context (eg. shutdown).

Comment 1

[Alex Cottner]

Attachment: Fix attachments bundle extraction to prevent shutdown crash in IndexedDB

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1889617

Comment 3

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 9418936 [details]
Fix attachments bundle extraction to prevent shutdown crash in IndexedDB

Beta/Release Uplift Approval Request

• User impact if declined: A lot of users (~2%) seem to report crash during shutdown without this patch.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The previous code was initiating promising in a loop, this patch adds a await, which will run the operations of the loop sequentially. The change is trivial.
• String changes made/needed:
• Is Android affected?: Yes

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6fe6944f58a8
Fix attachments bundle extraction to prevent shutdown crash in IndexedDB r=leplatrem

Comment 5

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6fe6944f58a8

Comment 6

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/6ddf0e23d89d

Comment 7

[Pascal Chevrel:pascalc]

Comment on attachment 9418936 [details]
Fix attachments bundle extraction to prevent shutdown crash in IndexedDB

Approved for 130 beta 5, thanks.

Comment 8

[:Gijs (he/him)]

Just to close the loop, from the patch, Nick Alexander asked:

Can we add a test for this, to ensure the queue doesn't grow without bound? (Possibly as follow-up.)

Would bug 1913068 cover this or is there a separate bug (filed or yet to be filed) that would cover this? Could we assert in debug builds or something?

Comment 9

[Mathieu Leplatre [:leplatrem]]

Would bug 1913068 cover this

Not really, this one was more about have an end-to-end test from bundle extraction to indexeddb.

or is there a separate bug (filed or yet to be filed) that would cover this?

I am not sure about this, and/or how to implement such tests.

Could we assert in debug builds or something?

Why not, but what would be a reasonable limit?

We'll definitely want to save attachments in bulk, and opened https://bugzilla.mozilla.org/show_bug.cgi?id=1913142 for this.

If you have a precise idea in mind on how we should do this follow-up, could you please guide us? We would be happy to do the work

Comment 10

[:Gijs (he/him)]

(In reply to Mathieu Leplatre [:leplatrem] from comment #9)

Would bug 1913068 cover this

Not really, this one was more about have an end-to-end test from bundle extraction to indexeddb.

or is there a separate bug (filed or yet to be filed) that would cover this?

I am not sure about this, and/or how to implement such tests.

Could we assert in debug builds or something?

Why not, but what would be a reasonable limit?

We'll definitely want to save attachments in bulk, and opened https://bugzilla.mozilla.org/show_bug.cgi?id=1913142 for this.

If you have a precise idea in mind on how we should do this follow-up, could you please guide us? We would be happy to do the work

I imagine a marionette test could check that downloading and saving the bundles is aborted/completed in a sensible fashion if the browser is shutdown in the interim? Or you could simulate the same in an xpcshell test (which may be easier). I'm not sure if Nick had other ideas. I'd needinfo him but those are blocked while he's on PTO...