This bug was filed from the Socorro interface and is report bp-4eea3f94-e788-40d5-87a8-a64852170106. ============================================================= I think this might be another case where the actor constructor is failing causing ActorDestroy() to be called immediately. In this case the CacheStorage class does not expect the CacheStorageChild actor to be destroed in the middle of ActorCreated().

This fixes CacheStorage::ActorCreated() to expect mActor and mStatus to be modified if the PSendCacheStorageConstructor() synchronously fails. I'm still not 100% sure how we are writing to a bad address in MaybeRunPendingRequests(), but I'm pretty sure this patch is needed.

Could also be related to this crash: https://crash-stats.mozilla.com/report/index/972c5852-04b1-48a2-94a7-c23172170105

Comment on attachment 8825086 [details] [diff] [review] Gracefully handle immediate ActorDestroy() in CacheStorage::ActorCreated(). r=asuth Review of attachment 8825086 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, something is weird with the crash in MaybeRunPendingRequests(). I got more curious than I should, and the disassembly shows MaybeRunPendingRequests() exploding in the VC stack overrun detection prolog on function entry. The PC in question should be saving off the "encrypted" stack pointer (xor'ed with cookie) onto the freshly claimed region of stack. And the stack pointer looks sane enough. (Or at least have nothing to do with the crash address.) So, yeah, not sure what's up with that, but the fact that the pre-patch code would try and do mActor->ClearListener() on a null mActor twice is definitely not improving stability...

Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c55138fae801 Gracefully handle immediate ActorDestroy() in CacheStorage::ActorCreated(). r=asuth

Comment on attachment 8825086 [details] [diff] [review] Gracefully handle immediate ActorDestroy() in CacheStorage::ActorCreated(). r=asuth Approval Request Comment [Feature/Bug causing the regression]: Service workers [User impact if declined]: About 2 crashes per week in release channel. It may be under reported, however, since its more likely to trigger during shutdown. [Is this code covered by automated tests?]: Yes, but our tests do not trigger this rare corner case. [Has the fix been verified in Nightly?]: We don't have exact steps to reproduce so we cannot verify manually. [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?]: Minimal [Why is the change risky/not risky?]: The patch restructures how we handle errors during Cache API initialization but should have no impact on the typical success path. Diagnostic assertions are added as well to ensure things are working as we expect. [String changes made/needed]: None

Comment on attachment 8825086 [details] [diff] [review] Gracefully handle immediate ActorDestroy() in CacheStorage::ActorCreated(). r=asuth Clear the flags as I think this patch is wrong.

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/a571b65429dc8beb6d406cb0d8926353fcda4ba7 My understanding what happened in a failed constructor was wrong. We only do the sync ActorDestroy() in a *parent* call to Send*Constructor(). On the child we just kill the process via FatalError(). Anyway, I don't think this patch is quite right.

Note, the synchronous ActorDestroy in the child may start happening in trunk. See bug 1332054. In that case we should probably land the patch in a different bug.

Mass wontfix for bugs affecting firefox 52.

Closing because no crash reported since 12 weeks.