If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

ServiceWorker is reused between apps with the same origin

RESOLVED FIXED in Firefox 41

Status

()

Core
DOM: Service Workers
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla41
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 affected, firefox41 fixed)

Details

Attachments

(3 attachments, 11 obsolete attachments)

16.63 KB, patch
nsm
: review+
Details | Diff | Splinter Review
24.86 KB, patch
Bobby Holley (parental leave - send mail for anything urgent)
: feedback+
Details | Diff | Splinter Review
90.90 KB, patch
nsm
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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?
(Assignee)

Updated

3 years ago
Flags: needinfo?(nsm.nikhil)

Updated

3 years ago
Blocks: 1059784
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?
Flags: needinfo?(nsm.nikhil)
(Assignee)

Comment 2

3 years ago
> 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?
Flags: needinfo?(nsm.nikhil)
(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.
Flags: needinfo?(nsm.nikhil)
(Assignee)

Updated

3 years ago
Assignee: nobody → amarchesini
(Assignee)

Comment 4

3 years ago
Created attachment 8603360 [details] [diff] [review]
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;
Attachment #8603360 - Flags: review?(nsm.nikhil)
(Assignee)

Updated

3 years ago
Component: DOM → DOM: Service Workers
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");

:)
Attachment #8603360 - Flags: review?(ehsan)
(Assignee)

Comment 6

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

jdm, can you take a look at the nsDocShell changes?
Attachment #8603360 - Flags: review?(josh)
(Assignee)

Comment 7

3 years ago
Created attachment 8603807 [details] [diff] [review]
principal.patch

Path rebased, plus first comments applied.
Attachment #8603360 - Attachment is obsolete: true
Attachment #8603360 - Flags: review?(nsm.nikhil)
Attachment #8603360 - Flags: review?(josh)
Attachment #8603360 - Flags: review?(ehsan)
Attachment #8603807 - Flags: review?(nsm.nikhil)
Attachment #8603807 - Flags: review?(josh)
Attachment #8603807 - Flags: review?(ehsan)

Comment 8

3 years ago
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.
Attachment #8603807 - Flags: review?(ehsan) → review+
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.
Attachment #8603807 - Flags: review?(nsm.nikhil) → review+
(Assignee)

Comment 10

3 years ago
nsm, I need more info about how you want to fix this push principal issue.
Flags: needinfo?(nsm.nikhil)
(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
Flags: needinfo?(nsm.nikhil)
(Assignee)

Updated

2 years ago
Blocks: 1166350
(Assignee)

Comment 12

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0af05d88416
(Assignee)

Comment 13

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

ehsan gave me the r+ for the docshell changes.
Attachment #8603807 - Flags: review?(josh)
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
Also caused https://treeherder.mozilla.org/logviewer.html#?job_id=9975970&repo=mozilla-inbound on multiple platforms.
(Assignee)

Comment 16

2 years ago
Created attachment 8608681 [details] [diff] [review]
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().
Attachment #8603807 - Attachment is obsolete: true
Attachment #8608681 - Flags: review?(josh)
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.
Attachment #8608681 - Flags: review?(josh) → review?(bugs)
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.)
Attachment #8608681 - Flags: review?(bugs) → review?(ehsan)

Comment 19

2 years ago
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!
Attachment #8608681 - Flags: review?(ehsan) → review?(bzbarsky)
(Assignee)

Comment 20

2 years ago
Created attachment 8608978 [details] [diff] [review]
principal.patch
Attachment #8608681 - Attachment is obsolete: true
Attachment #8608681 - Flags: review?(bzbarsky)
Attachment #8608978 - Flags: review?(bzbarsky)
Blocks: 1168226
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 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.
Attachment #8608978 - Flags: review?(bzbarsky) → review-
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?
Flags: needinfo?(nsm.nikhil)
Flags: needinfo?(josh)
(Assignee)

Comment 24

2 years ago
Created attachment 8610657 [details] [diff] [review]
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?
Attachment #8608978 - Attachment is obsolete: true
Attachment #8610657 - Flags: review?(ehsan)
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.
Attachment #8610657 - Flags: review-
Andrea, does Bug 1165162 allow any simplifications here?
(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.
Flags: needinfo?(nsm.nikhil)
(Assignee)

Comment 28

2 years ago
Created attachment 8611112 [details] [diff] [review]
principal.patch

Here we use appId+browserElement everywhere (except for the testing method).
Attachment #8610657 - Attachment is obsolete: true
Attachment #8611112 - Flags: review?(nsm.nikhil)
Attachment #8610657 - Flags: review?(ehsan)
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.
Attachment #8611112 - Flags: review-
Blocks: 1169249
(Assignee)

Updated

2 years ago
Blocks: 1155153
(Assignee)

Comment 30

2 years ago
Created attachment 8612275 [details] [diff] [review]
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.
Attachment #8611112 - Attachment is obsolete: true
Attachment #8611112 - Flags: review?(nsm.nikhil)
Attachment #8612275 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 31

2 years ago
Created attachment 8612282 [details] [diff] [review]
principal.patch

Make OriginAttributes::CreateSuffix const.
Attachment #8612275 - Attachment is obsolete: true
Attachment #8612275 - Flags: review?(nsm.nikhil)
Attachment #8612282 - Flags: review?(nsm.nikhil)
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.
Attachment #8612282 - Flags: review?(nsm.nikhil) → review-
(Assignee)

Comment 33

2 years ago
Created attachment 8612754 [details] [diff] [review]
interdiff
Attachment #8612754 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 34

2 years ago
Created attachment 8612755 [details] [diff] [review]
patch 1
Attachment #8612282 - Attachment is obsolete: true
Attachment #8612755 - Flags: review?(bobbyholley)
(Assignee)

Comment 35

2 years ago
Created attachment 8612790 [details] [diff] [review]
patch 2

ServiceWorkerRegistrar updated:

1. no system principal
2. use of principal for the unregister
Attachment #8612790 - Flags: review?(nsm.nikhil)
Attachment #8612790 - Flags: review?(nsm.nikhil) → review+
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.
Attachment #8612754 - Flags: review?(nsm.nikhil) → review+
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?
Attachment #8612755 - Flags: review?(bobbyholley)
(Assignee)

Comment 38

2 years ago
Created attachment 8613384 [details] [diff] [review]
interdiff
Attachment #8612754 - Attachment is obsolete: true
Attachment #8613384 - Flags: review?(bobbyholley)
(Assignee)

Comment 39

2 years ago
Created attachment 8613385 [details] [diff] [review]
patch 1
Attachment #8612755 - Attachment is obsolete: true
(Assignee)

Comment 40

2 years ago
> > +  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 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.
Attachment #8613384 - Flags: review?(bobbyholley) → feedback+
(Assignee)

Comment 42

2 years ago
Created attachment 8613547 [details] [diff] [review]
patch 1
Attachment #8613385 - Attachment is obsolete: true
Attachment #8613547 - Flags: review?(nsm.nikhil)
Blocks: 1170493
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?
Attachment #8613547 - Flags: review?(nsm.nikhil) → review+
(Assignee)

Comment 44

2 years ago
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
Flags: needinfo?(josh)
https://hg.mozilla.org/mozilla-central/rev/8bcfd31f7d5e
https://hg.mozilla.org/mozilla-central/rev/58aab1e6a65b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Blocks: 1171175
Depends on: 1171432
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!

Updated

2 years ago
Blocks: 1171915
Ryan, can you please back this out per bug 1171432, such that the backout is in tonight's Nightly?
Flags: needinfo?(ryanvm)
Lots of conflicts on the backout.
Flags: needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.