Crash in mozilla::dom::workers::ServiceWorkerPrivate::SpawnWorkerIfNeeded

RESOLVED FIXED in Firefox 54

Status

()

defect
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 1 bug, {crash, regression})

unspecified
mozilla54
Unspecified
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 unaffected, firefox53 unaffected, firefox54 fixed)

Details

(Whiteboard: [e10s-multi:+] , crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

This bug was filed from the Socorro interface and is 
report bp-37e16330-9499-4a6e-8621-42aa12170215.
=============================================================

We are hitting the diagnostic assertion I added in bug 1337543 verifying that we don't get a principal with CSP.  I'm not sure what path is leading to this, though.
About 40 crashes here so far.  I need to look at it.
This just adds a few more assertions to try to narrow down what is happening in nightly channel.  I'd like to land this before the weekend so I have results on monday if possible.
Attachment #8838586 - Flags: review?(amarchesini)
Keywords: leave-open
Attachment #8838586 - Flags: review?(amarchesini) → review+
These assertions don't trigger anything in tests locally, but lets check try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28191ae0f30fe7167b91dd51e820369a165238cb
So, I think maybe I should just make ServiceWorkerPrivate::SpawnWorkerIfNeeded() create a pristine principle without any CSP.  This is easier than trying to make sure we never pass the registration principal somewhere that CSP gets added.
Iteration: --- → 54.2 - Feb 20
Whiteboard: [e10s-multi:+]
Attachment #8838586 - Attachment is obsolete: true
Keywords: leave-open
This moves the place where we sanitize the principal from registration creation to spawning the ServiceWorker thread.  We just can't guarantee the registration principal doesn't get passed somewhere where it gets polluted with CSP.

I thought I would be able to remove override the principal after loading the data from the CacheStorage, but I can't.  It turns out our referrer implementation depends on getting the script URL out of the principal on worker thread.  This is terrible, but the current implementation.  Therefore keep overriding the principal with the final result principal.
Comment on attachment 8838716 [details] [diff] [review]
Create a pristine principal in ServiceWorkerPrivate::SpawnWorkerIfNeeded(). r=baku

I meant to flag for review here before.  Please see comment 5 for the description.
Attachment #8838716 - Flags: review?(amarchesini)
Comment on attachment 8838716 [details] [diff] [review]
Create a pristine principal in ServiceWorkerPrivate::SpawnWorkerIfNeeded(). r=baku

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

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +1736,5 @@
>      return rv;
>    }
>  
> +  nsCOMPtr<nsIURI> uri;
> +  rv = mInfo->GetPrincipal()->GetURI(getter_AddRefs(uri));

GetPrincipal() sounds like it can return nullptr. Maybe would be nice to rename it _or_ to check the return value before using it.
Attachment #8838716 - Flags: review?(amarchesini) → review+
#7 Windows topcrash in Nightly 20170217030229, with 44 occurrences. I'm looking forward to seeing the patch land! :)
(In reply to Andrea Marchesini [:baku] from comment #8)
> GetPrincipal() sounds like it can return nullptr. Maybe would be nice to
> rename it _or_ to check the return value before using it.

I'll file a follow-up for this.
See Also: → 1341040
Updated to remove the principal sanitizing from ServiceWorkerManager::CreateNewRegistration() which is no longer necessary.  I meant to include that here.
Attachment #8838716 - Attachment is obsolete: true
Attachment #8839153 - Flags: review+

Comment 12

2 years ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59e30ba1b38f
Create a pristine principal in ServiceWorkerPrivate::SpawnWorkerIfNeeded(). r=baku

Comment 13

2 years ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3feeb492d8f
Create a pristine principal in ServiceWorkerPrivate::SpawnWorkerIfNeeded(). r=baku

Comment 14

2 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fa61f7d1186
Backed out changeset 59e30ba1b38f on a CLOSED TREE
Note, the comment 14 backout was for comment 12 landing.  Comment 13 is the re-landing.

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d3feeb492d8f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Last crash seems to be in build 20170220030205.  Overall graph of crash shows downward trend.  This seems to have fixed it.
You need to log in before you can comment on or make changes to this bug.