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

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bkelly, Assigned: baku)

Tracking

(Blocks 2 bugs)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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()
Priority: -- → P3
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.
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?
Flags: needinfo?(amarchesini)
I tried setting a nsIDocShellLoadInfo with the container triggering principal, but it had no effect.
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.
Flags: needinfo?(tanvi)

Comment 5

2 years ago
Jonathan, please take look at this bug.
Flags: needinfo?(jkt)
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).
Flags: needinfo?(jkt)

Comment 7

2 years ago
Thanks for the update. Christoph, assigning this bug to you to take a look at it.
Assignee: nobody → ckerschb
See Also: → 1328246
(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?
Assignee

Comment 9

2 years ago
I'll work on this tomorrow or next week.
Assignee: ckerschb → amarchesini
Flags: needinfo?(amarchesini)
Assignee

Comment 10

2 years ago
Posted patch OA.patch (obsolete) — Splinter Review
Attachment #8911149 - Flags: review?(bugs)
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 on attachment 8911149 [details] [diff] [review]
OA.patch

># HG changeset patch
># User Andrea Marchesini <amarchesini@mozilla.com>
># 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)
Attachment #8911149 - Flags: review?(bugs) → review-
Assignee

Comment 13

2 years ago
> >-  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.
Assignee

Comment 14

2 years ago
Posted patch OA_test.patchSplinter Review
Attachment #8911724 - Flags: review?(bugs)
Assignee

Comment 15

2 years ago
Posted patch OA.patch (obsolete) — Splinter Review
Basically the same patch, with isPrivateBrowsing boolean set.
Attachment #8911149 - Attachment is obsolete: true
Attachment #8911725 - Flags: review?(bugs)
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?
Attachment #8911725 - Flags: review?(bugs)
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
Attachment #8911724 - Flags: review?(bugs) → review+
(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?
Flags: needinfo?(amarchesini)
Assignee

Comment 19

2 years ago
> 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().
Flags: needinfo?(amarchesini)
Assignee

Comment 20

2 years ago
Attachment #8911725 - Attachment is obsolete: true
Attachment #8912324 - Flags: review?(bzbarsky)
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.
Flags: needinfo?(tanvi)
(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 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?
Attachment #8912324 - Flags: review?(bzbarsky) → review+

Comment 24

2 years ago
Pushed by amarchesini@mozilla.com:
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

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa2f3c4c3242
ServiceWorkerClients::openWindow() - OriginAttributes test using tabs, r=me
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.
Flags: needinfo?(amarchesini)
(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.
Assignee

Updated

2 years ago
Flags: needinfo?(amarchesini)

Comment 28

2 years ago
Pushed by amarchesini@mozilla.com:
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
Wait, did you just disable the test on non-e10s?  Don't we need to fix that?
Flags: needinfo?(amarchesini)
Assignee

Comment 30

2 years ago
(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.
Blocks: 1404328
Flags: needinfo?(amarchesini)

Comment 31

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7c4062d2329b
https://hg.mozilla.org/mozilla-central/rev/de11d13c7181
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.