Implement Service Worker Clients.OpenWindow() for Desktop

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: noemi, Assigned: catalinb)

Tracking

({dev-doc-complete})

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected, firefox44 fixed, firefox45 fixed, b2g-v2.5 fixed)

Details

Attachments

(5 attachments, 18 obsolete attachments)

31.73 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.07 KB, patch
Details | Diff | Splinter Review
52.81 KB, patch
Details | Diff | Splinter Review
30.14 KB, patch
Details | Diff | Splinter Review
9.73 KB, patch
Details | Diff | Splinter Review
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.
Depends on: 1172869
Assignee: nobody → alberto.crespellperez
Status: NEW → ASSIGNED
Whiteboard: [s4]
Target Milestone: --- → NGA S3 (26Jun)
Posted patch Patch WIP (obsolete) — Splinter Review
Adds the component in charge of open window
Whiteboard: [s4]
Component: DOM → DOM: Service Workers
Target Milestone: NGA S3 (26Jun) → FxOS-S1 (26Jun)
Posted patch Patch WIP (obsolete) — Splinter Review
Added open window logic
Attachment #8621462 - Attachment is obsolete: true
Target Milestone: FxOS-S1 (26Jun) → FxOS-S2 (10Jul)
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.
Flags: needinfo?(wmccloskey)
Nikhil probably has a better idea of who could work on this.
Flags: needinfo?(wmccloskey) → needinfo?(nsm.nikhil)
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.
Flags: needinfo?(nsm.nikhil) → needinfo?(markh)
(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...
Flags: needinfo?(markh) → needinfo?(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?
Flags: needinfo?(mconley) → needinfo?(alberto.crespellperez)
(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
Flags: needinfo?(alberto.crespellperez) → needinfo?(mconley)
We really need someone from platform to work on this. I'll ask Andrew Overholt about it.
Flags: needinfo?(mconley)
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?
Flags: needinfo?(alberto.crespellperez)
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!
Flags: needinfo?(alberto.crespellperez)
Target Milestone: FxOS-S2 (10Jul) → ---
Releasing it, please ni me if you need some feedback.
Assignee: alberto.crespell → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(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?
Flags: needinfo?(mconley) → needinfo?(overholt)
Catalin knows the details.
Flags: needinfo?(overholt) → needinfo?(catalin.badea392)
Posted patch Move CreateWindow to PContent (obsolete) — Splinter Review
(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
Flags: needinfo?(catalin.badea392)
Flags: needinfo?(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?
Flags: needinfo?(mconley) → needinfo?(catalin.badea392)
(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.
Flags: needinfo?(catalin.badea392)
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.
Based on patches from:
  Nikhil Marathe <nsm.nikhil@gmail.com>
  Alberto Crespell Perez <alberto.crespell@gmail.com>
Attachment #8625155 - Attachment is obsolete: true
Attachment #8660912 - Attachment is obsolete: true
Attachment #8660913 - Attachment is obsolete: true
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.
Attachment #8668894 - Flags: review?(amarchesini)
Assignee: nobody → catalin.badea392
Status: NEW → ASSIGNED
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.
Attachment #8668894 - Flags: review?(bugs)
Attachment #8668894 - Flags: review?(amarchesini)
Attachment #8668894 - Flags: review+
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)
Attachment #8668894 - Flags: review?(bugs) → review-
(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.
(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)
Please see the previous comment.
Flags: needinfo?(bugs)
No longer depends on: 1172869
Blocks: 1130687
(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.
Flags: needinfo?(bugs)
Btw, there is https://github.com/slightlyoff/ServiceWorker/issues/728
and I'd say the spec is broken here. Way too racy.
Based on patches from:
  Nikhil Marathe <nsm.nikhil@gmail.com>
  Alberto Crespell Perez <alberto.crespell@gmail.com>
Attachment #8676341 - Flags: review?(bugs)
Based on patches from:
  Nikhil Marathe <nsm.nikhil@gmail.com>
  Alberto Crespell Perez <alberto.crespell@gmail.com>
Attachment #8668894 - Attachment is obsolete: true
Attachment #8676341 - Attachment is obsolete: true
Attachment #8676341 - Flags: review?(bugs)
Attachment #8676342 - Flags: review?(bugs)
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!
Attachment #8676815 - Flags: feedback?(bugs)
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.
Attachment #8676342 - Flags: review?(bugs) → review-
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.
Attachment #8676815 - Flags: feedback?(bugs) → feedback+
oh, one thing, don't use any name for the opened window.
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.
This also includes tests for redirects.

Based on patches from:
  Nikhil Marathe <nsm.nikhil@gmail.com>
  Alberto Crespell Perez <alberto.crespell@gmail.com>
Attachment #8676342 - Attachment is obsolete: true
Attachment #8677501 - Flags: review?(bugs)
(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.
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 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)
Attachment #8677501 - Flags: review?(bugs) → review-
Based on patches from:
  Nikhil Marathe <nsm.nikhil@gmail.com>
  Alberto Crespell Perez <alberto.crespell@gmail.com>
Attachment #8677501 - Attachment is obsolete: true
Based on patches from:
  Nikhil Marathe <nsm.nikhil@gmail.com>
  Alberto Crespell Perez <alberto.crespell@gmail.com>
Attachment #8678272 - Attachment is obsolete: true
Based on patches from:
  Nikhil Marathe <nsm.nikhil@gmail.com>
  Alberto Crespell Perez <alberto.crespell@gmail.com>
Attachment #8678275 - Attachment is obsolete: true
Attachment #8678279 - Flags: review?(bugs)
Attachment #8678279 - Flags: review?(bugs) → review+
Keywords: leave-open
Attachment #8676815 - Attachment is obsolete: true
Attachment #8679652 - Flags: review?(bugs)
Attachment #8679652 - Attachment is obsolete: true
Attachment #8679652 - Flags: review?(bugs)
Attachment #8679766 - Flags: review?(bugs)
I'll review this stuff first thing tomorrow. (getting late here today)
Attachment #8679768 - Flags: review?(bugs) → review+
I wonder if I could keep CreateWindow in PBrowser and in ContentChild::ProvideWindowCommon do:
newTabChild = SendCreatePBrowser();
newWindow = newTabChild->CreateWindow(maybeTabChildOpener);
yes, I think it would make sense, given that the patch has PBrowserParent* aThisTab now as the first param.
Hmm, or maybe not, now that I've read the other patch. The first param becomes sort of optional there.
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?
Attachment #8679767 - Flags: review?(bugs) → review-
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 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.
Attachment #8679766 - Flags: review?(bugs) → review+
Attachment #8680217 - Attachment is obsolete: true
Posted patch interdiff for part 2 patch. (obsolete) — Splinter Review
Attachment #8680237 - Attachment description: patch.patch → interdiff for part 2 patch.
Attachment #8680224 - Flags: review?(bugs) → review+
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
Or right, merge date changed. crap.
Attachment #8680224 - Attachment is obsolete: true
Attachment #8680237 - Attachment is obsolete: true
bad to backout part 3, this caused test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=16552402&repo=mozilla-inbound
Flags: needinfo?(catalin.badea392)
I did a git bisect and the failure was introduced in bug 1191724. Working on a fix.
Flags: needinfo?(catalin.badea392)
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/0e3900a90031
https://hg.mozilla.org/mozilla-central/rev/ccc1d3d2d2a7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Depends on: 1220417
:catalinb - the push team would like to get your patch uplifted to aurora, can you make the request. I spoke with overholt as well.
Flags: needinfo?(catalin.badea392)
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
Flags: needinfo?(catalin.badea392)
Attachment #8678279 - Flags: approval-mozilla-aurora?
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
Attachment #8680952 - Flags: approval-mozilla-aurora?
(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 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
Attachment #8680953 - Flags: approval-mozilla-aurora?
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.
Attachment #8680954 - Flags: approval-mozilla-aurora?
Depends on: 1221992
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Comment on attachment 8680952 [details] [diff] [review]
Part 1 - Move PBrowser::CreateWindow to PContent.

More Service Workers related uplifts for Aurora44.
Attachment #8680952 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8680953 [details] [diff] [review]
Part 2 - Enable ServiceWorkerClients::OpenWindow on e10s desktop.

Aurora44+
Attachment #8680953 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8680954 [details] [diff] [review]
Part 3 - Fix openWindow mochitest to work on e10s.

Test-only change, Aurora44+.
Attachment #8680954 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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)
Flags: needinfo?(catalin.badea392)
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.
Flags: needinfo?(catalin.badea392)
Attachment #8678279 - Flags: approval-mozilla-aurora?
(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.
(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)
(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?
Flags: needinfo?(catalin.badea392)
(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.
Flags: needinfo?(catalin.badea392)
You need to log in before you can comment on or make changes to this bug.