Crash in mozilla::dom::cache::TypeUtils::ToInternalRequest

RESOLVED FIXED in Firefox 52

Status

()

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

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 1 bug, {crash})

unspecified
mozilla54
x86
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed, firefox54 fixed)

Details

(crash signature)

Attachments

(1 attachment)

This bug was filed from the Socorro interface and is 
report bp-f1656cad-4227-4d96-873b-542032170130.
=============================================================

I think perhaps this might be caused by assuming success in these jsapi calls here:

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

We should probably handle failure there instead of assuming the Cache object is valid here.  I'm not sure this is truly infallible under OOM conditions, etc.
This patch does two things:

1) Don't assume jsapi success on CacheStorage::Open() promise resolution.  Its not clear this will always succeed if we are out of memory, etc.
2) Ensure we properly execute our failure code if DeleteCache() is called.

These are speculative fixes to try to narrow down and possibly fix the low frequency dom::cache crashes we are seeing in ScriptLoader code.
Attachment #8832090 - Flags: review?(amarchesini)
Comment on attachment 8832090 [details] [diff] [review]
Improve ScriptLoader CacheCreator's ability to handle failures and cancelation. r=baku

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

::: dom/workers/ScriptLoader.cpp
@@ +1657,5 @@
>  
>    JS::Rooted<JSObject*> obj(aCx, &aValue.toObject());
>    Cache* cache = nullptr;
>    nsresult rv = UNWRAP_OBJECT(Cache, obj, cache);
> +  if (NS_FAILED(rv)) {

NS_WARN_IF
this should not really happen, right?
Attachment #8832090 - Flags: review?(amarchesini) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d38b5eb6394f
Improve ScriptLoader CacheCreator's ability to handle failures and cancelation. r=baku
Comment on attachment 8832090 [details] [diff] [review]
Improve ScriptLoader CacheCreator's ability to handle failures and cancelation. r=baku

Approval Request Comment
[Feature/Bug causing the regression]: Service workers
[User impact if declined]: Low frequency crashes
[Is this code covered by automated tests?]: We have extensive service worker tests, but they don't trigger this race condition
[Has the fix been verified in Nightly?]: Its a race condition and difficult to manually verify
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: This patch adds some extra error checking and improves error handling to ensure cleanup is always completed.  It should have minimal risk of regression.
[String changes made/needed]: None
Attachment #8832090 - Flags: approval-mozilla-beta?
Attachment #8832090 - Flags: approval-mozilla-aurora?
Modified from Uplift Dashboard.
https://hg.mozilla.org/mozilla-central/rev/d38b5eb6394f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8832090 [details] [diff] [review]
Improve ScriptLoader CacheCreator's ability to handle failures and cancelation. r=baku

better error handling for Service Workers, aurora53+, beta52+
Attachment #8832090 - Flags: approval-mozilla-beta?
Attachment #8832090 - Flags: approval-mozilla-beta+
Attachment #8832090 - Flags: approval-mozilla-aurora?
Attachment #8832090 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.