1329669 - Crash in mozilla::dom::cache::CacheOpChild::Recv__delete__

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

This bug was filed from the Socorro interface and is 
report bp-e4251650-8df6-472e-8ec3-7c7492170106.
=============================================================

When the parent side fails to open the storage we return a nullptr actor back to the child.  The child, however, tries to use the actor without checking for nullptr.  I think that is causing this low frequency crash.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Well, in theory we should be sending an error code in this case, but it seems we sometimes still have a nullptr actor.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Gracefully handle nullptr Cache actor in CacheStorage::Open() result. r=asuth

This patch tries to verify the actor pointer provided in the OpenStorageResult is valid before using it.  In theory this should not happen when we have a successful result code, so the patch also does a MOZ_DIAGNOSTIC_ASSERT().  For release builds, though, we reject with a generic TypeError message.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8825079 [details] [diff] [review]
Gracefully handle nullptr Cache actor in CacheStorage::Open() result. r=asuth

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

Wow, this is a weird error!  Anyways, I think it probably also makes sense to add:
- MOZ_DIAGNOSTIC_ASSERT(mCacheId != INVALID_CACHE_ID) just before both of the success "return rv" paths in Manager::StorageOpenAction::RunSyncWithDBOnTarget to ensure nothing insane is happening in those methods.  These seem unlikely to screw up, but make the next assertion more useful...
- Add MOZ_DIAGNOSTIC_ASSERT(aRv.Failed() || mCacheId != INVALID_CACHE_ID) before the aListener->OnOpComplete call.  This should ideally help verify that no code screwed up and invoked Resolve(NS_OK) somehow, somewhere.  (And because this is literally the only call-site to construct a StorageOpenResult() and CacheOpChild is switching on the discriminated union type, it should guarantee this assertion gets a chance to do its thing and cause an early explosion closer to the point of failure than the diagnostic in the child.)

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1329669_openfailed.patch

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba05b37f1dd9
Gracefully handle nullptr Cache actor in CacheStorage::Open() result. r=asuth

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8825423 [details] [diff] [review]
bug1329669_openfailed.patch

Approval Request Comment
[Feature/Bug causing the regression]: Service workers
[User impact if declined]: About 10 crashes per week in release and beta channels.
[Is this code covered by automated tests?]: Yes, but it did not catch this rare corner case.
[Has the fix been verified in Nightly?]: We don't have exact steps to reproduce so it cannot be manually verified.
[Needs manual test from QE? If yes, steps to reproduce]: None
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: The patch only adds a nullptr check and some diagnostic assertions.
[String changes made/needed]: None

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 8

[Gerry Chang [:gchang]]

Comment on attachment 8825423 [details] [diff] [review]
bug1329669_openfailed.patch

Fix a crash. Beta51+ & Aurora52+. Should be in 51 beta 14.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0ce0e61bac824bdfab2c73726c856bf9f9b764e0

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/releases/mozilla-beta/rev/614acf4f43e8783b028fd8f95ce31f86cbfed22e