Crash in mozilla::dom::cache::CacheStorage::Delete

RESOLVED FIXED in Firefox 51

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 1 bug, {crash})

unspecified
mozilla53
x86
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox50 wontfix, firefox51 fixed, firefox52 fixed, firefox53 fixed)

Details

(crash signature)

Attachments

(1 attachment)

This bug was filed from the Socorro interface and is 
report bp-53d9068a-3025-4bdc-9769-2d2422170109.
=============================================================

Fair number of crashes in the worker ScriptLoader at address 0x2c when using CacheStorage.  Guessing this is a ScriptLoader problem and not a CacheStorage problem.
This is happening because we try to delete the Cache when we cancel, but we may not be fully initialized if the cancel is initiated here:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#601
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Andrea, this handles the case where we cancel worker script loading here:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#601

Depending on when RunInternal() returns an error we may not have initialized the mCacheStorage yet.  So we cannot unconditionally use it in cancellation.
Attachment #8825095 - Flags: review?(amarchesini)
BTW, this accounts for 70% of the crashes related to dom::cache between Dec 1 and Jan 9.  About 500 crashes.
Attachment #8825095 - Flags: review?(amarchesini) → review+

Comment 4

3 years ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bb63d1c87d7
Handle partial initialized when ScriptLoader is canceled. r=baku
Comment on attachment 8825095 [details] [diff] [review]
Handle partial initialized when ScriptLoader is canceled. r=baku

Approval Request Comment
[Feature/Bug causing the regression]: Service workers
[User impact if declined]: About 200 crashes per week in release channel.
[Is this code covered by automated tests?]: Yes, but it does not trigger this corner case.
[Has the fix been verified in Nightly?]: Its very racy, so we have not been able to reproduce 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?]: No
[Why is the change risky/not risky?]: Its simply an extra nullptr check.  Its very low risk.
[String changes made/needed]: None
Attachment #8825095 - Flags: approval-mozilla-beta?
Attachment #8825095 - Flags: approval-mozilla-aurora?

Comment 6

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8bb63d1c87d7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8825095 [details] [diff] [review]
Handle partial initialized when ScriptLoader is canceled. r=baku

Fix a crash. Beta51+ & Aurora52+. Should be in 51 beta 14.
Attachment #8825095 - Flags: approval-mozilla-beta?
Attachment #8825095 - Flags: approval-mozilla-beta+
Attachment #8825095 - Flags: approval-mozilla-aurora?
Attachment #8825095 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.