1172870 - Implement Service Worker Clients.OpenWindow() for Desktop

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Noemí Freire (:noemi)]

As raised in https://bugzilla.mozilla.org/show_bug.cgi?id=1130687#c36, we will need to deal with the different scenarios independently, so opening this bug to track Desktop one.

Comment 1

[Albert [:albert]]

Attachment: Patch WIP

Adds the component in charge of open window

Comment 2

[Albert [:albert]]

Attachment: Patch WIP

Added open window logic

Comment 3

[Albert [:albert]]

I've been trying to implement the window open feature but the current methods available in gecko do not allow to do it without an opener. As Bill described in meta bug https://bugzilla.mozilla.org/show_bug.cgi?id=1130687#c51 we need to add a new way to open a window. 

Bill, would be there anyone could face that problem? I'm not familiar with tabbed browser code and it seems to be a bit complicated. However, if someone can mentor me here I could start working on it with his support.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Nikhil probably has a better idea of who could work on this.

Comment 5

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

Hmm... This is quite hairy code as far as I understand. I'm ni?ing :markh who knows some of the JS side code related to this and may be able to put you in touch with the right people.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

(In reply to Nikhil Marathe [:nsm] (back on July 13) from comment #5)
> Hmm... This is quite hairy code as far as I understand. I'm ni?ing :markh
> who knows some of the JS side code related to this and may be able to put
> you in touch with the right people.

I'm out of touch with this code, but I do see the ever helpful Mike Conley has touched that tab opening code recently, so he might have more insights...

Comment 7

[Mike Conley (:mconley) (:⚙️)]

I'm not sure what the question is here. The stuff that billm is talking about in https://bugzilla.mozilla.org/show_bug.cgi?id=1130687#c51 is very much in Gecko, down in ContentParent and TabParent/TabChild. While I've touched some of that code, I'm not sure sure I'd be the best guide through it.

(In reply to Albert [:albert] from comment #3)
> Bill, would be there anyone could face that problem? I'm not familiar with
> tabbed browser code and it seems to be a bit complicated. However, if
> someone can mentor me here I could start working on it with his support.

Which tabbed browser code are you referring to? The stuff that billm refers to in https://bugzilla.mozilla.org/show_bug.cgi?id=1130687#c51?

Comment 8

[Albert [:albert]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #7)
> I'm not sure what the question is here. The stuff that billm is talking
> about in https://bugzilla.mozilla.org/show_bug.cgi?id=1130687#c51 is very
> much in Gecko, down in ContentParent and TabParent/TabChild. While I've
> touched some of that code, I'm not sure sure I'd be the best guide through
> it.
> 
> (In reply to Albert [:albert] from comment #3)
> > Bill, would be there anyone could face that problem? I'm not familiar with
> > tabbed browser code and it seems to be a bit complicated. However, if
> > someone can mentor me here I could start working on it with his support.
> 
> Which tabbed browser code are you referring to? The stuff that billm refers
> to in https://bugzilla.mozilla.org/show_bug.cgi?id=1130687#c51?

Yes, I'm talking about https://bugzilla.mozilla.org/show_bug.cgi?id=1130687#c51 guidelines

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We really need someone from platform to work on this. I'll ask Andrew Overholt about it.

Comment 10

[Andrew Overholt [:overholt]]

Someone from the DOM team can look into this but we need to get the rest of Service Workers into shape first.

Albert, when do you need this?  Is the (tomorrow!) target milestone accurate?

Comment 11

[Noemí Freire (:noemi)]

Hi,

Checking with SW Toolkit folks the needed milestone here (so removing it for now). We will update it once it has been clarified. Thanks!

Comment 12

[Albert [:albert]]

Releasing it, please ni me if you need some feedback.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Just so I understand, you mentioned that you had fixes for opening windows for all cases except the case where there are no remote tabs with e10s enabled. Are those fixes rolled up in the patch attached to this bug?

Comment 14

[Andrew Overholt [:overholt]]

Catalin knows the details.

Comment 15

[Cătălin Badea (:catalinb)]

Attachment: Move CreateWindow to PContent

Comment 16

[Cătălin Badea (:catalinb)]

Attachment: dispatch openWindow to the parent.

Comment 17

[Cătălin Badea (:catalinb)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #13)
> Just so I understand, you mentioned that you had fixes for opening windows
> for all cases except the case where there are no remote tabs with e10s
> enabled. Are those fixes rolled up in the patch attached to this bug?

I've attached a very rough version of the patches. Before these can be reviewed I need to figure out how
to rewrite the TabContext checks to make more sense when there's no TabContext.
Specifically, the checks performed here:
https://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabContext.h?case=true&from=MaybeInvalidTabContext&offset=0#231
https://dxr.mozilla.org/mozilla-central/source/dom/ipc/nsIContentParent.cpp#71
https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentProcessManager.cpp#139

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Hey catalinb - I'd like to talk about this patch, and the requirements for openWindow from Service Workers. I suspect we can find the right path by merging what we both know.

Can we find some time to chat this week?

Comment 19

[Cătălin Badea (:catalinb)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #18)
> Hey catalinb - I'd like to talk about this patch, and the requirements for
> openWindow from Service Workers. I suspect we can find the right path by
> merging what we both know.
> 
> Can we find some time to chat this week?

Sure. I'll be on irc tomorrow.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

So catalinb's patches involve moving all of our window-opening gunk from TabChild/TabParent into ContentChild/ContentParent. I'm a bit wary, as I'm not entirely certain that that change in assumptions (that the TabChild opener is guaranteed to have a TabParent, for example) will result in this code being somewhat erroneous.

I'm just a bit worried, and hoping for a second opinion. As I said in IRC, I feel like I've only got about 60% of the window opening code understood at this point.

Comment 21

[Cătălin Badea (:catalinb)]

Attachment: Implement service workers clients.openWindow for desktop (non-e10s). r?baku

Based on patches from:
  Nikhil Marathe <[e-mail]>
  Alberto Crespell Perez <[e-mail]>

Comment 22

[Cătălin Badea (:catalinb)]

Comment on attachment 8668894 [details] [diff] [review]
Implement service workers clients.openWindow for desktop (non-e10s).  r?baku

baku, could you please review this patch? It is mainly based on alberto's patch from bug 1130687. We need to have openWindow landed for single process desktop as soon as possible.

Comment 23

[Andrea Marchesini [:baku]]

Comment on attachment 8668894 [details] [diff] [review]
Implement service workers clients.openWindow for desktop (non-e10s).  r?baku

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

Smaug, can you take a look at the window management calls?

::: dom/workers/ServiceWorkerClients.cpp
@@ +8,5 @@
>  #include "mozilla/dom/PromiseWorkerProxy.h"
>  
> +#include "nsIWindowWatcher.h"
> +#include "nsPIWindowWatcher.h"
> +#include "nsWindowWatcher.h"

alphabetic order.

@@ +355,5 @@
> +      nsRefPtr<LoadListener> listener = new LoadListener(mPromiseProxy,
> +                                                         window,
> +                                                         baseURI);
> +      nsCOMPtr<EventTarget> target = window->GetFrameElementInternal();
> +      target->AddEventListener(NS_LITERAL_STRING("load"), listener, true);

this can fail.

@@ +406,5 @@
> +                                 getter_AddRefs(browserWindow));
> +    if (NS_WARN_IF(NS_FAILED(rv)) || !browserWindow) {
> +      // It is possible to be running without a browser window on Mac OS, so
> +      // we need to open a new chrome window.
> +      // TODO(catalinb): open new chrome window

follow up... + ID.

@@ +494,3 @@
>  {
> +  // XXXcatalinb: This works only on non-multiprocess for now, bail if we're
> +  // running in a content process.

do we have a follow up for this?

@@ +513,5 @@
> +  }
> +
> +  // [[4. If this algorithm is not allowed to show a popup ..]]
> +  // XXXcatalinb: The service worker is allowed to show a popup if
> +  // the user just clicked on a notification.

follow up for this too, right? maybe write the bug IDs.

Comment 24

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

Comment on attachment 8668894 [details] [diff] [review]
Implement service workers clients.openWindow for desktop (non-e10s).  r?baku


>+      WorkerPrivate::LocationInfo& info =
>+        mPromiseProxy->GetWorkerPrivate()->GetLocationInfo();
>+      nsCOMPtr<nsIURI> baseURI;
>+      nsresult rv = NS_NewURI(getter_AddRefs(baseURI), info.mOrigin);
>+      if (NS_WARN_IF(NS_FAILED(rv))) {
>+        return NS_ERROR_FAILURE;
>+      }
>+
>+      nsRefPtr<LoadListener> listener = new LoadListener(mPromiseProxy,
>+                                                         window,
>+                                                         baseURI);
>+      nsCOMPtr<EventTarget> target = window->GetFrameElementInternal();
>+      target->AddEventListener(NS_LITERAL_STRING("load"), listener, true);
This looks odd. We wait for some load event to propagate to xul:browser?
And why are we waiting for load event? I don't see that in the spec, but perhaps I'm missing something.
My interpretation is that we should resolve the promise with a client asap we have a Window object.

>+  OpenWindow(nsPIDOMWindow** aWindow)
...
>+    // [[6.1 Open Window]]
>+    nsCOMPtr<nsIWindowMediator> wm = do_GetService(NS_WINDOWMEDIATOR_CONTRACTID,
>+                                                   &rv);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      return rv;
>+    }
>+
>+    // Find the most recent browser window and open a new tab in it.
>+    nsCOMPtr<nsIDOMWindow> browserWindow;
>+    rv = wm->GetMostRecentWindow(MOZ_UTF16("navigator:browser"),
>+                                 getter_AddRefs(browserWindow));
>+    if (NS_WARN_IF(NS_FAILED(rv)) || !browserWindow) {
>+      // It is possible to be running without a browser window on Mac OS, so
>+      // we need to open a new chrome window.
>+      // TODO(catalinb): open new chrome window
>+      return NS_ERROR_FAILURE;
>+    }
So if we don't have a most recent navigator:browser, we want to just use 
some window and do openDialog on it or so, similar to browser.js.
(That is what osx ends up effectively doing http://mxr.mozilla.org/mozilla-central/source/browser/base/content/macBrowserOverlay.xul#31)
Would be good to ask browser devs.


r- especially because of that 'load' handling.
(some large image in the page might take quite some time and block load event)

Comment 25

[Cătălin Badea (:catalinb)]

(In reply to Andrea Marchesini (:baku) from comment #23)
> @@ +513,5 @@
> > +  }
> > +
> > +  // [[4. If this algorithm is not allowed to show a popup ..]]
> > +  // XXXcatalinb: The service worker is allowed to show a popup if
> > +  // the user just clicked on a notification.
> 
> follow up for this too, right? maybe write the bug IDs.
This is already implemented, there's a check for workerPrivate->GlobalScope()->WindowInteractionAllowed() which passes only if the user just clicked on a notification. It is also covered by the test.

Comment 26

[Cătălin Badea (:catalinb)]

(In reply to Olli Pettay [:smaug] from comment #24)
> Comment on attachment 8668894 [details] [diff] [review]
> Implement service workers clients.openWindow for desktop (non-e10s).  r?baku
> 
> 
> >+      WorkerPrivate::LocationInfo& info =
> >+        mPromiseProxy->GetWorkerPrivate()->GetLocationInfo();
> >+      nsCOMPtr<nsIURI> baseURI;
> >+      nsresult rv = NS_NewURI(getter_AddRefs(baseURI), info.mOrigin);
> >+      if (NS_WARN_IF(NS_FAILED(rv))) {
> >+        return NS_ERROR_FAILURE;
> >+      }
> >+
> >+      nsRefPtr<LoadListener> listener = new LoadListener(mPromiseProxy,
> >+                                                         window,
> >+                                                         baseURI);
> >+      nsCOMPtr<EventTarget> target = window->GetFrameElementInternal();
> >+      target->AddEventListener(NS_LITERAL_STRING("load"), listener, true);
> This looks odd. We wait for some load event to propagate to xul:browser?
> And why are we waiting for load event? I don't see that in the spec, but
> perhaps I'm missing something.
> My interpretation is that we should resolve the promise with a client asap
> we have a Window object.
Maybe waiting for the load event is not the best option, but we definitely can't resolve at this point.
First, we need to wait for all http redirects to finish and check if the last url is in the same origin as
the service worker.
Next, the properties of the WindowClient don't make sense at this point:
we need the browsing context's active document for |focused| and |visibility|.

> 
> >+  OpenWindow(nsPIDOMWindow** aWindow)
> ...
> >+    // [[6.1 Open Window]]
> >+    nsCOMPtr<nsIWindowMediator> wm = do_GetService(NS_WINDOWMEDIATOR_CONTRACTID,
> >+                                                   &rv);
> >+    if (NS_WARN_IF(NS_FAILED(rv))) {
> >+      return rv;
> >+    }
> >+
> >+    // Find the most recent browser window and open a new tab in it.
> >+    nsCOMPtr<nsIDOMWindow> browserWindow;
> >+    rv = wm->GetMostRecentWindow(MOZ_UTF16("navigator:browser"),
> >+                                 getter_AddRefs(browserWindow));
> >+    if (NS_WARN_IF(NS_FAILED(rv)) || !browserWindow) {
> >+      // It is possible to be running without a browser window on Mac OS, so
> >+      // we need to open a new chrome window.
> >+      // TODO(catalinb): open new chrome window
> >+      return NS_ERROR_FAILURE;
> >+    }
> So if we don't have a most recent navigator:browser, we want to just use 
> some window and do openDialog on it or so, similar to browser.js.
What do you mean by some window? We're in the case where we don't have any windows.

> (That is what osx ends up effectively doing
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> macBrowserOverlay.xul#31)
> Would be good to ask browser devs.
Could you point me to someone specific?

The solution I have in mind for this is as follows:
1. open a chrome window with windowwatcher->openWindow("chrome://content/browser");
2. wait until it loads and chromeWindow->GetBrowserDOMWindow becomes available, which happens here
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#996
3. use nsIBrowserDOMWindow to open a new tab.
The only problem I have with this approach is that, unless I pass an initial url to load (like a default about:blank tab) when opening the chrome window, the window's decorations are all messed up. So by passing
a dummy url I'll get the correct decorations but will end up with two tabs instead of just 1.

I'm considering two options here: Either pass the url to the |arguments| array in browser.js and find a way
to retrieve the DOM window OR open the chrome window with about:blank and somehow reuse that tab to load
the desired url.


> r- especially because of that 'load' handling.
> (some large image in the page might take quite some time and block load
> event)

Comment 27

[Cătălin Badea (:catalinb)]

Please see the previous comment.

Comment 28

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

(In reply to Cătălin Badea (:catalinb) from comment #26)
> Maybe waiting for the load event is not the best option, but we definitely
> can't resolve at this point.
> First, we need to wait for all http redirects to finish and check if the
> last url is in the same origin as
> the service worker.
> Next, the properties of the WindowClient don't make sense at this point:
> we need the browsing context's active document for |focused| and
> |visibility|.

I didn't say resolve immediately, but asap when we have the relevant Window ready
(if we resolved immediately, we might get an about:blank Document).



> > So if we don't have a most recent navigator:browser, we want to just use 
> > some window and do openDialog on it or so, similar to browser.js.
> What do you mean by some window? We're in the case where we don't have any
> windows.
There is always a window, possibly just the hidden window.


> 
> > (That is what osx ends up effectively doing
> > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> > macBrowserOverlay.xul#31)
> > Would be good to ask browser devs.
> Could you point me to someone specific?
> 
> The solution I have in mind for this is as follows:
> 1. open a chrome window with
> windowwatcher->openWindow("chrome://content/browser");
Or use the the someWindow->openDialog approach which browser.js has used
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1942
...



> I'm considering two options here: Either pass the url to the |arguments|
> array in browser.js 
...which is effectively this.

Comment 29

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

Btw, there is https://github.com/slightlyoff/ServiceWorker/issues/728
and I'd say the spec is broken here. Way too racy.

Comment 30

[Cătălin Badea (:catalinb)]

Attachment: Implement service workers clients.openWindow for desktop (non-e10s).

Based on patches from:
  Nikhil Marathe <[e-mail]>
  Alberto Crespell Perez <[e-mail]>

Comment 31

[Cătălin Badea (:catalinb)]

Attachment: Implement service workers clients.openWindow for desktop (non-e10s).

Based on patches from:
  Nikhil Marathe <[e-mail]>
  Alberto Crespell Perez <[e-mail]>

Comment 32

[Cătălin Badea (:catalinb)]

Attachment: Implement service worker clients.openWindow for desktop (e10s).

This is still a very rough patch, but it's decent enough for someone else to
look at it. The patch moves CreateWindow from PBrowser to PContent and makes
ContentChild implement nsIWindowProvider. For ServiceWorkerClients::OpenWindow,
it deals with the lack of a TabChild opener by using a special tab context that's
accepted by the parent process only on desktop.

I think this approach is the best we can do unless a service worker can provide
a tab child.

smaug, could you please have a look at this and comment on the overall approach?
Thanks!

Comment 33

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

Comment on attachment 8676342 [details] [diff] [review]
Implement service workers clients.openWindow for desktop (non-e10s).

>+    } else if (mClientInfo) {
>+      nsRefPtr<ServiceWorkerWindowClient> client =
>+        new ServiceWorkerWindowClient(promise->GetParentObject(),
>+                                      *mClientInfo);
>+        promise->MaybeResolve(client);
Nit, there is 2 extra spaces before 'promise'

>+class WebProgressListener final : public nsIWebProgressListener,
>+                                  public nsSupportsWeakReference
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  WebProgressListener(PromiseWorkerProxy* aPromiseProxy,
>+                      nsPIDOMWindow* aWindow,
>+                      nsIURI* aBaseURI)
>+  : mPromiseProxy(aPromiseProxy)
>+  , mWindow(aWindow)
>+  , mBaseURI(aBaseURI)
>+  {
>+    MOZ_ASSERT(aPromiseProxy);
>+    MOZ_ASSERT(aWindow);
>+    MOZ_ASSERT(aBaseURI);
>+    AssertIsOnMainThread();
>+
>+    mSelfRef = this;
Per IRC, ServiceWorkerPrivate will keep this object alive as long as it is needed.

>+  OnStateChange(nsIWebProgress* aWebProgress,
>+                nsIRequest* aRequest,
>+                uint32_t aStateFlags, nsresult aStatus) override
>+  {
>+    if (!(aStateFlags & STATE_IS_DOCUMENT)) {
>+      return NS_OK;
>+    }
So I think we should return early here also when aStateFlags doesn't have STATE_TRANSFERRING or STATE_STOP. 
Do we have tests for redirects?


>+    nsCOMPtr<nsIDocument> doc = mWindow->GetExtantDoc();
GetExtantDoc() may return null, so, null check.


>+  // [[4. If this algorithm is not allowed to show a popup ..]]
>+  // XXXcatalinb: The service worker is allowed to show a popup if
>+  // the user just clicked on a notification.
Would be better to remove XXXcatalinb: and just explain something like
"In Gecko the service worker is allowed..."
The spec does allow UAs to have different behavior.


Looks good but could take another look especially after mSelfRef removal.

Comment 34

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

Comment on attachment 8676815 [details] [diff] [review]
Implement service worker clients.openWindow for desktop (e10s).

Yeah, I think I like this approach. Would be nice to split the patch to two though. One which does just code move from TabChild/Parent to ContentChild/Parent and the other one could do the changes needed for sw.
Reviewing code move + changes in one patch tends to be hard.

Please don't use XXX when commenting the code, only when there is some issue to fix.

Comment 35

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

oh, one thing, don't use any name for the opened window.

Comment 36

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

Comment on attachment 8676815 [details] [diff] [review]
Implement service worker clients.openWindow for desktop (e10s).



>+      nsCOMPtr<nsIDOMWindow> newWindow;
>+      pwwatch->OpenWindow2(nullptr,
>+                           spec.get(),
>+                           "ServiceWorkerWindow!",
>+                           "dialog=no,all",
So, could name be null and also features, so that default are used.


>@@ -490,16 +491,17 @@ nsWindowWatcher::OpenWindowInternal(nsIDOMWindow* aParent,
I'd prefer is hasChromeParent was initialized to true only if we're in parent process.
And then in case there is no aParent, jsapiChromeGuard would need to be initialized with the
Init() method with no params.

Comment 37

[Cătălin Badea (:catalinb)]

Attachment: Implement service workers clients.openWindow for desktop (non-e10s).

This also includes tests for redirects.

Based on patches from:
  Nikhil Marathe <[e-mail]>
  Alberto Crespell Perez <[e-mail]>

Comment 38

[Cătălin Badea (:catalinb)]

(In reply to Olli Pettay [:smaug] from comment #33)
> >+  OnStateChange(nsIWebProgress* aWebProgress,
> >+                nsIRequest* aRequest,
> >+                uint32_t aStateFlags, nsresult aStatus) override
> >+  {
> >+    if (!(aStateFlags & STATE_IS_DOCUMENT)) {
> >+      return NS_OK;
> >+    }
> So I think we should return early here also when aStateFlags doesn't have
> STATE_TRANSFERRING or STATE_STOP. 
I discovered STATE_STOP is required to correctly handle redirects, but I don't understand what 
STATE_TRANSFERRING is needed for.

Comment 39

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

To deal with the case when we have some data even before the request has been fully loaded
"Wait for one or more bytes to be available or for the user agent to establish that the resource in question is empty."

Comment 40

[Cătălin Badea (:catalinb)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=240974b91d56

Comment 41

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

Comment on attachment 8677501 [details] [diff] [review]
Implement service workers clients.openWindow for desktop (non-e10s).

>+  OnStateChange(nsIWebProgress* aWebProgress,
>+                nsIRequest* aRequest,
>+                uint32_t aStateFlags, nsresult aStatus) override
>+  {
>+    if (!((aStateFlags & STATE_IS_DOCUMENT) && (aStateFlags & STATE_STOP))) {
So add STATE_TRANSFERRING check here too

>+      return NS_OK;
>+    }
>+
At this point we know we will handle the state change, so we should remove the listener from
aWebProgress and remove from ServiceWorkerManager::GetInstance() too.
DocLoader should keep a strong reference to 'this' during OnStateChange call, so the object should stay alive even after 
removing from ServiceWorkerManager.

>+
>+      nsCOMPtr<nsIDocShell> docShell = window->GetDocShell();
>+      if (!docShell) {
>+        return NS_ERROR_FAILURE;
>+      }
>+
>+      nsCOMPtr<nsIWebProgress> webProgress = do_GetInterface(docShell);
>+      if (webProgress) {
docShell _must_ implemnt webProgress so no need for this 'if'.
Or you could move the QI call to be before if (!dosShell) and then
do if (!webProgress) (QI is after all null safe.)




>+    MOZ_ASSERT(win);
I would just make this
NS_ENSURE_STATE(win);


>+    nsRefPtr<nsGlobalWindow> globalWindow = static_cast<nsGlobalWindow*>(win.get());
>+    nsRefPtr<nsGlobalWindow> outerWindow = static_cast<nsGlobalWindow*>(globalWindow->GetOuterWindow());
>+    nsCOMPtr<nsIDOMWindow> content = outerWindow->GetContent();
>+    nsCOMPtr<nsPIDOMWindow> contentWindow = do_QueryInterface(content);
>+    contentWindow.forget(aWindow);
Why you call GetContent() here?

Couldn't you just have 
nsCOMPtr<nsPIDOMWindow> pWin = do_QueryInterface(win);
pWin = pWin->GetOuterWindow();
pWin.forget(aWindow);



WebProgressListener ctor could assert that it is dealing with outer window

>+
>+  // Meant for keeping objects alive while handling requests from the worker
>+  // on the main thread. Released whenever the worker is terminated.
or when object is removed from the array.
(you should add RemoveISupports or some such)

Comment 42

[Cătălin Badea (:catalinb)]

Attachment: Implement service workers clients.openWindow for desktop (non-e10s).

Based on patches from:
  Nikhil Marathe <[e-mail]>
  Alberto Crespell Perez <[e-mail]>

Comment 43

[Cătălin Badea (:catalinb)]

Attachment: Implement service workers clients.openWindow for desktop (non-e10s).

Based on patches from:
  Nikhil Marathe <[e-mail]>
  Alberto Crespell Perez <[e-mail]>

Comment 44

[Cătălin Badea (:catalinb)]

Attachment: Implement service workers clients.openWindow for desktop (non-e10s).

Based on patches from:
  Nikhil Marathe <[e-mail]>
  Alberto Crespell Perez <[e-mail]>

Comment 45

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65dd5a0d18ae

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/65dd5a0d18ae

Comment 47

[Cătălin Badea (:catalinb)]

Attachment: Part 1 - Move PBrowser::CreateWindow to PContent.

Comment 48

[Cătălin Badea (:catalinb)]

Attachment: Part 1 - Move PBrowser::CreateWindow to PContent.

Comment 49

[Cătălin Badea (:catalinb)]

Attachment: Part 2 - Enable ServiceWorkerClients::OpenWindow on e10s desktop.

Comment 50

[Cătălin Badea (:catalinb)]

Attachment: Part 3 - Fix openWindow mochitest to work on e10s.

Comment 51

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

I'll review this stuff first thing tomorrow. (getting late here today)

Comment 52

[Cătălin Badea (:catalinb)]

I wonder if I could keep CreateWindow in PBrowser and in ContentChild::ProvideWindowCommon do:
newTabChild = SendCreatePBrowser();
newWindow = newTabChild->CreateWindow(maybeTabChildOpener);

Comment 53

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

yes, I think it would make sense, given that the patch has PBrowserParent* aThisTab now as the first param.

Comment 54

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

Hmm, or maybe not, now that I've read the other patch. The first param becomes sort of optional there.

Comment 55

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

Comment on attachment 8679767 [details] [diff] [review]
Part 2 - Enable ServiceWorkerClients::OpenWindow on e10s desktop.

>+    if (aTabOpener) {
>+        // We now need to provide a PBrowser as the opener to pass
>+        // the checks in AllocatePBrowser. The code assumes that the ipc layer
>+        // validates the PBrowser object we serialize - Is this really the case?
Not sure what you mean with validate here, and this isn't exactly about this bug anyway

>+        nsAutoPtr<MaybePBrowser> openerPBrowser;
>+        if (aTabOpener) {
>+            openerPBrowser = new MaybePBrowser(aTabOpener);
>+        } else {
>+            openerPBrowser = new MaybePBrowser(void_t());
>+        }
So this is weird. Can't you just make aThisTab nullable in webidl

>+ContentParent::RecvCreateWindow(const MaybePBrowser& aThisTab,
>                                 PBrowserParent* aNewTab,
>                                 const uint32_t& aChromeFlags,
>                                 const bool& aCalledFromJS,
>                                 const bool& aPositionSpecified,
>                                 const bool& aSizeSpecified,
>                                 const nsString& aURI,
>                                 const nsString& aName,
>                                 const nsString& aFeatures,
>@@ -5395,20 +5395,22 @@ ContentParent::RecvCreateWindow(PBrowserParent* aThisTab,
> 
>     // The content process should never be in charge of computing whether or
>     // not a window should be private or remote - the parent will do that.
>     MOZ_ASSERT(!(aChromeFlags & nsIWebBrowserChrome::CHROME_PRIVATE_WINDOW));
>     MOZ_ASSERT(!(aChromeFlags & nsIWebBrowserChrome::CHROME_NON_PRIVATE_WINDOW));
>     MOZ_ASSERT(!(aChromeFlags & nsIWebBrowserChrome::CHROME_PRIVATE_LIFETIME));
>     MOZ_ASSERT(!(aChromeFlags & nsIWebBrowserChrome::CHROME_REMOTE_WINDOW));
> 
>-    TabParent* thisTabParent = TabParent::GetFrom(aThisTab);
>-    MOZ_ASSERT(thisTabParent);
>+    TabParent* thisTabParent = nullptr;
>+    if (aThisTab.type() == MaybePBrowser::TPBrowserParent) {
>+        thisTabParent = TabParent::GetFrom(aThisTab.get_PBrowserParent());
>+    }
So this would become just a null check.

>+union MaybePBrowser
>+{
>+  PBrowser;
>+  void_t;
>+};
>+
So, you could drop this

>-    sync CreateWindow(PBrowser aThisTab,
>+    sync CreateWindow(MaybePBrowser aThisTab,
and here
nullable PBrowser aThisTab

>+// XXXcatalinb: This is only used by ServiceWorkerClients::OpenWindow.
>+// Because service workers don't have an associated TabChild
>+// we can't satisfy the security constraints on b2g. As such, the parent
>+// process will accept this tab context only on desktop.
>+// This is meant as a temporary solution until service workers can provide
>+// a TabChild equivalent.
>+struct UnsafeIPCTabContext
>+{ };
>+
So I wouldn't say "until service workers can provide a TabChild equivalent."
since it isn't clear to me that is needed. 
ServiceWorker may need some magical 'tabcontext' from parent when it is created and it would send it
back when new window is created so that one can validate the same flags are on both sides. But no need for
TabChild or anything heavy like that, as far as I see.

In fact, in a followup we could perhaps change aThisTab to union like thing:
aThisTabOrTabContext or so.

> nsIContentParent::CanOpenBrowser(const IPCTabContext& aContext)
> {
>-  const IPCTabContextUnion& contextUnion = aContext.contextUnion();
>-
>-  // We don't trust the IPCTabContext we receive from the child, so we'll bail
>-  // if we receive an IPCTabContext that's not a PopupIPCTabContext.
>   // (PopupIPCTabContext lets the child process prove that it has access to
>   // the app it's trying to open.)
>-  if (contextUnion.type() != IPCTabContextUnion::TPopupIPCTabContext) {
>-    ASSERT_UNLESS_FUZZING("Unexpected IPCTabContext type.  Aborting AllocPBrowserParent.");
>-    return false;
>-  }
So this deals with all the types, except TPopupIPCTabContext

>+  // On e10s we also allow UnsafeTabContext to allow service workers to open
>+  // windows. This is enforce in MaybeInvalidTabContext.
>+  if (aContext.type() == IPCTabContext::TPopupIPCTabContext) {
>+    const PopupIPCTabContext& popupContext = aContext.get_PopupIPCTabContext();
>+    if (popupContext.opener().type() != PBrowserOrId::TPBrowserParent) {
>+      ASSERT_UNLESS_FUZZING("Unexpected PopupIPCTabContext type.  Aborting AllocPBrowserParent.");
>+      return false;
>+    }
But now we deal with TPopupIPCTabContext only. Why you change the behavior for FrameIPCTabContext?

Comment 56

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

Could we add a check in parent process that if we get UnsafeIPCTabContext from child, we must have
service workers enabled. And if not enabled, crash the child.
Somewhere in nsIContentParent::CanOpenBrowser I guess.

Comment 57

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

Comment on attachment 8679766 [details] [diff] [review]
Part 1 - Move PBrowser::CreateWindow to PContent.

>+    nsCOMPtr<nsIDOMWindow> window;
>+
>+    TabParent::AutoUseNewTab aunt(newTab, aWindowIsNew, aURLToLoad);
>+
>+    *aResult = pwwatch->OpenWindow2(parent, finalURIString.get(),
>+                                    NS_ConvertUTF16toUTF8(aName).get(),
>+                                    NS_ConvertUTF16toUTF8(aFeatures).get(), aCalledFromJS,

You're changing aFeatures handling.
The old code has 
const char* features = aFeatures.Length() ? aFeatures.get() : nullptr;
and you just do NS_ConvertUTF16toUTF8(aFeatures).get(). Those are different things and OpenWindow2 even cares about that difference.
So, please revert to the old behavior.
Oh, now I see you also changed the type of aFeatures. Please don't do that.


Also, this very code is about to change so that we'll pass nullptr even in more cases. So depending on who lands first, this may need to be
merged. (the changes in the other bug are trivial, just more char* usage)

>-    sync CreateWindow(PBrowser aNewTab,
>-                      uint32_t aChromeFlags,
>-                      bool aCalledFromJS,
>-                      bool aPositionSpecified,
>-                      bool aSizeSpecified,
>-                      nsString aURI,
>-                      nsString aName,
>-                      nsCString aFeatures,
here you have nsCString


>+    sync CreateWindow(PBrowser aThisTab,
>+                      PBrowser aNewTab,
>+                      uint32_t aChromeFlags,
>+                      bool aCalledFromJS,
>+                      bool aPositionSpecified,
>+                      bool aSizeSpecified,
>+                      nsString aURI,
>+                      nsString aName,
>+                      nsString aFeatures,
here aFeatures.
Please don't make changes when moving code.


r+ assuming you upload an interdiff showing that the string handling changes are reverted.

Comment 58

[Cătălin Badea (:catalinb)]

Attachment: Interdiff - Revert CreateWindow changes when moving to PContent.

Comment 59

[Cătălin Badea (:catalinb)]

Attachment: Part 2 - Enable ServiceWorkerClients::OpenWindow on e10s desktop. (v2)

Comment 60

[Cătălin Badea (:catalinb)]

Attachment: Part 2 - Enable ServiceWorkerClients::OpenWindow on e10s desktop.

Comment 61

[Cătălin Badea (:catalinb)]

Attachment: interdiff for part 2 patch.

Comment 62

[Cătălin Badea (:catalinb)]

Looks like this hits an assert in mochitest-1. I'll fix it tomorrow, but it probably won't catch the merge to aurora.

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

Comment 63

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

Or right, merge date changed. crap.

Comment 64

[Cătălin Badea (:catalinb)]

Attachment: Part 1 - Move PBrowser::CreateWindow to PContent.

Comment 65

[Cătălin Badea (:catalinb)]

Attachment: Part 2 - Enable ServiceWorkerClients::OpenWindow on e10s desktop.

Comment 66

[Cătălin Badea (:catalinb)]

Attachment: Part 3 - Fix openWindow mochitest to work on e10s.

Comment 67

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e3900a90031
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccc1d3d2d2a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/99a4fb4ba5c1

Comment 68

[Carsten Book [:Tomcat]]

bad to backout part 3, this caused test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=16552402&repo=mozilla-inbound

Comment 69

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/828a355a570f

Comment 70

[Cătălin Badea (:catalinb)]

I did a git bisect and the failure was introduced in bug 1191724. Working on a fix.

Comment 71

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89c9c63c7654

Comment 72

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

https://hg.mozilla.org/mozilla-central/rev/0e3900a90031
https://hg.mozilla.org/mozilla-central/rev/ccc1d3d2d2a7

Comment 73

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/89c9c63c7654

Comment 74

[Edwin Wong [:edwong]]

:catalinb - the push team would like to get your patch uplifted to aurora, can you make the request. I spoke with overholt as well.

Comment 75

[Cătălin Badea (:catalinb)]

Comment on attachment 8678279 [details] [diff] [review]
Implement service workers clients.openWindow for desktop (non-e10s).

Approval Request Comment
[Feature/regressing bug #]: Implement openWindow in ServiceWorkerClients
[User impact if declined]: Some Push usage scenarios will remain broken.
[Describe test coverage new/current, TreeHerder]: Mochitest included in a different patch. 
[Risks and why]: Low, this is a service worker specific feature.
[String/UUID change made/needed]: None

Comment 76

[Cătălin Badea (:catalinb)]

Comment on attachment 8680952 [details] [diff] [review]
Part 1 - Move PBrowser::CreateWindow to PContent.

Approval Request Comment
[Feature/regressing bug #]: Implement ServiceWorkerClients.openWindow
[User impact if declined]: Some Push usage scenarios will remain broken.
[Describe test coverage new/current, TreeHerder]: Mochitest added in a different patch.
[Risks and why]: Low, this particular patch just moves code around but doesn't make any changes in terms of functionality.
[String/UUID change made/needed]: None

Comment 77

[Cătălin Badea (:catalinb)]

(In reply to Cătălin Badea (:catalinb) from comment #75)
> Comment on attachment 8678279 [details] [diff] [review]
> Implement service workers clients.openWindow for desktop (non-e10s).
> 
> Approval Request Comment
> [Feature/regressing bug #]: Implement openWindow in ServiceWorkerClients
> [User impact if declined]: Some Push usage scenarios will remain broken.
> [Describe test coverage new/current, TreeHerder]: Mochitest included in a
> different patch. 
> [Risks and why]: Low, this is a service worker specific feature.
> [String/UUID change made/needed]: None

I made a mistake, this is the patch that adds testing.

Tested openWindow with same origin and different origin urls, including redirects. I also covered
the permission checking when calling openWindow.

Comment 78

[Cătălin Badea (:catalinb)]

Comment on attachment 8680953 [details] [diff] [review]
Part 2 - Enable ServiceWorkerClients::OpenWindow on e10s desktop.

Approval Request Comment
[Feature/regressing bug #]: Implement ServiceWorkerClients openWindow
[User impact if declined]: openWindow will not work in e10s. This feature is important for Push.
[Describe test coverage new/current, TreeHerder]: Test added in a different patch.
[Risks and why]: Low, service workers specific change.
[String/UUID change made/needed]: None

Comment 79

[Cătălin Badea (:catalinb)]

Comment on attachment 8680954 [details] [diff] [review]
Part 3 - Fix openWindow mochitest to work on e10s.

Approval Request Comment
[Feature/regressing bug #]: Implement ServiceWorkerClients openWindow
[User impact if declined]: This is needed for Push.
[Describe test coverage new/current, TreeHerder]: 
[Risks and why]: Low, test only change.
[String/UUID change made/needed]: None.

This patch updates the openWindow test to work correctly in e10s.

Comment 80

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0e3900a90031
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/ccc1d3d2d2a7
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/89c9c63c7654

Comment 81

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment 82

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8680952 [details] [diff] [review]
Part 1 - Move PBrowser::CreateWindow to PContent.

More Service Workers related uplifts for Aurora44.

Comment 83

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8680953 [details] [diff] [review]
Part 2 - Enable ServiceWorkerClients::OpenWindow on e10s desktop.

Aurora44+

Comment 84

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8680954 [details] [diff] [review]
Part 3 - Fix openWindow mochitest to work on e10s.

Test-only change, Aurora44+.

Comment 85

[Carsten Book [:Tomcat]]

part 1 failed to apply:

grafting 312165:0e3900a90031 "Bug 1172870 - Part 1 - Move PBrowser::CreateWindow to PContent. r=smaug"
merging dom/ipc/ContentChild.cpp
merging dom/ipc/ContentChild.h
merging dom/ipc/ContentParent.cpp
merging dom/ipc/PBrowser.ipdl
warning: conflicts during merge.
merging dom/ipc/PBrowser.ipdl incomplete! (edit conflicts, then use 'hg resolve --mark')
merging dom/ipc/PContent.ipdl
merging dom/ipc/TabChild.cpp
warning: conflicts during merge.
merging dom/ipc/TabChild.cpp incomplete! (edit conflicts, then use 'hg resolve --mark')
merging dom/ipc/TabParent.cpp
warning: conflicts during merge.
merging dom/ipc/TabParent.cpp incomplete! (edit conflicts, then use 'hg resolve --mark')
merging dom/ipc/TabParent.h
warning: conflicts during merge.
merging dom/ipc/TabParent.h incomplete! (edit conflicts, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

Comment 86

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Catalin, do you also want attachment 8678279 [details] [diff] [review] in addition to part 1-3 uplifted to Aurora? I would like to confirm as that patch still has a:?. Please let me know.

Comment 87

[Cătălin Badea (:catalinb)]

(In reply to Ritu Kothari (:ritu) from comment #86)
> Catalin, do you also want attachment 8678279 [details] [diff] [review] in
> addition to part 1-3 uplifted to Aurora? I would like to confirm as that
> patch still has a:?. Please let me know.

No, I didn't realize that patch was already merged to aurora.

Comment 88

[Cătălin Badea (:catalinb)]

(In reply to Carsten Book [:Tomcat] from comment #85)
> part 1 failed to apply:
> 
> grafting 312165:0e3900a90031 "Bug 1172870 - Part 1 - Move
> PBrowser::CreateWindow to PContent. r=smaug"
> merging dom/ipc/ContentChild.cpp
> merging dom/ipc/ContentChild.h
> merging dom/ipc/ContentParent.cpp
> merging dom/ipc/PBrowser.ipdl
> warning: conflicts during merge.

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/953d8110ed9d
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/be8391c58a83
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/70132ea00d70
> merging dom/ipc/PBrowser.ipdl incomplete! (edit conflicts, then use 'hg
> resolve --mark')
> merging dom/ipc/PContent.ipdl
> merging dom/ipc/TabChild.cpp
> warning: conflicts during merge.
> merging dom/ipc/TabChild.cpp incomplete! (edit conflicts, then use 'hg
> resolve --mark')
> merging dom/ipc/TabParent.cpp
> warning: conflicts during merge.
> merging dom/ipc/TabParent.cpp incomplete! (edit conflicts, then use 'hg
> resolve --mark')
> merging dom/ipc/TabParent.h
> warning: conflicts during merge.
> merging dom/ipc/TabParent.h incomplete! (edit conflicts, then use 'hg
> resolve --mark')
> abort: unresolved conflicts, can't continue
> (use hg resolve and hg graft --continue)

Comment 89

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/953d8110ed9d
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/be8391c58a83
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/70132ea00d70

Comment 90

[Chris Mills [:cmills]]

Documented here:

https://developer.mozilla.org/en-US/docs/Web/API/Clients/openWindow

Release note added:

https://developer.mozilla.org/en-US/Firefox/Releases/45#Service_Workers

A tech review would be great...cheers!

Comment 91

[Andrew Overholt [:overholt]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #90)
> Documented here:
> 
> https://developer.mozilla.org/en-US/docs/Web/API/Clients/openWindow
> 
> Release note added:
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/45#Service_Workers
> 
> A tech review would be great...cheers!

Catalin, can you take a look?

Comment 92

[Cătălin Badea (:catalinb)]

(In reply to Andrew Overholt [:overholt] from comment #91)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #90)
> > Documented here:
> > 
> > https://developer.mozilla.org/en-US/docs/Web/API/Clients/openWindow
> > 
> > Release note added:
> > 
> > https://developer.mozilla.org/en-US/Firefox/Releases/45#Service_Workers
> > 
> > A tech review would be great...cheers!
> 
> Catalin, can you take a look?

I edited the description and added a better example for using openWindow.