1329693 - Crash in mozilla::dom::cache::CacheStorage::MaybeRunPendingRequests

Status: RESOLVED WONTFIX

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

Description

[Ben Kelly [:bkelly, not reviewing]]

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().

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Gracefully handle immediate ActorDestroy() in CacheStorage::ActorCreated(). r=asuth

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.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Could also be related to this crash:

https://crash-stats.mozilla.com/report/index/972c5852-04b1-48a2-94a7-c23172170105

Comment 3

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

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...

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c55138fae801
Gracefully handle immediate ActorDestroy() in CacheStorage::ActorCreated(). r=asuth

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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 6

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 8

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

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

Comment 9

[Guilherme Lima]

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

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 11

[Julien Cristau [:jcristau]]

Mass wontfix for bugs affecting firefox 52.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.