1388413 - clicking twitter push notification does not open twitter window in correct container

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement, P3)

Description

[Ben Kelly [:bkelly, not reviewing]]

I currently run twitter.com in a container.  I have push notifications enabled for twitter.  When I get a notification and click on it, though, the resulting tab is opened in the default context with no container.

We must be losing origin attributes at one of these places:

1. Firing the push event
2. Firing the notification click event
3. Clients.openWindow()

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

This happens in the facebook push notifications as well.

It seems likely that we are losing the origin attributes during openWindow() since the service worker is associated with the container-restricted principal.  If we lost it earlier we would try opening a window at all.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I looked at this a bit and I think its a container bug on non-e10s.  We are are passing a triggering principal with the correct context ID from the service worker code here:

http://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerClients.cpp#761

But the triggering principal origin attributes are ignored.  Only the opener is used to determine the container.  If I make this change we get the correct container in non-e10s:

diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -5324,17 +5324,20 @@ nsBrowserAccess.prototype = {
         // to the nsIDOMWindow of the opened tab right away. For e10s windows,
         // this means forcing the newly opened browser to be non-remote so that
         // we can hand back the nsIDOMWindow. The XULBrowserWindow.shouldLoadURI
         // will do the job of shuttling off the newly opened browser to run in
         // the right process once it starts loading a URI.
         let forceNotRemote = !!aOpener;
         let userContextId = aOpener && aOpener.document
                               ? aOpener.document.nodePrincipal.originAttributes.userContextId
-                              : Ci.nsIScriptSecurityManager.DEFAULT_USER_CONTEXT_ID;
+                              : aTriggeringPrincipal
+                                ? aTriggeringPrincipal.originAttributes.userContextId
+                                : Ci.nsIScriptSecurityManager.DEFAULT_USER_CONTEXT_ID;
+        dump('### ### opening tab with user context id: ' + userContextId + '\n');
         let openerWindow = (aFlags & Ci.nsIBrowserDOMWindow.OPEN_NO_OPENER) ? null : aOpener;
         let browser = this._openURIInNewTab(aURI, referrer, referrerPolicy,
                                             isPrivate, isExternal,
                                             forceNotRemote, userContextId,
                                             openerWindow, null, aTriggeringPrincipal);
         if (browser)
           newWindow = browser.contentWindow;
         break;

For e10s, however, its not clear to me how we request a container via nsWindowWatcher.  Andrea, do you know how this is supposed to work?  Do we need to create an nsDocShellLoadInfo with the pringipal data in it?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I tried setting a nsIDocShellLoadInfo with the container triggering principal, but it had no effect.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Tanvi, do you have any ideas here?  Trying to figure out how c++ can open a new tab in the correct container without a window.opener.  We have a triggering principal with the container origin attributes set, but it seems the APIs we are using only pull the container value from the opener.

Comment 5

[Wennie]

Jonathan, please take look at this bug.

Comment 6

[Jonathan Kingston [:jkt] he/him]

I already spoke with Tanvi and :bkelly about this. Either Tanvi, Baku or Christoph would be better candidates to fixing this as it involved triggering principles etc (I likely won't get onto this any faster than the others with the other work you sent me, feel free to reorganise priorities etc).

Comment 7

[Wennie]

Thanks for the update. Christoph, assigning this bug to you to take a look at it.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Wennie from comment #7)
> Thanks for the update. Christoph, assigning this bug to you to take a look
> at it.

Baku, any chance I could assign this bug to you since you have been working on OA?

Comment 9

[Andrea Marchesini [:baku]]

I'll work on this tomorrow or next week.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: OA.patch

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8911149 [details] [diff] [review]
OA.patch

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

This just changes the e10s path, right?  Do we need something to fix non-e10s as well?

Comment 12

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

Comment on attachment 8911149 [details] [diff] [review]
OA.patch

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># Parent  18784fceb535a55fb1981e17a65ce6cb36444308
>
>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
>--- a/docshell/base/nsDocShell.cpp
>+++ b/docshell/base/nsDocShell.cpp
>@@ -8140,17 +8140,19 @@ nsDocShell::CreateAboutBlankContentViewe
>   }
> 
>   // mContentViewer->PermitUnload may release |this| docshell.
>   nsCOMPtr<nsIDocShell> kungFuDeathGrip(this);
> 
>   AutoRestore<bool> creatingDocument(mCreatingDocument);
>   mCreatingDocument = true;
> 
>-  if (aPrincipal && !nsContentUtils::IsSystemPrincipal(aPrincipal) &&
>+  if (aPrincipal &&
>+      BasePrincipal::Cast(aPrincipal)->Kind() != BasePrincipal::eNullPrincipal &&
>+      !nsContentUtils::IsSystemPrincipal(aPrincipal) &&
>       mItemType != typeChrome) {
>     MOZ_ASSERT(aPrincipal->OriginAttributesRef() == mOriginAttributes);
>   }
> 
I don't understand these changes. Why is this needed? These kind of changes are worrisome and make the setup around OA more fragile -
the less we have special cases the better.

>+CheckUserContextCompatibility(nsIDocShell* aDocShell,
>+                              const OriginAttributes* aAttrs)
> {
>   MOZ_ASSERT(aDocShell);
> 
>   uint32_t userContextId =
>     static_cast<nsDocShell*>(aDocShell)->GetOriginAttributes().mUserContextId;
> 
>+  if (aAttrs) {
>+    return aAttrs->mUserContextId == userContextId;
>+  }
This needs a comment that why we check only user context id and not all the attributes.



>@@ -665,16 +672,17 @@ nsWindowWatcher::OpenWindowInternal(mozI
>                                     const char* aFeatures,
>                                     bool aCalledFromJS,
>                                     bool aDialog,
>                                     bool aNavigate,
>                                     nsIArray* aArgv,
>                                     bool aIsPopupSpam,
>                                     bool aForceNoOpener,
>                                     nsIDocShellLoadInfo* aLoadInfo,
>+                                    const mozilla::OriginAttributes* aAttrs,
Could you call aAttrs aOriginAttributes


> 
>+    if (aAttrs) {
>+      docShell->SetOriginAttributes(*aAttrs);
>     // If this is not a chrome docShell, we apply originAttributes from the
>     // subjectPrincipal unless if it's an expanded or system principal.
>-    if (subjectPrincipal &&
>-        !nsContentUtils::IsSystemOrExpandedPrincipal(subjectPrincipal) &&
>-        docShell->ItemType() != nsIDocShellTreeItem::typeChrome) {
>+    } else if (subjectPrincipal &&
>+               !nsContentUtils::IsSystemOrExpandedPrincipal(subjectPrincipal) &&
>+               docShell->ItemType() != nsIDocShellTreeItem::typeChrome) {
>       isPrivateBrowsingWindow =
>         !!subjectPrincipal->OriginAttributesRef().mPrivateBrowsingId;
Shouldn't you set isPrivateBrowsingWindow also when aAttrs is passed.


r- mainly because of missing tests. Otherwise we will just regress the behavior later. (window opening is unfortunately very fragile code)

Comment 13

[Andrea Marchesini [:baku]]

> >-  if (aPrincipal && !nsContentUtils::IsSystemPrincipal(aPrincipal) &&
> >+  if (aPrincipal &&
> >+      BasePrincipal::Cast(aPrincipal)->Kind() != BasePrincipal::eNullPrincipal &&
> >+      !nsContentUtils::IsSystemPrincipal(aPrincipal) &&
> >       mItemType != typeChrome) {
> >     MOZ_ASSERT(aPrincipal->OriginAttributesRef() == mOriginAttributes);
> >   }
> > 
> I don't understand these changes. Why is this needed? These kind of changes
> are worrisome and make the setup around OA more fragile -
> the less we have special cases the better.

When SWClient->OpenWindw() is executed, we end up using a null principal. This change avoid the comparison of the OriginAttributes of a NullPrincipal with the current one.

> 
> >+CheckUserContextCompatibility(nsIDocShell* aDocShell,
> >+                              const OriginAttributes* aAttrs)
> >+  if (aAttrs) {
> >+    return aAttrs->mUserContextId == userContextId;
> >+  }
> This needs a comment that why we check only user context id and not all the
> attributes.

This method is just about checking userContextId.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: OA_test.patch

Comment 15

[Andrea Marchesini [:baku]]

Attachment: OA.patch

Basically the same patch, with isPrivateBrowsing boolean set.

Comment 16

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

Comment on attachment 8911725 [details] [diff] [review]
OA.patch

As I said on IRC, I'm not really happy adding new special cases to docshell.
The more we add such, the harder maintaining OA handling becomes.

Could we do something like... pass null principal with right OA around and not the fallback singleton null principal?

Comment 17

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

Comment on attachment 8911724 [details] [diff] [review]
OA_test.patch

Could you check somewhere, perhaps before "// Registration of the SW" that the content actually has the right user context ID?
content.document.nodePrincipal.userContextID == USER_CONTEXT_ID kind of check.

Also check that before return content.navigator.serviceWorker.getRegistration.

We want to ensure the web pages actually use the right principal, not just that the tab has usercontextid

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini [:baku] from comment #13)
> > >-  if (aPrincipal && !nsContentUtils::IsSystemPrincipal(aPrincipal) &&
> > >+  if (aPrincipal &&
> > >+      BasePrincipal::Cast(aPrincipal)->Kind() != BasePrincipal::eNullPrincipal &&
> > >+      !nsContentUtils::IsSystemPrincipal(aPrincipal) &&
> > >       mItemType != typeChrome) {
> > >     MOZ_ASSERT(aPrincipal->OriginAttributesRef() == mOriginAttributes);
> > >   }
> > > 
> > I don't understand these changes. Why is this needed? These kind of changes
> > are worrisome and make the setup around OA more fragile -
> > the less we have special cases the better.
> 
> When SWClient->OpenWindw() is executed, we end up using a null principal.
> This change avoid the comparison of the OriginAttributes of a NullPrincipal
> with the current one.

Why do we have a null principal here in this case?  We have a real triggering principal we can pass, no?

Comment 19

[Andrea Marchesini [:baku]]

> Why do we have a null principal here in this case?  We have a real
> triggering principal we can pass, no?

We have a null principal because when we initialize AutoJSAPI, aParent is null:

http://searchfox.org/mozilla-central/source/toolkit/components/windowwatcher/nsWindowWatcher.cpp#806-808

This makes so that here we have a NullPrincipal:

http://searchfox.org/mozilla-central/source/toolkit/components/windowwatcher/nsWindowWatcher.cpp#1067-1069

See this: http://searchfox.org/mozilla-central/source/dom/base/nsContentUtils.cpp#3207-3229

Discussing this with smaug, the idea is to change AutoJSAPI so that it's possible to pass an OriginAttributes when initialized.
And add an extra OriginAttributes param to nsIWindowWatcher::OpenWindow2().

Comment 20

[Andrea Marchesini [:baku]]

Attachment: Part 1 - Let's use a Sandbox with the triggering principal

Comment 21

[Tanvi Vyas[:tanvi]]

Containers was designed so that permissions are global and not container specific.  If I grant geolocation access to maps.google.com in the Personal Container, the Work Container will have access as well.  So will the default Container.

Push is a special permission because it involves a service worker.  And that service worker has Origin Attributes attached to it, correct?  So if I click a push notification, I should be able to use the Origin Attributes to open the right type of tab.

A few scenarios to consider:
I sign into twitter.com in the Work Container.  I enable push notifications.  I sign into twitter.com in my Personal Container.  Are notifications automatically enabled, or does twitter ask me?  Can I get push notifications from two different accounts?  Will this also work in the default Container - with maybe a third twitter account.

With baku's patch, would clicking on a push notification from any one of these 3 accounts take me to the right container and right account?

Again, we didn't consider push when we created Containers, so the behavior probably doesn't make sense.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tanvi Vyas[:tanvi] from comment #21)
> A few scenarios to consider:
> I sign into twitter.com in the Work Container.  I enable push notifications.
> I sign into twitter.com in my Personal Container.  Are notifications
> automatically enabled, or does twitter ask me?  Can I get push notifications
> from two different accounts?  Will this also work in the default Container -
> with maybe a third twitter account.

I'm not sure about if you have push enabled for the same origin across many containers.  I did verify that for a single push subscription, though, that the `push` and `notificationclick` events are fired in a service worker with the correct origin attributes on its container.

I think we just need to make sure origin attributes propagate through `clients.openWindow()` just like they do for `window.open()`.  This `clients.openWindow()` API is how service workers open a tab in response to a notification click.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8912324 [details] [diff] [review]
Part 1 - Let's use a Sandbox with the triggering principal

>+      RefPtr<JSObjectHolder> holder = new JSObjectHolder(cx, sandbox);

Why?  There's no cross-thread anything here.  "sandbox" is already rooted, so it's not like it will go away on you....

>+      // The CreateSandbox call returns a proxy to the actual sandbox object. 

Yes, if you call it while in a compartment (will wrap into that compartment).  But you're pretty explicitly calling it while NOT in a compartment.  So I would expect that it does nothing of the sort here.

I assume you copied this from ConsoleRunnable::RunWindowless.  But that one uses AutoJSContext, so has no idea what compartment it's in (a bug; should be fixed).

>+      global = js::UncheckedUnwrap(global);  

So this should go away.

r=me with those bits fixed, but can you please file a bug to consider removing this once bug 1316480 is fixed?

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d6924a59f1
ServiceWorkerClients::OpenWindow() should use the triggeringPrincipal, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f8b7e60f397
ServiceWorkerClients::openWindow() - OriginAttributes test, r=smaug

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa2f3c4c3242
ServiceWorkerClients::openWindow() - OriginAttributes test using tabs, r=me

Comment 26

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

I had to back out all three patches because a test was still failing: https://treeherder.mozilla.org/logviewer.html#?job_id=133919108&repo=mozilla-inbound

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

Seems limited to win7debug, if that helps.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Wes Kocher (:KWierso) from comment #26)
> I had to back out all three patches because a test was still failing:
> https://treeherder.mozilla.org/logviewer.html#?job_id=133919108&repo=mozilla-
> inbound
> 
> https://hg.mozilla.org/mozilla-central/rev/
> e6c32278f32cd5f7d159627b2157396b62d0c4a9
> 
> Seems limited to win7debug, if that helps.

This is a non-e10s test.  We only run non-e10s browser chrome mochitests on win7 debug now, I believe.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c4062d2329b
ServiceWorkerClients::OpenWindow() should use the triggeringPrincipal, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/de11d13c7181
ServiceWorkerClients::openWindow() - OriginAttributes test, r=smaug

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Wait, did you just disable the test on non-e10s?  Don't we need to fix that?

Comment 30

[Andrea Marchesini [:baku]]

(In reply to Ben Kelly [:bkelly] from comment #29)
> Wait, did you just disable the test on non-e10s?  Don't we need to fix that?

I'm working on that. I'll re-enable the tests in a separate bug.

Comment 31

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

https://hg.mozilla.org/mozilla-central/rev/7c4062d2329b
https://hg.mozilla.org/mozilla-central/rev/de11d13c7181