Closed Bug 1059859 Opened 7 years ago Closed 7 years ago

Race in CrashManager._getStore()

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: poiru, Assigned: poiru)

References

Details

Attachments

(1 file, 1 obsolete file)

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 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.
Attachment #8480670 - Flags: review?(benjamin) → review-
(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.
Attachment #8480670 - Attachment is obsolete: true
Attachment #8480748 - Flags: review?(benjamin)
Blocks: 1059940
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 * () { ... });
Attachment #8480748 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/43cb7e41dfd6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.