1162088 - ServiceWorker is reused between apps with the same origin

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Andrea Marchesini [:baku]]

STR required b2g:

1. in the browser app, opens appA.
2. appA registers a SW and this SW loads data into the cache.
3. install AppA
4. open appA directly (no browser app involved)

the appA is killed because it uses the wrong principal: browser AppId, InBrowserElement.

It seems that SW is reused between browser app and appA. Should:

http://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#2635

use the nsIPrincipal instead the nsIURI?

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I guess so. Shouldn't the SW registrations not be loaded at all for appA after appA is installed? appA in installed form should have its own set of registrations. I think the root cause is if the SWR sends all the registrations to every SWM instance, then InstalledAppA has entries for mServiceWorkerRegistrationInfos mapped from the scope to BrowserAppA's RegistrationInfo. This is bad bad! Perhaps modify mServiceWorkerRegistrationInfos to map from nsIPrincipal->SWRI? I don't know how much work it will be internally. Would it be simpler to send the right subsets from SWR to SWM instances?

Comment 2

[Andrea Marchesini [:baku]]

> mServiceWorkerRegistrationInfos to map from nsIPrincipal->SWRI? I don't know
> how much work it will be internally. Would it be simpler to send the right
> subsets from SWR to SWM instances?

Sending the right subset from SWR to SWM means that we know what SWM is going to open and having the principal when LoadRegistrations is called. Plus, a single process can run multiple apps (the parent process for instance).

I guess the correct way is to change IsAvailableForURI and IsControlled to check also the principal.
and mServiceWorkerRegistrationInfos should have multiple instances for the same origin/scope based on several principals too.

What do you think about this?

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Andrea Marchesini (:baku) from comment #2)
> > mServiceWorkerRegistrationInfos to map from nsIPrincipal->SWRI? I don't know
> > how much work it will be internally. Would it be simpler to send the right
> > subsets from SWR to SWM instances?
> 
> Sending the right subset from SWR to SWM means that we know what SWM is
> going to open and having the principal when LoadRegistrations is called.
> Plus, a single process can run multiple apps (the parent process for
> instance).
> 
> I guess the correct way is to change IsAvailableForURI and IsControlled to
> check also the principal.
> and mServiceWorkerRegistrationInfos should have multiple instances for the
> same origin/scope based on several principals too.
> 
> What do you think about this?

actually any of the SWM::GetRegistration() callers would need the principal correct? As would consumers like Push API etc. which would need to ping the correct principal. Converting it to use principals seems reasonable.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: principal.patch

I'm going to write a mochitest in a separate patch.

Basically with this patch we have an hash table with key 'appId-isInBrowserElement' and the value is a RegistrationDataPerPrincipal data struct containing:

  nsTArray<nsCString> mOrderedScopes;
  nsRefPtrHashtable<nsCStringHashKey, ServiceWorkerRegistrationInfo> mInfos;

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8603360 [details] [diff] [review]
principal.patch

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

I ran out of time today, so haven't finished the review. I'm also flagging ehsan for the custom serialization we are doing of principals.
Please have jdm take a look at the docshell change.

::: dom/push/PushServiceChildPreload.js
@@ +10,5 @@
>                                     "nsIServiceWorkerManager");
>  
>  addMessageListener("push", function (aMessage) {
> +  let principal = content.document.nodePrincipal;
> +  swm.sendPushEvent(principal, aMessage.data.scope, aMessage.data.payload);

I don't think this will work. what is the document pointing to in this case? Push is relying on the ServiceWorkerRegistrationInfo knowing it's own principal, so it only sends the scope. We will have to modify PushService to store some additional info when pages use it. Could you file a dependency and block landing this on it.

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +291,5 @@
> +    };
> +
> +    nsRefPtr<WorkerControlRunnable> runnable =
> +      new ReleaseRunnable(mWorkerPrivate, this);
> +    runnable->Dispatch(nullptr);

Am I understanding correctly that you continue to not let the worker die until this runnable has run to ensure you have a valid mWorkerPrivate? In that case, please add a comment at the top of UpdateRunnable.

::: dom/workers/test/serviceworkers/fetch/https/https_test.js
@@ +1,1 @@
> +dump("BAKU 1\n");

:)

