1059859 - Race in CrashManager._getStore()

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Biru Mohanathas [:poiru]]

While trying to extend browser_CTP_crashreporting.js with a check to make sure the submission is recorded, I noticed that the store instance was not the same between the CrashStore.addCrash (in CrashService.js) and the CrashStore.addSubmission{Attempt,Request) (in CrashSubmit.jsm) calls. It turned out to be a race in CrashManager._getStore().

Comment 1

[Biru Mohanathas [:poiru]]

Attachment: Fix race in CrashManager._getStore resulting in multiple stores being created

Comment 2

[Benjamin Smedberg]

Comment on attachment 8480670 [details] [diff] [review]
Fix race in CrashManager._getStore resulting in multiple stores being created

I think this._getStoreTask = null; should be in a finally block so that even in the case of exceptions, we don't _getStore for the life of the process.

Comment 3

[Biru Mohanathas [:poiru]]

Attachment: Fix race in CrashManager._getStore() resulting in multiple stores being created

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Comment on attachment 8480670 [details] [diff] [review]
> Fix race in CrashManager._getStore resulting in multiple stores being created
> 
> I think this._getStoreTask = null; should be in a finally block so that even
> in the case of exceptions, we don't _getStore for the life of the process.

Fixed.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8480748 [details] [diff] [review]
Fix race in CrashManager._getStore() resulting in multiple stores being created

Review of attachment 8480748 [details] [diff] [review]:
-----------------------------------------------------------------

Nice catch on this race condition!

::: toolkit/components/crashes/CrashManager.jsm
@@ +594,2 @@
>  
> +    this._getStoreTask = Task.spawn(function* () {

To increase readability, please write this as:

return this._getStoreTask = Task.spawn(function * () { ... });

Comment 5

[Biru Mohanathas [:poiru]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43cb7e41dfd6

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/43cb7e41dfd6