Closed Bug 1335425 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Service Workers, defect)

x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

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