@@ +5,5 @@
>    }));
>  });
>  
>  self.addEventListener("fetch", function(event) {
> +dump("BAKU 2: " + event.request.url + "\n");

:)

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 8603360 [details] [diff] [review]
principal.patch

jdm, can you take a look at the nsDocShell changes?

Comment 7

[Andrea Marchesini [:baku]]

Attachment: principal.patch

Path rebased, plus first comments applied.

Comment 8

[(no longer active)]

Comment on attachment 8603807 [details] [diff] [review]
principal.patch

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

r=me on ServiceWorkerManager::PrincipalToScopeKey.  I didn't look at the rest, please let me know if you need me to.

::: dom/workers/ServiceWorkerManager.cpp
@@ +2356,5 @@
> +ServiceWorkerManager::PrincipalToScopeKey(nsIPrincipal* aPrincipal,
> +                                          nsACString& aKey)
> +{
> +  MOZ_ASSERT(aPrincipal);
> +  aKey.Truncate();

Nit: you can truncate after the IsSystemPrincipal check.

@@ +2388,5 @@
> +  bool inBrowserElement;
> +  rv = aPrincipal->GetIsInBrowserElement(&inBrowserElement);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }

Let's only change aKey when we return NS_OK.

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8603807 [details] [diff] [review]
principal.patch

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

r=me except for the push changes which need to be removed.

::: dom/workers/ServiceWorkerManager.cpp
@@ +3443,5 @@
>  void
>  ServiceWorkerManager::RemoveRegistration(ServiceWorkerRegistrationInfo* aRegistration)
>  {
>    RemoveRegistrationInternal(aRegistration);
> +#ifdef DEBUG

MOZ_ASSERT is debug only by default, so we don't need this check.

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +248,5 @@
>  
> +// This Runnable needs to have a valid WorkerPrivate. For this reason it is also
> +// a WorkerFeature that is registered before dispatching itself to the
> +// main-thread and it's removed with ReleaseRunnable when the operation is
> +// completed. This will keep the worker alive for the all needed time.

Nit: alive as long as necessary.

Comment 10

[Andrea Marchesini [:baku]]

nsm, I need more info about how you want to fix this push principal issue.

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Andrea Marchesini (:baku) from comment #10)
> nsm, I need more info about how you want to fix this push principal issue.

Just file a follow up bug and CC me and dougt. There is some discussion at [1] that will affect how push will work. I'm going to hack on it today. Please go ahead and break push for now. We can always add a shim that will pick the first principal or something on desktop.

[1]: https://groups.google.com/forum/#!topic/mozilla.dev.platform/2sOcriUyAyI

Comment 12

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0af05d88416

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8603807 [details] [diff] [review]
principal.patch

ehsan gave me the r+ for the docshell changes.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for various web-platform-test failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=9976725&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=9975604&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/321403d331dc

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Also caused https://treeherder.mozilla.org/logviewer.html#?job_id=9975970&repo=mozilla-inbound on multiple platforms.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: principal.patch

jdm, can you take a look at how I use the principal in docShell?
The reason why we had so many broken mochitests is because I did docShell::GetDocument() to retrieve the principal. That created the docShell::mContentViewer.

Now I use nsDocShell::GetInheritedPrincipal().

Comment 17

[Josh Matthews [:jdm]]

Comment on attachment 8608681 [details] [diff] [review]
principal.patch

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

I think Olli should look at this rather than me.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8608681 [details] [diff] [review]
principal.patch

Sorry, I know next to nothing about service worker implementation and I don't understand the security model we're supposed to use in case of b2g+SW.


(I assume appA opened in browser won't use the same SW as AppA installed explicitly.)

Comment 19

[(no longer active)]

Comment on attachment 8608681 [details] [diff] [review]
principal.patch

Boris, do you mind looking at the docshell bits, please?  (See comment 16 for the background.)

Thanks!

Comment 20

[Andrea Marchesini [:baku]]

Attachment: principal.patch

Comment 21

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8608978 [details] [diff] [review]
principal.patch

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

::: toolkit/content/aboutServiceWorkers.js
@@ +131,5 @@
>  
>    let updateButton = document.createElement("button");
>    updateButton.appendChild(document.createTextNode(bundle.GetStringFromName('update')));
>    updateButton.onclick = function() {
> +    gSWM.softUpdate(info.principal, info.scope);

FYI You missed to update https://mxr.mozilla.org/mozilla-central/source/b2g/components/AboutServiceWorkers.jsm#133

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8608978 [details] [diff] [review]
principal.patch

r- because this is using the principal of the thing that we already have loaded, which is clearly not what you think you're using....

Talking on IRC right now to figure out how this stuff _should_ work.

Comment 23

[Boris Zbarsky [:bzbarsky]]

OK, so summary of IRC discussion....

1)  It's possible that we only really care about our appid/mozbrowser pair.  In which case we should just be passing those, since docshell knows them directly, instead of passing a principal, because the latter just raises questions about which origin the principal should have and whatnot.  But if that's all we really care about, what's the deal with the system/null handling?

2)  The service worker spec is pretty wacky in that it seems to use different service workers for <iframe src="http://bar.com/index.html"> and <object src="http://bar.com/index.html"> when loaded from a web page at http://foo.com, at least as far as Andrea and I can tell.  Is that actually the case?

Comment 24

[Andrea Marchesini [:baku]]

Attachment: principal.patch

This new version uses just appId and inBrowserElement in docShell because these are the only things we need and the only thinks are not going to change in the loading of the URI.

I also disabled system principal. We are not using it right now. Correct?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8610657 [details] [diff] [review]
principal.patch

Please don't use GetInheritedPrincipal in the docshell code.  Just use AppId() and IsInBrowserElement().

Also, softUpdate, getScopeForUrl, sendPushEvent, sendPushSubscriptionChangeEvent need documentation explaining what principal should be passed in.  Without that, it's impossible to tell what callers are supposed to be doing.

Comment 26

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Andrea, does Bug 1165162 allow any simplifications here?

Comment 27

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Not doing reviews right now from comment #23)
> OK, so summary of IRC discussion....
> 
> 1)  It's possible that we only really care about our appid/mozbrowser pair. 
> In which case we should just be passing those, since docshell knows them
> directly, instead of passing a principal, because the latter just raises
> questions about which origin the principal should have and whatnot.  But if
> that's all we really care about, what's the deal with the system/null
> handling?

Are you asking about passing appid and mozbrowser for stuff like sendPushEvent? In that case, yes, we should just pass those. The 'hassle' is having to have all code deal with such things.
Would it make sense to factor the 'key' for many of these operations into some sort of ServiceWorkerId struct which contains a serialized origin (using the changes in Bug 1165162) and scope and use that? Then, for example, PushManager.register(), which knows its scope and principal can pass the serialized id to the parent process, which can store it in IDB and pass it back later when a push is received.

The system/null principal check is probably because we don't support chrome serviceworkers right now.

> 
> 2)  The service worker spec is pretty wacky in that it seems to use
> different service workers for <iframe src="http://bar.com/index.html"> and
> <object src="http://bar.com/index.html"> when loaded from a web page at
> http://foo.com, at least as far as Andrea and I can tell.  Is that actually
> the case?

