Ben Kelly [:bkelly, not reviewing] Assignee
	Description • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 1 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 2 • 8 years ago

See comment 0 and comment 1 for a description of the problem.

	Andrew Sutherland [:asuth] (he/him)
	Comment 3 • 8 years ago

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.

	Pulsebot
	Comment 4 • 8 years ago

Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1b25168d2cc
Make ServiceWorkerManager::CreateNewRegistration() safer. r=asuth

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 5 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/d1b25168d2cc

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 6 • 8 years ago

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

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 7 • 8 years ago

Comment on attachment 8832057 [details] [diff] [review]
Make ServiceWorkerManager::CreateNewRegistration() safer. r=asuth

Fixes potential problem in SW code, Aurora53+, Beta52+

	Ryan VanderMeulen [:RyanVM]
	Comment 8 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/c7fe5329bb0a

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/933c8878910e