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.

Adds the component in charge of open window

Added open window logic

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.

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

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.

(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...

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?

(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

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

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?

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!

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

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?

Catalin knows the details.

(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

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?

(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.

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>

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 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 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)

(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.

(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.

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>

Based on patches from: Nikhil Marathe <nsm.nikhil@gmail.com> Alberto Crespell Perez <alberto.crespell@gmail.com>

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 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 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.

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>

(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)

Based on patches from: Nikhil Marathe <nsm.nikhil@gmail.com> Alberto Crespell Perez <alberto.crespell@gmail.com>

Based on patches from: Nikhil Marathe <nsm.nikhil@gmail.com> Alberto Crespell Perez <alberto.crespell@gmail.com>

Based on patches from: Nikhil Marathe <nsm.nikhil@gmail.com> Alberto Crespell Perez <alberto.crespell@gmail.com>

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

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?

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.

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.

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

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

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

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

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

: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 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 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

(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

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.

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

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.

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

Comment on attachment 8680954 [details] [diff] [review] Part 3 - Fix openWindow mochitest to work on e10s. Test-only change, Aurora44+.

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)

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.

(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)

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

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!

(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?

(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.