iframe -> Controlled by bar.com's serviceworker
object/embed -> not intercepted at all, see discussion at https://github.com/slightlyoff/ServiceWorker/issues/249 for reasons. I've filed Bug 1168676 for this.

Comment 28

[Andrea Marchesini [:baku]]

Attachment: principal.patch

Here we use appId+browserElement everywhere (except for the testing method).

Comment 29

[Bobby Holley (:bholley)]

Comment on attachment 8611112 [details] [diff] [review]
principal.patch

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

Post bug 1165162 we shouldn't be explicitly passing appid/inbrowser anywhere for origin checks, and we should instead be passing OriginAttributes around. Ping me if there's any confusion on this.

Comment 30

[Andrea Marchesini [:baku]]

Attachment: principal.patch

Several changes:

1. I use OriginAttributes everywhere instead appId + browserElement.

2. OriginAttributes dictionary is not a direct friend of XPIDL. We have to convert it a object and pass it as jsval, or stringify it. Anyway, instead doing all of this, I prefer a different appraoch: this patch removes IsAvailableForURL and IsControlled and DispatchFetchEvent from XPIDL using directly the SWM singleton in docShell.

3. If you like what I did in point 2, softUpdate will be removed in the e10s about:sw patch.

4. Internally we use OriginAttributes suffix but this is not exposed in anyway from the singleton or from the XPIDL interface.

