1130687 - [Meta] Implement Service Worker Clients.openWindow()

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

See:

  https://github.com/slightlyoff/ServiceWorker/issues/603
  https://github.com/slightlyoff/ServiceWorker/commit/6fd0399a145e657d9d96190f1f2fa00e0bef4e2e

And also:

  https://github.com/slightlyoff/ServiceWorker/issues/602

Comment 1

[Albert [:albert]]

Attachment: Patch v1

Comment 2

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

Comment on attachment 8582521 [details] [diff] [review]
Patch v1

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

Patch looks good, but I have a few comments listed below.

Please also add a mochitest for this.

::: dom/workers/ServiceWorkerClients.cpp
@@ +15,5 @@
>  #include "WorkerScope.h"
>  
> +#include "nsIWindowMediator.h"
> +#include "nsIDOMWindow.h"
> +#include "nsIWindowWatcher.h"

alphabetical order

@@ +51,5 @@
>  // keeping the worker alive.
>  class PromiseHolder final : public WorkerFeature
>  {
>    friend class MatchAllRunnable;
> +  friend class OpenWindowRunnable;

The PromiseHolder will be refactored because it duplicates a lot of code from PromiseWorkerProxy. See bug 1130686.

@@ +169,5 @@
>  
> +class ResolveOrRejectPromiseWorkerRunnable final : public WorkerRunnable
> +{
> +  nsRefPtr<PromiseHolder> mPromiseHolder;
> +  nsAutoPtr<ServiceWorkerClientInfo> mClientInfo;

I think you can use mozilla::UniquePtr<ServiceWorkerClientInfo> and Move() for passing the client object.

@@ +197,5 @@
> +
> +    if (mResult == NS_OK) {
> +      nsRefPtr<ServiceWorkerWindowClient> client;
> +      if (mClientInfo) {
> +        client = new ServiceWorkerWindowClient(promise->GetParentObject(), *mClientInfo);

resolve promise with client.

@@ +373,5 @@
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    if (!mUrl.EqualsLiteral(ABOUT_BLANK) && !uriOrigin.Equals(info.mOrigin)) {
> +      return NS_OK;

The window should be created even if the origins don't match, but the promise will be resolved to null.

Step 1 and step 4 from the spec.

@@ +404,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(win);

Have you checked if the load event is dispatched on the new window when the url is about:blank?

Comment 3

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

Comment on attachment 8582521 [details] [diff] [review]
Patch v1

Please ask for review from Catalin for code related to clients stuff. I'm happy to feedback/review changes if Catalin feels that is required after he is done. Thanks.

Comment 4

[Albert [:albert]]

Attachment: Patch v2

Changes from comment 2

Comment 5

[Albert [:albert]]

Depends on bug 1130686 for promise holder refactor.

Comment 6

[Albert [:albert]]

Attachment: Tests v1

Tests

Comment 7

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

Comment on attachment 8587278 [details] [diff] [review]
Tests v1

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

::: dom/workers/test/serviceworkers/clients_open_window_worker.js
@@ +4,5 @@
> +
> +  self.clients.openWindow('about:blank').then(function(result) {
> +    if (result && result instanceof WindowClient) {
> +      self.clients.openWindow('open_window.html').then(function(result) {
> +        if (result && result instanceof WindowClient) {

please check that result is a valid window client here.

@@ +6,5 @@
> +    if (result && result instanceof WindowClient) {
> +      self.clients.openWindow('open_window.html').then(function(result) {
> +        if (result && result instanceof WindowClient) {
> +          self.clients.openWindow('http://example.com').then(function(result) {
> +            if (result === false) {

Shouldn't the result be null here?

::: dom/workers/test/serviceworkers/test_clients_open_window.html
@@ +31,5 @@
> +
> +  function getMessageListener() {
> +    return new Promise(function(res, rej) {
> +      window.onmessage = function(e) {
> +        ok(e.data, "windowOpen successfull");

nit: successful

@@ +57,5 @@
> +    start()
> +      .then(test)
> +      .catch(function(e) {
> +        ok(false, "Some test failed with error " + e);
> +      }).then(SimpleTest.finish);

Close all opened windows before finishing the test.

Comment 8

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

Comment on attachment 8586711 [details] [diff] [review]
Patch v2

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

r=me with these changes.
Please also ask review from baku.

::: dom/workers/ServiceWorkerClients.cpp
@@ +118,5 @@
> +    MOZ_ASSERT(promise);
> +
> +    if (mResult == NS_OK) {
> +      if (!mClientInfo) {
> +        promise->MaybeResolve(nullptr);

promise->MaybeResolve(JS::NullHandleValue);

@@ +198,5 @@
> +public:
> +  OpenWindowRunnable(WorkerPrivate* aWorkerPrivate,
> +                     PromiseWorkerProxy* aPromiseProxy,
> +                     const nsCString& aUrl)
> +    : mWorkerPrivate(aWorkerPrivate),

nit: comma on the next line

I know this is inconsistent with the rest of the file, but I'd like to change the coding style to match other SW source files.

@@ +224,5 @@
> +    nsRefPtr<ResolvePromiseWorkerOpenWindowRunnable> r =
> +      new ResolvePromiseWorkerOpenWindowRunnable(mWorkerPrivate, mPromiseProxy,
> +                                                 Move(clientInfo), result);
> +
> +    AutoSafeJSContext cx;

AutoJSAPI jsapi;
jsapi.Init();
JSContext* cx = jsapi.cx();

@@ +286,5 @@
> +    }
> +
> +    nsCOMPtr<nsIDOMWindow> win;
> +    rv = ww->OpenWindow(content, mUrl.get(), nullptr, nullptr, nullptr,
> +                        getter_AddRefs(win));

It looks like you're setting the currently opened window as the opener. This is not included in the spec.

@@ +302,5 @@
> +    nsAutoString uriOrigin;
> +    rv = nsContentUtils::GetUTFOrigin(uri, uriOrigin);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return NS_ERROR_FAILURE;
> +    }

You can use

nsCOMPtr<nsIScriptSecurityManager> securityManager = nsContentUtils::GetSecurityManager();
securityManager->CheckSameOriginURI(..);

@@ +309,5 @@
> +      return NS_OK;
> +    }
> +
> +    nsCOMPtr<nsIDocument> doc = window->GetExtantDoc();
> +    UniquePtr<ServiceWorkerClientInfo> clientInfo;

Remove this, it's not used anywhere.

Comment 9

[Albert [:albert]]

Attachment: Patch v3

Changes from comment 8

Comment 10

[Albert [:albert]]

(In reply to Cătălin Badea (:catalinb) from comment #8)
> Comment on attachment 8586711 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8586711 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +286,5 @@
> > +    }
> > +
> > +    nsCOMPtr<nsIDOMWindow> win;
> > +    rv = ww->OpenWindow(content, mUrl.get(), nullptr, nullptr, nullptr,
> > +                        getter_AddRefs(win));
> 
> It looks like you're setting the currently opened window as the opener. This
> is not included in the spec.

Then where should it be opened? If I pass nullptr instead of the browser window, a new window is created with size = 0, like a popup window.

Comment 11

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

(In reply to Albert [:albert] from comment #10)
> (In reply to Cătălin Badea (:catalinb) from comment #8)
> > Comment on attachment 8586711 [details] [diff] [review]
> > Patch v2
> > 
> > Review of attachment 8586711 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > @@ +286,5 @@
> > > +    }
> > > +
> > > +    nsCOMPtr<nsIDOMWindow> win;
> > > +    rv = ww->OpenWindow(content, mUrl.get(), nullptr, nullptr, nullptr,
> > > +                        getter_AddRefs(win));
> > 
> > It looks like you're setting the currently opened window as the opener. This
> > is not included in the spec.
> 
> Then where should it be opened? If I pass nullptr instead of the browser
> window, a new window is created with size = 0, like a popup window.

I've talked with bz yesterday, using the window mediator to find a browser window is the correct approach, but we certainly don't want to pass a parent to the new window. Have you looked at the aOpeningTab option?

I'll try to dig this further.

Comment 12

[Albert [:albert]]

Attachment: Patch v4

Use openURI of nsIBrowserDOMWindow instead of openWindow of nsIWindowWatcher. Now window.opener is null.

Comment 13

[Albert [:albert]]

Comment on attachment 8590227 [details] [diff] [review]
Patch v4

Remove review request because I found some problems while testing, ServiceWorkerClient url is always about:blank

Comment 14

[Albert [:albert]]

Attachment: Patch v5

Fixed client url

Comment 15

[Albert [:albert]]

Attachment: Tests v2

Changes from comment 7

Comment 16

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

Comment on attachment 8591620 [details] [diff] [review]
Tests v2

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

I have only a few comments. See below.

::: dom/workers/test/serviceworkers/clients_open_window/clients_open_window.html
@@ +4,5 @@
> +-->
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Bug 1139425 - controlled page</title>

nit: wrong bug number

@@ +9,5 @@
> +<script class="testbody" type="text/javascript">
> +  var testWindow = parent;
> +  if (opener) {
> +    testWindow = opener;
> +  }

this is not needed, you'll only use opener.

::: dom/workers/test/serviceworkers/test_clients_open_window.html
@@ +62,5 @@
> +    SimpleTest.finish();
> +  }
> +
> +  function runTest() {
> +    info(window.opener == undefined);

remove this.

@@ +66,5 @@
> +    info(window.opener == undefined);
> +    start()
> +      .then(test)
> +      .catch(function(e) {
> +        closeWindows();

Is this defined anywhere? From what I understand from this test, you're closing the windows in clear().

Comment 17

[Albert [:albert]]

Attachment: Tests v3

Changes from comment 16

Comment 18

[Andrea Marchesini [:baku]]

Comment on attachment 8590998 [details] [diff] [review]
Patch v5

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

r- just for the incumbent settings object.

::: dom/workers/ServiceWorkerClients.cpp
@@ +105,5 @@
> +    : WorkerRunnable(aWorkerPrivate, WorkerThreadModifyBusyCount),
> +      mPromiseProxy(aPromiseProxy),
> +      mClientInfo(Move(aClientInfo)),
> +      mResult(aResult)
> +

extra line here.

@@ +212,5 @@
> +
> +    UniquePtr<ServiceWorkerClientInfo> clientInfo;
> +    clientInfo.reset(new ServiceWorkerClientInfo(doc));
> +
> +    nsRefPtr<ResolvePromiseWorkerOpenWindowRunnable> r =

This is already implemented in the OpenWindowRunnable.
Can you unify these 2 blocks?

@@ +256,5 @@
> +                     PromiseWorkerProxy* aPromiseProxy,
> +                     const nsCString& aUrl)
> +    : mWorkerPrivate(aWorkerPrivate)
> +    , mPromiseProxy(aPromiseProxy)
> +    , mUrl(aUrl)

Here you do

ctr()
  : a
  , b
{ ...

but in the other classes you do:

ctr()
  : a,
    b
{ ...

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

this could fail. do:

result = target->AddEventListener( ...
if (NS_SUCCEEDED(result)) {
  return NS_OK;
}

NS_WARNING("Failed to set a 'load' EventListener.");

The ResolvePromiseWorkerOpenWindowRUnnable will about the promise.

@@ +320,5 @@
> +    WorkerPrivate::LocationInfo& info = mWorkerPrivate->GetLocationInfo();
> +
> +    nsresult rv = NS_NewURI(getter_AddRefs(baseURI), info.mOrigin);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return NS_ERROR_TYPE_ERR;

return rv;

@@ +325,5 @@
> +    }
> +
> +    rv = NS_NewURI(getter_AddRefs(uri), mUrl, nullptr, baseURI);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return NS_ERROR_TYPE_ERR;

return rv;

@@ +333,5 @@
> +
> +    // Open window
> +    nsCOMPtr<nsIWindowMediator> wm = do_GetService(NS_WINDOWMEDIATOR_CONTRACTID, &rv);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return NS_ERROR_FAILURE;

return rv;

@@ +366,5 @@
> +
> +    nsCOMPtr<nsIDOMWindow> win;
> +    rv = bwin->OpenURI(uri, nullptr, nsIBrowserDOMWindow::OPEN_DEFAULTWINDOW,
> +                                     nsIBrowserDOMWindow::OPEN_NEW,
> +                                     getter_AddRefs(win));

weird indentation.

@@ +367,5 @@
> +    nsCOMPtr<nsIDOMWindow> win;
> +    rv = bwin->OpenURI(uri, nullptr, nsIBrowserDOMWindow::OPEN_DEFAULTWINDOW,
> +                                     nsIBrowserDOMWindow::OPEN_NEW,
> +                                     getter_AddRefs(win));
> +    if (NS_WARN_IF(NS_FAILED(rv)) || !win) {

I would do:

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

MOZ_ASSERT(win);

@@ +375,5 @@
> +    // Check origin
> +    nsCOMPtr<nsIScriptSecurityManager> securityManager =
> +      nsContentUtils::GetSecurityManager();
> +    rv = securityManager->CheckSameOriginURI(uri, baseURI, false);
> +    if (!NS_SUCCEEDED(rv) && !mUrl.EqualsLiteral(ABOUT_BLANK)) {

I don't think we should allow about:blank, personally. But you are right implementing the spec.

"If the origin of url is not the origin of the incumbent settings object, then: "
this part of the spec is not implemented. right? the baseURI is not the origin of the incumbent settings object.

@@ +448,5 @@
> +  nsRefPtr<PromiseWorkerProxy> promiseProxy =
> +    PromiseWorkerProxy::Create(workerPrivate, promise);
> +  if (!promiseProxy->GetWorkerPromise()) {
> +    // Don't dispatch if adding the worker feature failed.
> +    return promise.forget();

promise->MaybeRejectr(NS_ERROR_FAILURE); before returning the promise?

Comment 19

[Andrea Marchesini [:baku]]

For the incumbent global, check mozilla::dom::GetIncumbentGlobal(). QI to nsPIDOMWindow, and from there you can have access to the nsIPrincipal. From the nsIPrincipal do: checkMayLoad().

Comment 20

[Albert [:albert]]

(In reply to Andrea Marchesini (:baku) from comment #18)
> Comment on attachment 8590998 [details] [diff] [review]
> Patch v5
> 
> Review of attachment 8590998 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- just for the incumbent settings object.
> 
> @@ +212,5 @@
> > +
> > +    UniquePtr<ServiceWorkerClientInfo> clientInfo;
> > +    clientInfo.reset(new ServiceWorkerClientInfo(doc));
> > +
> > +    nsRefPtr<ResolvePromiseWorkerOpenWindowRunnable> r =
> 
> This is already implemented in the OpenWindowRunnable.
> Can you unify these 2 blocks?

I've used Runnable and WorkerRunnable in order ro be consistent with MatchAll implementation. But no problem, I can unify.

> @@ +256,5 @@
> > +                     PromiseWorkerProxy* aPromiseProxy,
> > +                     const nsCString& aUrl)
> > +    : mWorkerPrivate(aWorkerPrivate)
> > +    , mPromiseProxy(aPromiseProxy)
> > +    , mUrl(aUrl)
> 
> Here you do
> 
> ctr()
>   : a
>   , b
> { ...
> 
> but in the other classes you do:
> 
> ctr()
>   : a,
>     b
> { ...

I am aware of that. Catalin wanted to do the change as you can see in comment 8 (@@ +198,5 @@). I can modify the style of the whole file.

Comment 21

[Albert [:albert]]

(In reply to Andrea Marchesini (:baku) from comment #19)
> For the incumbent global, check mozilla::dom::GetIncumbentGlobal(). QI to
> nsPIDOMWindow, and from there you can have access to the nsIPrincipal. From
> the nsIPrincipal do: checkMayLoad().

mozilla::dom::GetIncumbentGlobal() returns null, I guess it is null because we don't have a window in sw context.

Comment 22

[Andrea Marchesini [:baku]]

Comment on attachment 8595185 [details] [diff] [review]
Tests v3

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

::: dom/workers/test/serviceworkers/clients_open_window/clients_open_window.html
@@ +8,5 @@
> +  <title>Bug 1130687 - controlled page</title>
> +<script class="testbody" type="text/javascript">
> +  window.onload = function() {
> +    navigator.serviceWorker.ready.then(function(swr) {
> +        swr.active.postMessage("Start");

2 spaces indentation.

::: dom/workers/test/serviceworkers/clients_open_window_worker.js
@@ +1,1 @@
> +onmessage = function(e) {

/* Any copyright is dedicated to the Public Domain.
 * http://creativecommons.org/publicdomain/zero/1.0/ */

@@ +19,5 @@
> +        e.source.postMessage(response);
> +      });
> +      return;
> +    }
> +    e.source.postMessage(response);

It's untested code, but what do you think about this approach:

self.clients.openWindow('about:blank').then(function(result) {
  if (!result || !(result instanceof WindowClient) || result.url != 'about:blank') {
    throw("openWindow with about:blank failed.");
  }
  return self.clients.openWindow('/open_window.html');
}).then(function(result) {
  if (!result || !(result instanceof WindowClient) || result.url != 'http://mochi.test:8888/open_window.html') {
    throw("openWindow with /open_window.html failed.");
  }
  return self.clients.openWindow('http://example.com');
}).then(function(result) {
  response = (result === null);
}).then(function() {
  e.source.postMessage(response);
}, function(e) {
  e.source.postMessage(false); // Here actually you can send th error message.
});

Comment 23

[Noemí Freire (:noemi)]

(In reply to Albert [:albert] from comment #21)
> (In reply to Andrea Marchesini (:baku) from comment #19)
> > For the incumbent global, check mozilla::dom::GetIncumbentGlobal(). QI to
> > nsPIDOMWindow, and from there you can have access to the nsIPrincipal. From
> > the nsIPrincipal do: checkMayLoad().
> 
> mozilla::dom::GetIncumbentGlobal() returns null, I guess it is null because
> we don't have a window in sw context.

A bug has been filled to clarify step 5.4 from the spec - https://github.com/slightlyoff/ServiceWorker/issues/695

A couple of issues have been also raised related to openWindow spec:
*step 5.3 - https://github.com/slightlyoff/ServiceWorker/issues/694
*"about:blank" - https://github.com/slightlyoff/ServiceWorker/issues/696

waiting for those clarifications in order to move forward with this implementation. Thanks!

Comment 24

[Albert [:albert]]

:baku and :bz managed the spec bugs, clearing the ni.

Comment 25

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

There is a possibility this won't work on e10s in its current incarnation. Please test that. We may have to use IPC in the GeckoProcessType_Content case.

Comment 26

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

Also, have you considered just using PIWindowWatcher::OpenWindow2() except with the parent set to nullptr? That is what window.open() does. Of course, there might be some assertions or undefined behaviour with having the parent null, but it may be worth a try since it deals with e10s.

Comment 27

[Albert [:albert]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #25)
> There is a possibility this won't work on e10s in its current incarnation.
> Please test that. We may have to use IPC in the GeckoProcessType_Content
> case.

You are right, it doesn't work with e10s enabled and we are in GeckoProcessType_Content.

Comment 28

[Albert [:albert]]

Attachment: Patch WIP

Following Nikhil advice about using IPC due to GeckoProcessType_Content case when e10s is enabled, the patch adds IPC call from child to content in order to open a window in parent. However there are some points that need to be fixed:

- 'nsIBrowserDOMWindow::OpenURI' opens a new tab as expected in parent but the returned nsIDOMWindow is null, so can't create a ServiceWorkerClientInfo.

- 'nsPIWindowWatcher::OpenWindow2' opens a new browser window in parent instead of a tab. In this case I get the created nsIDOMWindow reference.

- I need to return the window reference from parent to child in order to create the ServiceWorkerClientInfo needed to resolve the promise. Is it possible to pass a nsIDOMWindow through IPC?

On the other hand, talking with Andrea he told me that he doesn't like the idea of using IPC to open a window in parent because he thinks that we have the all IPC thing in order to avoid having the parent busy and controlling windows and if we open the window in the parent it means that we have a way to run code in the parent process, what seems wrong to him.

So trying to figure what's going wrong when we are in GeckoProcessType_Content case I've seen that I get the assertion:

###!!! ASSERTION: attempted to open a new window with no WindowCreator: 'mWindowCreator', file /mozilla-central/embedding/components/windowwatcher/nsWindowWatcher.cpp, line 726

After trying to create a WindowCreator and set it to 'nsWindowWatcher' I get another assertion:
###!!! ASSERTION: Content should not be calling this: 'Not Reached', file /mozilla-central/chrome/nsChromeRegistryContent.cpp, line 216

Do you know if there is some way to avoid these errors?

Comment 29

[Boris Zbarsky [:bzbarsky]]

> but the returned nsIDOMWindow is null

Why is it null?  Sounds like a bug in the nsIBrowserDOMWindow implementation which we should fix.

That said, isn't this thing expected to work even if there is no browser window open?  If so, where are you getting the nsIBrowserDOMWindow from?

> 'nsPIWindowWatcher::OpenWindow2' opens a new browser window in parent

Sure, if you're not passing an opener from which it could find a place to open the tab...

> I need to return the window reference from parent to child

I have no idea how this is expected to work, actually.  What currently happens when a web page does window.open() in e10s?  Where is the opening handled?

> Is it possible to pass a nsIDOMWindow through IPC?

I would expect not, but check with Bill?

> he doesn't like the idea of using IPC to open a window in parent

I don't see how you have much of a choice; the new tab needs creating in the parent no matter what, no?  So at some point the parent will be involved.

Certainly you can't use the window creator in the child process (hence the asserts), because that needs to do things like create window chrome, which always lives in the parent process.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Oh, as far as sending over IPC goes... I'd think you want to ask the parent process to create a new window in the content process and tell you about it.  The only reason I'm not sure how it works in practice is that I thought we created about:blank windows in the parent process...  but that can't possibly work for the window.open case.

What I don't see in this bug is a clear description of where this new window is expected to open in the following cases:

1)  Multiple child processes, which may or may not currently have any content windows in them.

2)  No toplevel (parent process) windows at all.  Assuming this case is relevant; I guess the hidden window is always there on Mac, but trying to open things in it seems like a bad idea.

Comment 31

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

CC'ing mconley for window.open()-in-e10s discussions.

Comment 32

[Albert [:albert]]

Attachment: Patch WIP

Added serializable ServiceWorkerClientInfo in order to be able to send the window client from parent to child through IPC after window opened.

Comment 33

[Wilson Page [:wilsonpage]]

Where is the best place to learn more about this feature?

Comment 34

[Albert [:albert]]

Attachment: Patch WIP

Added IPC types

Comment 35

[Albert [:albert]]

I tried the following in the parent:

a) Get the mostRecent browser window from WindowMediator in order to get a nsIBrowserWindow and call OpenURI

  FirefoxOS -> Failed, GetBrowserDOMWindow on nsIDOMChromeWindow returns null.
  Desktop -> Successfully opens the url in a new tab as expected but the window returned by OpenURI is null.

b) Call OpenWindow2 of WindowWatcher and set the most recent browser window as a opener

  FirefoxOS -> Failed, debugging I see that OpenWindow2 internally gets a null xulParent in nsAppStartup::CreateChromeWindow2.
  Desktop -> Successfully opens the url in a separate window (should be a tab) and returns a window reference not null.

c) Call Open (equivalent to window.open in js) in the nsGlobalWindow that I get from WindowMediator most recent window.

  FirefoxOS -> Failed, debugging I see that OpenWindow2 internally gets a null xulParent in nsAppStartup::CreateChromeWindow2.
  Desktop -> Successfully opens the url in a separate window (should be a tab) and returns a window reference not null.

I saw that window.open uses WindowOpen2, for that reason cases 'b' and 'c' are the same.

Do you see something wrong or do I miss some other method to open a window? How can I open a window in Firefox OS?

Comment 36

[Boris Zbarsky [:bzbarsky]]

I have no idea what the setup on FxOS looks like, sorry.  It's not even clear to me that "open a window" is a sensible operation there or that there is any XUL involved at all.  Note that the desktop window stuff is all very XUL-based, of course.

Past that:

* Did you test your option (a) on Mac desktop while no browser windows are open?
* I suspect that you do not want to use a browser window as opener (as in option (b)).

You noticed yourself that your (b) and (c) are the same, ok.

Here's what I think you want to do here.  You want to set up some service that exposes an API to do what you want.  You will then want different implementations of this service for at least FxOS and desktop, and probably a different one again for Android I expect, with its native UI that I bed WindowMediator doesn't work on at all.  Then you talk to the people familiar with those front ends to see how they want to implement this functionality for their front end.

Comment 37

[Albert [:albert]]

As detailed in comment 35, I didn't find a way to open a window in Firefox OS. Can you give me some advice about how to manage the open window?

Maybe sending a system message to shell.js, where I guess I can open the window and send back a message with  the info of the new window?

Comment 38

[Albert [:albert]]

I'm getting null window when calling OpenURI from chrome process (see comment 35). In bug 1102020 I see the same behavior reported and you said in comment 1 that OpenURI is no longer used in e10s anymore, you advice to use openURIInFrame instead. So I tried to use the openURIInFrame and it seems to work, a new tab is created and I obtain a nsIFrameLoaderOwner from where I can get the info I need for the ServiceWorkerClientInfo. However I see a couple of problems:

- The new tab opened about:blank instead of the uri provided, I guess that I need to tweak something, doesn't seem a big problem.

- I'm concerned about how openURIInFrame will work is the browser doesn't have tabs enabled. openURI works as a kind of wrapper that tries to open window or tab depending on the environment (preferences and other stuff). So with openURIInFrame funciton do I have to check these thinks in my own and call openURIInFrame or openwindow2 if I want a tab or a window?

Comment 39

[Noemí Freire (:noemi)]

Hi,

As raised in comment 36 we've just opened different bugs to deal with all the tasks to be performed here:
*Bug 1172869 Implement a single API to deal with window.open() scenarios
*Bug 1172870 Implement Service Worker Clients.OpenWindow() for Desktop
*Bug 1172871 Implement Service Worker Clients.OpenWindow() for FirefoxOS
*Bug 1172872 Implement Service Worker Clients.OpenWindow() for Android

Comment 40

[Noemí Freire (:noemi)]

Since we will be working on the specific tasks listed in comment 39 rather than within this meta just removing the assignment here. Thanks!

Comment 41

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

I think the right solution here is to open the window from the content process. The parent can open new windows/tabs, but then there's no way to get the DOM window out, since it lives in the content process. And it sounds like you want the DOM window.

We probably just want to do something like window.open does. The low-level code to do that in the child process is here:
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1414
However, you probably want to use the WindowWatcher code here:
http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/nsPIWindowWatcher.idl#69
I believe that should work for both b2g and desktop when called from the content process.

Comment 42

[Albert [:albert]]

(In reply to Bill McCloskey (:billm) from comment #41)
> I think the right solution here is to open the window from the content
> process. The parent can open new windows/tabs, but then there's no way to
> get the DOM window out, since it lives in the content process. And it sounds
> like you want the DOM window.
> 
> We probably just want to do something like window.open does. The low-level
> code to do that in the child process is here:
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1414
> However, you probably want to use the WindowWatcher code here:
> http://mxr.mozilla.org/mozilla-central/source/embedding/components/
> windowwatcher/nsPIWindowWatcher.idl#69
> I believe that should work for both b2g and desktop when called from the
> content process.

I first tried to open window in the content process but it didn't work. As you can read in comment 28 using openwindow2 in content I got the following assertions:

###!!! ASSERTION: attempted to open a new window with no WindowCreator: 'mWindowCreator', file /mozilla-central/embedding/components/windowwatcher/nsWindowWatcher.cpp, line 726

After trying to create a WindowCreator and set it to 'nsWindowWatcher' I got another assertion:
###!!! ASSERTION: Content should not be calling this: 'Not Reached', file /mozilla-central/chrome/nsChromeRegistryContent.cpp, line 216

Comment 43

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

I think mWindowCreator is supposed to be null. We should be using this code here:
https://dxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/nsWindowWatcher.cpp#654
That code should set newDocShellItem to a non-null value, so we shouldn't end up at the mWindowCreator assertion.

Perhaps you're passing in flags that are causing us to try to open a chrome window? We don't support that right now and I doubt we ever will.

Comment 44

[(no longer active)]

Can you even open new windows on b2g like this?  I'm pretty sure that in a non-distant past, even window.open didn't work.  Fabrice, do you know anything about this by any chance?

Comment 45

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

I haven't ever tried it, but there's a bunch of code and tests for window.open on b2g (part of dom/browser-element). I'd be very surprised if it didn't work.

Comment 46

[[:fabrice] Fabrice Desré]

window.open works from web content for sure. I'm not sure about using the ww watcher directly. Note that for window.open, we actually rely on gaia to create the frame.

Comment 47

[Albert [:albert]]

(In reply to Bill McCloskey (:billm) from comment #43)
> I think mWindowCreator is supposed to be null. We should be using this code
> here:
> https://dxr.mozilla.org/mozilla-central/source/embedding/components/
> windowwatcher/nsWindowWatcher.cpp#654
> That code should set newDocShellItem to a non-null value, so we shouldn't
> end up at the mWindowCreator assertion.
> 
> Perhaps you're passing in flags that are causing us to try to open a chrome
> window? We don't support that right now and I doubt we ever will.

No, I'm not passing flags:

rv = pwwatch->OpenWindow2(browserWindow, url_str.get(), nullptr, nullptr, false, false, true,
                          nullptr, nullptr, getter_AddRefs(win));

Note that browserWindow is the result of nsIWindowMediator::GetMostRecentWindow with "navigator:browser" type, which is returning null in content process.

Comment 48

[Albert [:albert]]

(In reply to Fabrice Desré [:fabrice] from comment #46)
> window.open works from web content for sure. I'm not sure about using the ww
> watcher directly. Note that for window.open, we actually rely on gaia to
> create the frame.

For b2g I'm able to open a window sending 'content-handler' async message, it is received in shell.js where a view  activity is created that opens a new window in the browser app. Is that ok?

Comment 49

[Noemí Freire (:noemi)]

ni to Bill McCloskey per comment 47, waiting for your feedback. Thanks!

Comment 50

[[:fabrice] Fabrice Desré]

(In reply to Albert [:albert] from comment #48)
> (In reply to Fabrice Desré [:fabrice] from comment #46)
> > window.open works from web content for sure. I'm not sure about using the ww
> > watcher directly. Note that for window.open, we actually rely on gaia to
> > create the frame.
> 
> For b2g I'm able to open a window sending 'content-handler' async message,
> it is received in shell.js where a view  activity is created that opens a
> new window in the browser app. Is that ok?

If you want an app context that will not provide it to you, right? I think you will need something similar, but with clear semantics, so that gaia's system app will create the right kind of iframe.

Comment 51

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

This is harder than I realized. The window.open code assumes that you pass in a parent window. And it uses that parent window's TabChild to communicate with the parent process. Since you don't have a parent window, that doesn't work.

The parent is kind of important since it tells us which XUL window to open into (if we're opening a new tab), whether we're in private browsing mode, etc.

I talked to Nikhil and it sounds like there isn't going to be any parent window that we can use as the parent. The requirements here are as follows:

- We want to open a new tab (or a new window, depending on the users prefs, I think) in the current top XUL window.
- The DOM window has to run in the same content process as the service worker.
- The service worker code doesn't necessarily need an nsIDOMWindow for the new window; Nikhil says we can get away with just a window ID.

We just don't have anything that satisfies these requirements right now, unfortunately. If you open a new tab from the chrome process, it won't necessarily run from the right content process. And, right now, the only way to open a window in the content process is to have an opener.

I think we'll need a new code path that opens a new tab/window from a given content process without an opener. The code will be similar, but not identical, to what we do now for window.open. Hopefully we can share most of it.

I think the best I can do is to describe how window.open works now on desktop. I don't understand the b2g path well.

- We get to TabChild::ProvideWindow from the OpenWindow path I pasted earlier:
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1418
- That goes to ProvideWindowCommon.
- It allocates a new TabChild, which is sent to the parent process.
- Then it sends a CreateWindow message, including a reference to the new TabChild, call it NEWTAB.
- The message is received in TabParent::RecvCreateWindow.
- There we try to figure out whether we're opening a new tab or window. We call into browser chrome code via OpenURIInFrame or OpenWindow2 to do most of the work. We stash the new TabParent NEWTAB that was sent up in a global variable, TabParent::sNextTabParent.
- Chrome code does a bunch of JS stuff that causes us to create a new <browser remote=true> element. When that's added to the DOM, we end up here:
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#1141
- We look at the saved TabParent NEWTAB and attach it to the new <browser> element.

To do this for service workers, we would have to split this code off so it's no longer attached to TabParent/Child. Instead we'd need to pass in something to tell us what window to use for the tab, whether it's private or not, etc. I'm guessing the work for b2g would be similar, but perhaps less complex.

Comment 52

[Albert [:albert]]

Opened issue to request optional param 'name' in the spec as it will be useful for the toolkit team in order to get a reference to the new window:

https://github.com/slightlyoff/ServiceWorker/issues/711

Comment 53

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Albert [:albert] from comment #37)
> As detailed in comment 35, I didn't find a way to open a window in Firefox
> OS. Can you give me some advice about how to manage the open window?

I think we generally do this through what we call "Chrome events". I'd recommend talking to Gregor who should be able to point you to some examples of where we use this.

Comment 54

[duraseb]

(In reply to Jonas Sicking (:sicking) from comment #53)
> (In reply to Albert [:albert] from comment #37)
> > As detailed in comment 35, I didn't find a way to open a window in Firefox
> > OS. Can you give me some advice about how to manage the open window?
> 
> I think we generally do this through what we call "Chrome events". I'd
> recommend talking to Gregor who should be able to point you to some examples
> of where we use this.

That would be really helpful. I can receive push notifications in serviceworker, show a notification to the user, but still can't find a way to open a browser window when the user clicks on the notification.
Is there a place I can find more information on how to open a window from a service worker?

Comment 55

[Gregor Wagner [:gwagner]]

Kevin, can you help out here?

Comment 56

[Kevin Grandon :kgrandon]

(In reply to Gregor Wagner [:gwagner] from comment #55)
> Kevin, can you help out here?

I'm not sure exactly what's needed here. It looks like Fabrice chimed in later with some details? Happy to help if I can, but I'm not sure I'm the right person for the job. Are we talking about this?


(In reply to [:fabrice] NOT READING BUGMAIL UNTIL JULY 27 from comment #50)
> If you want an app context that will not provide it to you, right? I think
> you will need something similar, but with clear semantics, so that gaia's
> system app will create the right kind of iframe.


If there's something gaia-specific that I can help with, please file a gaia bug which isolates the problem. Thanks!

Comment 57

[Albert [:albert]]

(In reply to Jonas Sicking (:sicking) from comment #53)
> (In reply to Albert [:albert] from comment #37)
> > As detailed in comment 35, I didn't find a way to open a window in Firefox
> > OS. Can you give me some advice about how to manage the open window?
> 
> I think we generally do this through what we call "Chrome events". I'd
> recommend talking to Gregor who should be able to point you to some examples
> of where we use this.

That's what I tried in the WIP of bug 1172871. See https://bugzilla.mozilla.org/show_bug.cgi?id=1172871#c2

Comment 58

[Albert [:albert]]

(In reply to Kevin Grandon :kgrandon from comment #56)
> (In reply to Gregor Wagner [:gwagner] from comment #55)
> > Kevin, can you help out here?
> 
> I'm not sure exactly what's needed here. It looks like Fabrice chimed in
> later with some details? Happy to help if I can, but I'm not sure I'm the
> right person for the job. Are we talking about this?
> 
> 
> (In reply to [:fabrice] NOT READING BUGMAIL UNTIL JULY 27 from comment #50)
> > If you want an app context that will not provide it to you, right? I think
> > you will need something similar, but with clear semantics, so that gaia's
> > system app will create the right kind of iframe.
> 
> 
> If there's something gaia-specific that I can help with, please file a gaia
> bug which isolates the problem. Thanks!

There is already a gaia-specific bug https://bugzilla.mozilla.org/show_bug.cgi?id=1172871

Comment 59

[Noemí Freire (:noemi)]

Hi,

Please refer to comment 57 and comment 58. Thanks!

Comment 60

[Kevin Grandon :kgrandon]

Ok, will look at that bug, but it looks like it's a little bit more deep in platform than I typically deal with.

Comment 61

[Ben Kelly [:bkelly, not reviewing]]

Note, the spec has changed recently in how it validates the origin of the window:

  https://github.com/slightlyoff/ServiceWorker/issues/646

Comment 62

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

I tried comment 51 a few weeks ago, but didn't get very far, putting up the patches in case someone finds them useful, and the problems I faced.

Comment 63

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

Attachment: Use a chrome event

The problem with this is the same problem as with other efforts, since we may not have an existing window when calling openWindow, the chrome event dispatch just fails (at DispatchChromeEvent in ServiceWorkerClients.cpp

Comment 64

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

Attachment: move CreateWindow to ContentParent

This is disjoint from patch "Use a chrome event".

It implements the first step of moving TabParent::RecvCreateWindow(new window) to ContentParent::RecvCreateWindow(old window, new window), so that eventually old window will not be required, which is the crux of fixing this bug. I have added an openwindow call to the serviceworker for dom/workers/test/serviceworkers/test_notificationclick.html just for testing. It works in non-10s but the window that is opened doesn't have the right UI (it is missing the address bar and so on). It does not work on e10s, which will require finding/spawning a child process.

Comment 66

[Bill Maggs (bmaggs)]

Does there need to be a non-e10s version of this bug to get the behavior in the 43 timeframe? Pasted from duplicate bug: 

 Nikhil Marathe [:nsm] (please needinfo?) 2015-09-08 10:59:27 PDT

This needs work from someone who knows the window/PContent/PBrowser/e10s stuff, especially if we are going to ship this with e10s enabled. For non-e10s it *may* be easier. I tried taking a stab at this (patches in Bug 1130687) but no dice. The fact that any of our window opener code relies on there being an existing window around (based on the premise that such calls come from window.open() or similar) complicates everything when service workers are involved (where there may not be a window present). Based on this, I don't feel confident in assigning this to anyone not familiar with that code.

Comment 67

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

(In reply to Bill Maggs from comment #66)
> Does there need to be a non-e10s version of this bug to get the behavior in
> the 43 timeframe? 

My patch does work in non-e10s, although not completely. Does not work on e10s. Not sure what you were asking.

Comment 68

[Bill Maggs (bmaggs)]

Mostly I was asking if solving the problem (more completely) for the non-e10s case was worth prioritizing in some formal way. I know we're not supposed to land stuff that does not work in e10s, but in this case unless we can make the SW work right, we just have a broken experience with Push and should not ship it.

Comment 69

[(no longer active)]

I don't think we want to land something that doesn't work in e10s here.

Comment 70

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

I'm going to try working on this bug starting from Alberto's and Nikhil's patches.

The solution I have in mind is as follows:
For single process(non-e10s), we use WindowMediator to get the most recent browser window and open a new tab inside it. If we don't have a browser window, open a new xul window. I think we can do that using
windowwatcher.openWindow(parent=null, url="chrome://browser/content", args = nsISupportsArray(<target url>)). This call has the intended result, it opens a new window with chrome elements and loads the url, but I'm not sure about the security implications.

For e10s, based on billm's input from comment #51, have ContentChild implement nsIWindowProvider and try
to create a window through ipc. The tricky part is figuring out the proper way to allocate a new tab id
in the parent without bypassing security checks.

Comment 71

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

This would cover all desktop use cases, except for firefox running in e10s without any content process.

Comment 72

[Edwin Wong [:edwong]]

Any chance we can land the non-e10s fix asap and log a new bug for e10s use case - we have testing lined up for next week.

Comment 73

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

I can upload a patch for review later today.

Comment 74

[Ben Kelly [:bkelly, not reviewing]]

Catalin, I thought you were on vacation this week.  Please don't cut your break short.  This can wait until you get back.

Comment 75

[Chris Mills [:cmills]]

This method already seems to be documented, some time ago:

https://developer.mozilla.org/en-US/docs/Web/API/Clients/openWindow
https://developer.mozilla.org/en-US/Firefox/Releases/45#Service_Workers

Have I missed anything here? Let me know if I need to add anything else.