Closed Bug 1335414 Opened 3 years ago Closed 3 years ago

ServiceWorkerManager::CreateNewRegistration() is a bit unsafe

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

While looking at crash reports like this:

https://crash-stats.mozilla.com/report/index/362071fb-e72f-4c6e-a8e9-af3622170131

I noticed that ServiceWorkerManager::CreateNewRegistration() is a bit unsafe.  It new's a ref counted object but does not store it in a smart pointer.  Not sure if that is the cause of the crash, but lets fix that.
For example, if AddScopeAndRegistration() does an AddRef() and Release() synchronously it would delete the ServiceWorkerRegistrationInfo object before CreateNewRegistration() returns it.  I don't see this happening right now, but its a bit scary.
See comment 0 and comment 1 for a description of the problem.
Attachment #8832057 - Flags: review?(bugmail)
Comment on attachment 8832057 [details] [diff] [review]
Make ServiceWorkerManager::CreateNewRegistration() safer. r=asuth

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

Yes, it seems like someone was listening to Aerosmith's "Livin' on the Edge" when they wrote this! #topical-pop-culture-reference.
Attachment #8832057 - Flags: review?(bugmail) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1b25168d2cc
Make ServiceWorkerManager::CreateNewRegistration() safer. r=asuth
https://hg.mozilla.org/mozilla-central/rev/d1b25168d2cc
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8832057 [details] [diff] [review]
Make ServiceWorkerManager::CreateNewRegistration() safer. r=asuth

Approval Request Comment
[Feature/Bug causing the regression]: Service workers
[User impact if declined]: Speculative fix for some unsafe code.  Its not clear its causing crashes or not.  It should make it easier to reason about some of the crash reports we are seeing, though.
[Is this code covered by automated tests?]: Service workers are heavily tested.
[Has the fix been verified in Nightly?]: Its a speculative fix, so we don't have exact STR to verify.
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal risk
[Why is the change risky/not risky?]: This modifies the code to use smart pointers instead of raw pointers.  It should have no functional impact at all and makes it easier to reason about the lifetime of memory when looking at crash reports on branches.
[String changes made/needed]: None
Attachment #8832057 - Flags: approval-mozilla-beta?
Attachment #8832057 - Flags: approval-mozilla-aurora?
Comment on attachment 8832057 [details] [diff] [review]
Make ServiceWorkerManager::CreateNewRegistration() safer. r=asuth

Fixes potential problem in SW code, Aurora53+, Beta52+
Attachment #8832057 - Flags: approval-mozilla-beta?
Attachment #8832057 - Flags: approval-mozilla-beta+
Attachment #8832057 - Flags: approval-mozilla-aurora?
Attachment #8832057 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.