Closed Bug 1329693 Opened 5 years ago Closed 3 years ago

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

Categories

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

x86
Windows 7
defect

Tracking

()

RESOLVED WONTFIX
mozilla53
Tracking Status
firefox-esr45 --- wontfix
firefox50 --- wontfix
firefox51 --- affected
firefox52 --- wontfix
firefox53 --- affected

People

(Reporter: bkelly, Unassigned)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 obsolete file)

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.
Attachment #8825086 - Flags: review?(bugmail)
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...
Attachment #8825086 - Flags: review?(bugmail) → review+
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
Attachment #8825086 - Flags: approval-mozilla-beta?
Attachment #8825086 - Flags: approval-mozilla-aurora?
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.
Attachment #8825086 - Flags: approval-mozilla-beta?
Attachment #8825086 - Flags: approval-mozilla-aurora?
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.
https://hg.mozilla.org/mozilla-central/rev/c55138fae801
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
https://hg.mozilla.org/mozilla-central/rev/a571b65429dc
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8825086 - Attachment is obsolete: true
Assignee: bkelly → nobody
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.
Priority: -- → P3
Mass wontfix for bugs affecting firefox 52.
Closing because no crash reported since 12 weeks.
Status: REOPENED → RESOLVED
Closed: 5 years ago3 years ago
Resolution: --- → WONTFIX
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.