Comment 31

[Andrea Marchesini [:baku]]

Attachment: principal.patch

Make OriginAttributes::CreateSuffix const.

Comment 32

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8612282 [details] [diff] [review]
principal.patch

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

This looks good, r- only for the job queue bug. I'd like to take a look at the job queue parts (interdiff if possible). Also have bholley feedback? on the OriginAttributes use.

::: dom/workers/ServiceWorkerClients.cpp
@@ +197,5 @@
>      nsRefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
>      MOZ_ASSERT(swm);
>  
> +    nsresult rv =
> +      swm->ClaimClients(mPromiseProxy->GetWorkerPrivate()->GetPrincipal(),

You'll have to move this line below the IsClean() check for safety, where we already have a workerPrivate that can be used.

::: dom/workers/ServiceWorkerManager.cpp
@@ +2422,4 @@
>  /* static */ void
> +ServiceWorkerManager::AddScopeAndRegistration(nsIPrincipal* aPrincipal,
> +                                              const nsACString& aScope,
> +                                              ServiceWorkerRegistrationInfo* aInfo)

Just assert and use aInfo->mPrincipal

::: dom/workers/ServiceWorkerManager.h
@@ +376,5 @@
>    // Set of all documents that may be controlled by a service worker.
>    nsTHashtable<nsISupportsHashKey> mAllDocuments;
>  
>    // Maps scopes to job queues.
>    nsClassHashtable<nsCStringHashKey, ServiceWorkerJobQueue> mJobQueues;

The job queue also needs to be moved into RegistrationDataPerPrincipal, otherwise registrations in two different principals can block each other.

Comment 33

[Andrea Marchesini [:baku]]

Attachment: interdiff

Comment 34

[Andrea Marchesini [:baku]]

Attachment: patch 1

Comment 35

[Andrea Marchesini [:baku]]

Attachment: patch 2

ServiceWorkerRegistrar updated:

1. no system principal
2. use of principal for the unregister

Comment 36

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8612754 [details] [diff] [review]
interdiff

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

::: dom/workers/ServiceWorkerClients.cpp
@@ +194,5 @@
>    NS_IMETHOD
>    Run() override
>    {
>      nsRefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
>      MOZ_ASSERT(swm);

These lines can also be moved so we don't bother getting a swm if the proxy is already clean.

Comment 37

[Bobby Holley (:bholley)]

Comment on attachment 8612755 [details] [diff] [review]
patch 1

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

Comments inline, which should cover most of the originattributes handling. This is too much of a patchbomb for me to be willing to review, especially in code that I'm not familiar with. I'm happy to review atomic chunks related to the OriginAttributes handling.

::: docshell/base/nsDocShell.cpp
@@ +14035,5 @@
>    }
>  
>    if (aIsNavigate) {
> +    OriginAttributes attrs(GetAppId(), GetIsInBrowserElement());
> +    *aShouldIntercept = swm->IsAvailableForURI(attrs, aURI);

Seems like "IsAvailableForURI" should be renamed given that we're also keying it off OriginAttributes - maybe IsAvailable?

@@ +14080,5 @@
> +
> +  OriginAttributes attrs(GetAppId(), GetIsInBrowserElement());
> +
> +  ErrorResult error;
> +  swm->DispatchFetchEvent(attrs, doc, aChannel, isReload, error);

Given that the implementation of this API already grabs the URI off the document, couldn't it also grab the attrs off the document as well?

::: dom/push/PushService.jsm
@@ +1400,5 @@
>              scope
>            );
> +
> +          let data = {
> +            originAttributes: null, // TODO

This needs to be addressed or a followup bug needs to be filed.

@@ +1442,5 @@
>  
>      // TODO data.
>      let data = {
>        payload: "Short as life is, we make it still shorter by the careless waste of time.",
> +      originAttributes: null,

And here - "null" isn't a valid originAttributes value ({} is).

::: dom/workers/ServiceWorkerClients.cpp
@@ +113,5 @@
>  
>      nsRefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
>      nsTArray<ServiceWorkerClientInfo> result;
>  
> +    swm->GetAllClients(mWorkerPrivate->GetPrincipal(), mScope, result);

I have no idea if this is the right principal to be using - somebody who knows how serviceworkers work should check that.

@@ +205,5 @@
>  
>      WorkerPrivate* workerPrivate = mPromiseProxy->GetWorkerPrivate();
>      MOZ_ASSERT(workerPrivate);
>  
> +    nsresult rv = swm->ClaimClients(workerPrivate->GetPrincipal(),

Same here.

::: dom/workers/ServiceWorkerContainer.cpp
@@ +228,5 @@
> +  nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();
> +  MOZ_ASSERT(doc);
> +
> +  nsCOMPtr<nsIPrincipal> principal = doc->NodePrincipal();
> +  MOZ_ASSERT(principal);

No need to assert this, just pass doc->NodePrincipal() directly to GetScopeForURL.

::: dom/workers/ServiceWorkerManager.cpp
@@ +2577,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  // No null principals.

Check for expanded principals here too.

@@ +3453,5 @@
> +                                 const nsACString& aScope)
> +{
> +  MOZ_ASSERT(aPrincipal);
> +
> +  nsAutoCString suffix;

This should be called 'scopeKey' right?

@@ +3463,5 @@
> +  SoftUpdate(suffix, aScope);
> +}
> +
> +void
> +ServiceWorkerManager::SoftUpdate(const nsACString& aOriginAttributesSuffix,

I'd think this should be called aScopeKey - we should avoid exposing the details of what that key is.

@@ +3691,5 @@
> +                                      const nsACString& aScope) const
> +{
> +  MOZ_ASSERT(aPrincipal);
> +
> +  nsAutoCString suffix;

scopeKey.

@@ +3701,5 @@
> +  return GetRegistration(suffix, aScope);
> +}
> +
> +already_AddRefed<ServiceWorkerRegistrationInfo>
> +ServiceWorkerManager::GetRegistration(const nsACString& aOriginAttributesSuffix,

aScopeKey

::: dom/workers/ServiceWorkerManager.h
@@ +432,5 @@
> +  GetServiceWorkerRegistrationInfo(const nsACString& aOriginAttributesSuffix,
> +                                   nsIURI* aURI);
> +
> +  // This method generates a key using appId and isInElementBrowser from the
> +  // principal. We don't use the origin because it can simple change during the

"simple change"?

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +522,5 @@
> +  nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();
> +  MOZ_ASSERT(doc);
> +
> +  nsCOMPtr<nsIPrincipal> principal = doc->NodePrincipal();
> +  MOZ_ASSERT(principal);

Same here. And you can really just inline both of these, since doc->NodePrincipal() is infallible, and implicitly asserts |doc|.

::: toolkit/content/aboutServiceWorkers.js
@@ +131,5 @@
>  
>    let updateButton = document.createElement("button");
>    updateButton.appendChild(document.createTextNode(bundle.GetStringFromName('update')));
>    updateButton.onclick = function() {
> +    gSWM.softUpdate(info.principal.appId, info.principal.isInBrowserElement,

This method now takes originAttributes, right?

Comment 38

[Andrea Marchesini [:baku]]

Attachment: interdiff

Comment 39

[Andrea Marchesini [:baku]]

Attachment: patch 1

Comment 40

[Andrea Marchesini [:baku]]

> > +  swm->DispatchFetchEvent(attrs, doc, aChannel, isReload, error);
> 
> Given that the implementation of this API already grabs the URI off the
> document, couldn't it also grab the attrs off the document as well?

document can be null in this code.

> 
> ::: dom/push/PushService.jsm
> @@ +1400,5 @@
> >              scope
> >            );
> > +
> > +          let data = {
> > +            originAttributes: null, // TODO
> 
> This needs to be addressed or a followup bug needs to be filed.

Yes, Already done.

All the other comments are applied. nsm, reviewed the rest of the patch.

Comment 41

[Bobby Holley (:bholley)]

Comment on attachment 8613384 [details] [diff] [review]
interdiff

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

f=me on this interdiff, but per above I can't really grant review here, because this is an interdiff against a changeset that I'm not comfortable reviewing. I think nsm should give a quick lookover to the pieces he hasn't looked at already.

::: dom/push/PushService.jsm
@@ +1400,5 @@
>              scope
>            );
>  
>            let data = {
> +            originAttributes: {}, // TODO

Mention the followup bug, here and below.

::: dom/workers/ServiceWorkerContainer.cpp
@@ +235,5 @@
>      aRv.Throw(NS_ERROR_FAILURE);
>      return;
>    }
>  
>    aRv = swm->GetScopeForUrl(principal, aUrl, aScope);

Again, this can just be GetOwner()->GetExtantDoc()->NodePrincipal(), inlined directly, here, and eliminate the first 6 lines. Alternatively, you can do:

nsIPrincipal* prin = ...;
swm->GetScopeForURL(prin, ...)

If you want to separate them out.

::: dom/workers/ServiceWorkerManager.cpp
@@ +2599,5 @@
>    if (isNullPrincipal) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  // No expanded principals.

This code appears 3 times. We should factor out the checking for system + null + expanded principal into a method ->IsCodebasePrincipal() on BasePrincipal.

Comment 42

[Andrea Marchesini [:baku]]

Attachment: patch 1

Comment 43

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8613547 [details] [diff] [review]
patch 1

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +2543,2 @@
>    // ordered scopes and registrations better be in sync.
>    MOZ_ASSERT(registration);

Could you add a debug only assert that registration->mPrincipal->originSuffix === aScopeKey?

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +269,5 @@
> +      nsRefPtr<UpdateRunnable> mRunnable;
> +
> +    public:
> +      ReleaseRunnable(WorkerPrivate* aWorkerPrivate,
> +                      UpdateRunnable* aRunnable)

The use of the name 'aRunnable' and 'mRunnable' makes me think it is going to be Run() at some point, when it is acting like a feature. Could you call it aFeature just inside ReleaseRunnable?

Comment 44

[Andrea Marchesini [:baku]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8bcfd31f7d5e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/58aab1e6a65b


https://treeherder.mozilla.org/#/jobs?repo=try&revision=696fea664377

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8bcfd31f7d5e
https://hg.mozilla.org/mozilla-central/rev/58aab1e6a65b

Comment 46

[Noemí Freire (:noemi)]

Hi,

Just adding that it works as expected (appA in step 4 of the description is properly opened) with today's (6/5) master build. Environmental variables:
flame device
Build Id: 20150605052602
Gecko: 63c7e72
Gaia: 5f72017
Platform version: 41.0a1

Thanks!

Comment 47

[Bobby Holley (:bholley)]

Ryan, can you please back this out per bug 1171432, such that the backout is in tonight's Nightly?

Comment 48

[Ryan VanderMeulen [:RyanVM]]

Lots of conflicts on the backout.