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

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Improve ScriptLoader CacheCreator's ability to handle failures and cancelation. r=baku

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.

Comment 2

[Andrea Marchesini [:baku]]

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?

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d38b5eb6394f
Improve ScriptLoader CacheCreator's ability to handle failures and cancelation. r=baku

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 5

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Modified from Uplift Dashboard.

Comment 6

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

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

Comment 7

[Julien Cristau [:jcristau]]

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+

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ebaae8442fa1

https://hg.mozilla.org/releases/mozilla-beta/rev/a243cd4ce448