826325 - Decide where window.open should open content

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

This is a spin-off of bug 818336. We already open <a href="..." target="_blank"> links in an external browser. We need to determine how to handle window.open(url, "_blank");

Myk has a nice summary here: https://bugzilla.mozilla.org/show_bug.cgi?id=818336#c3

Comment 1

[Myk Melez [:myk] [@mykmelez]]

cc:ing Jonas, whose insight on this issue is particularly valuable, not only for Android, but for our webapp runtimes on other platforms (Firefox OS, desktop) too.

Comment 2

[Wesley Johnston (:wesj)]

MozActivity support (bug 825041) would help clear this up somewhat for us (but I'd rather that we make window.open work for this much simpler case).

Comment 3

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

In B2G we do the following:

* <a href="..." target=_blank> always opens in a browser rather than in the app.
* window.open(url, "_blank") always opens in a browser rather than in the app.
* window.open(url, "somename") lets you open 1 window in the app. If you try to open
  a second one, we replace the first window with the newly requested URL. But we
  always open the window inside the app.

I don't necessarily think that desktop should follow the last bullet. We mostly have it since window management is a pain on small-screen devices. It makes sense for fennec to do the same thing, but on desktop I think we can allow opening any number of windows.

By the way, the B2G logic lives here:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/popup_manager.js#L46

(All credit to Vivien for answering these questions, cc'ing him to keep me honest)

Comment 4

[Erin Lancaster [:elan]]

This likely has impact across many apps, Wikipedia is one example. Triage drivers consensus is that this is a: P1. Just needs an assignee.

Comment 5

[Erin Lancaster [:elan]]

This likely has impact across many apps, Wikipedia is one example. Triage drivers consensus is that this is a: P1. Just needs an assignee.

Comment 6

[Jason Smith [:jsmith]]

Talked this over with Brad about tracking-fennec policy. This is apps-specific, so we don't need to track this.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch v1

So, the issue here is that because we're now designating ourselves as an app in docshell [1], we're falling into some B2G codepath's here. The B2G codepaths basically create an iframe and send it to the frontend via. an event (is there some good reason they're not using our normal nsIBrowserDOMWindow here?) Also, I'm not sure why they're creating the iframe in C++...

Regardless, rather than re-implement all of their event handling code, I think maybe we're better to just handle this in platform and get consistent behaviour across platforms? This basically does the "is this target='_blank'" check for us and sends the uri to the externalHelperAppService if it is.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

What in here is making the "we are in an app" determination?

Comment 9

[Wesley Johnston (:wesj)]

That's done when via command line parameters passed to the app when it starts. We pass in the manifest and then use it when we create new "tabs" (xul-browsers) to mark them as apps: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2428

Comment 10

[Boris Zbarsky [:bzbarsky]]

No, that's not what I meant.  What I meant is where in the code being added here is "in an app" being checked?  Is it just the GetIsInBrowserOrApp() test above the code being added?

Comment 11

[Wesley Johnston (:wesj)]

Oh, apologies. Yes, we're getting true at the isBrowserOrApp call above.

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK, I see.

How was this patch tested, if I might ask?  It's not clear to me why this does what we want, since returning null from a window provider will simply go ahead and open a new window via the window creator, no?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Or did you mean to return NS_ERROR_ABORT?  The attached patch actually returns nothing, in a method which is supposed to return nsresult, which is one of the things confusing me about the how-has-this-been-tested situation.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Also, has this been tested on <form method="POST" target="_blank">?  What's the expected behavior in that case?

Comment 15

[Wesley Johnston (:wesj)]

I realized that after I left last night too. Let me test again. The first time I tested this I didn't have the return in place and noticed that both the app and external handler opened the link. I thought I built after I added the return, but maybe I didn't...

I'll look at the form bit too. I think we basically want anything that uses target="_blank" (in an app) to use the externalHelperAppService.

Comment 16

[Boris Zbarsky [:bzbarsky]]

The key part with the form is the method="POST".  What should happen with the POST data?

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

So apart from the form case, this seems reasonable...

Comment 18

[Wesley Johnston (:wesj)]

Hmm... The form case is also broken in b2g (i.e. the url is sent to the browser app, where its requested as a get request). Looking into a fix. I don't think it makes sense to use the external window behaviour in this case. Looking into a fix.

Do you think this should block on that?

Comment 19

[Boris Zbarsky [:bzbarsky]]

I think as long as we understand the tradeoffs and are ok with them we don't have to block on it...

Comment 20

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Great. Sorry for the spam, but as you mentioned, I was missing a return value here originally. I think we need to return NS_ERROR_ABORT here to ensure that the caller doesn't create a window of their own. Wanted to make sure this is right. (Rebuilt and tested and everything looks fine though).

For the post-data case, looks like we basically lose any info about whether we have post data or not at:

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8664

We'll need to pass this up or move this check into docShell (I think the former is probably preferable so that all this logic is in one place).

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

Another question, actually.  Why the b2g ifdef?  The old code was unconditional and would still make sense in a child process on non-b2g, no?  Should this instead be a runtime check on the process type?

Comment 22

[Wesley Johnston (:wesj)]

(In reply to Boris Zbarsky (:bz) from comment #21)
> Another question, actually.  Why the b2g ifdef?  The old code was
> unconditional and would still make sense in a child process on non-b2g, no? 
> Should this instead be a runtime check on the process type?

TBH, I'm not sure we'd want this code to run on any platform but b2g. Maybe if you were porting the b2g browser? Based on the description [1] and implementation [2], it seems highly specialized for the case of "Gecko is the OS". Also, I'm not sure I see how what process we're running in should determine whether it runs or not.

Trying to work out a way to do both. The bigger problem seems to be that b2g assumes NOT inserting the iframe into the document is equivalent to saying "Don't open this" whereas for other platforms it would be nice to instead take that as a "let someone else handle this for me"... Maybe we could use the event's status for that instead. Call preventDefault if you dont want the system to handle this?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.h#38
[2] http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.cpp#187

Comment 23

[Wesley Johnston (:wesj)]

#ifdef'ing this causes some desktop test failures that are depending on this mozbrowseropenwindow event firing, so a solution that lets it fire, but lets us handle things too would be nice...

https://tbpl.mozilla.org/?tree=Try&rev=0307f4fcee97

Alternatively, looking at intercepting these events in our frontend...

Comment 24

[Boris Zbarsky [:bzbarsky]]

I'll be honest: I'm not exactly current on what cases we have of mozapp/browser frames not on b2g and what all BrowserElementParent is really trying to do....  It may be worth it to have someone review this who is.

Comment 25

[Wesley Johnston (:wesj)]

No problem. Lets see what justin thinks. Are we ok to ifdef this (temporarily maybe?) or would we rather do something a bit more complex like fire this code, and have better fallbacks if the platform we're running on doesn't want to insert iframes into its dom?

Comment 26

[Justin Lebar (not reading bugmail)]

> The B2G codepaths basically create an iframe and send it to the frontend via. an event (is there 
> some good reason they're not using our normal nsIBrowserDOMWindow here?)

In B2G, the parent of the <iframe mozbrowser> is content HTML.  So we can't use nsIBrowserDOMWindow or any of our existing interfaces, because you have to be able to do everything from content.

> Also, I'm not sure why they're creating the iframe in C++...

We have a byzantine dance wherein

 * child process requests calls window.open, makes a blocking call to the parent
 * child process creates a new TabChild to back the new window
 * parent creates the iframe backing the window.open in C++, but doesn't hook the iframe up to a remote process yet
 * parent lets the browser app (remember, vanilla content HTML) insert the frame into the DOM, or not, if it chooses
 * if the iframe was inserted into the DOM, the parent lets the iframe create a remote frameloader in the original child process, connected to the TabParent that the child created
 * the child process checks whether the window.open succeeded, and if it did, forces the creation of a new window in the TabChild and returns that window as the result of window.open. 

I'm not sure if that clarifies anything...

Comment 27

[Justin Lebar (not reading bugmail)]

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

This ifdef is unfortunately not OK.  I'd expect it to turn the tree orange, in fact.

We run tests on in-process <iframe mozbrowser> in desktop Firefox.  We've used in-process <iframe mozbrowser> in Firefox in the past; I'm not sure if they're still in place.

If you want to ifdef this, it's going to have to be an Android-only ifdef.  Better would be to use a pref, although I'm in any case not wild about hardcoding this in this way; using the <iframe mozbrowser> interface gives you complete control here, and it is (in theory) totally compatible with a chrome JS embedding.

Comment 28

[Justin Lebar (not reading bugmail)]

> If you want to ifdef this, it's going to have to be an Android-only ifdef.

An Android-only ifdef or a pref would essentially preclude us from using <iframe mozbrowser> on Android, which would be a disappointing position to write ourselves into.  Doubly so if you're trying to use this code on desktop.

I'd bet you can use the mozbrowseropenwindow event for whatever you're trying to accomplish here.

Comment 29

[Justin Lebar (not reading bugmail)]

>  * child process requests calls window.open, makes a blocking call to the parent
>  * child process creates a new TabChild to back the new window

I guess this really goes like

 * child process calls window.open
 * child process creates a new TabChild to back the new window
 * child process makes a blocking open-window call to the parent

Comment 30

[Wesley Johnston (:wesj)]

Requiring every platform/browser to implement their own handlers for this stuff seems like an easy way to fragment our own platform with tons of tiny quirky behaviour differences. I'd much rather a solution where this stuff "just works".

Comment 31

[Justin Lebar (not reading bugmail)]

> I'd much rather a solution where this stuff "just works".

I suppose you could check whether the frame is contained inside an <iframe mozbrowser>, instead of checking whether it's an app.  Then at least you have to opt in to the crazy alternative API.

Comment 32

[Justin Lebar (not reading bugmail)]

> I suppose you could check whether the frame is contained inside an <iframe mozbrowser>, instead of 
> checking whether it's an app.

Oh, except we're already doing that.

>  // If aParent is inside an <iframe mozbrowser> and this isn't a request to
>  // open a modal-type window, we're going to create a new <iframe mozbrowser>
>  // and return its window here.

I don't think you should get to halfway opt in to <iframe mozbrowser>.  That seems like asking for serious complication.  If you don't like <iframe mozbrowser>, don't use it.

Comment 33

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Justin Lebar [:jlebar] from comment #32)

> I don't think you should get to halfway opt in to <iframe mozbrowser>.  That
> seems like asking for serious complication.  If you don't like <iframe
> mozbrowser>, don't use it.

We're not using <iframe mozbrowser>, we are using <browser>. Since we want to support Mozilla Web Apps, we give the <browser> an app ID. This is causing some trouble because the code ties "apps" to "apps, the way b2g implements it".

Comment 34

[Justin Lebar (not reading bugmail)]

>   if (docshell && docshell->GetIsInBrowserOrApp() &&

Oh, I see.  Sorry, I should have read the code, not the comment.

I guess the right way to do this is to add a new nsDocShell frametype that means "has an app-id, but is not inside mozbrowser."  You can walk the frame hierarchy and stop once you hit chrome or an actual mozbrowser (nsIMozBrowserFrame::reallyIsBrowserOrApp).  If you don't hit chrome, you must be OOP, in which case we're a mozbrowser.

You're going to have to change other stuff, too.  nsGlobalWindow::Close(), maybe nsFrameLoader::SwapWithOtherLoader and nsDSURIContentListener::CheckOneFrameOptionsPolicy.  Probably other stuff that I'm not finding at the moment.

It might be easier to give in here.  It was cjones's overt intent to force consumers to switch to mozbrowser.

Comment 35

[Wesley Johnston (:wesj)]

That still doesn't really "fix" this problem, which requires our frontend to handle the logic of where to open links, which is what we were trying to avoid. (i.e. we're sick of having to reverse engineer b2g's frontend).

I noticed that isBrowserOrApp is used rather frequently as well. Need to do an audit...

Comment 36

[Justin Lebar (not reading bugmail)]

> That still doesn't really "fix" this problem, which requires our frontend to handle the logic of 
> where to open links, which is what we were trying to avoid. (i.e. we're sick of having to reverse 
> engineer b2g's frontend).

I said that it might be easier, not that it would fix the problem the way you'd like to fix it.  :)

Someone needs to decide where to open links.  Our options are either the front-end or the back-end.  I think a good argument for putting this logic in the back-end is that you want to share logic between multiple front-ends.  In contrast, I don't think "I don't like reverse-engineering b2g's front-end" is a good argument...  I sincerely apologize for the lack of documentation, but unfortunately B2G has been shipping next week for the past nine months, and writing documentation for this large, complex interface has fallen by the wayside.

It sounds to me like we have at least three different modalities for opening windows:

 * B2G, which uses mozbrowseropenwindow
 * Desktop Firefox, which opens a new tab, and
 * The app runtime, which wants to load the browser for some window.open's.

When I write it like this, it sounds like the question you want to ask is, "is this frame inside the app runtime?"

There's no reason, by the way, that each consumer of the browser-api must start from scratch.  If your main concern is that using mozbrowser would require you to duplicate implementations between e.g. desktop and mobile, I'm pretty confident we can share code where appropriate.

Comment 37

[Wesley Johnston (:wesj)]

Attachment: Patch v3

How about something like this? Basically the idea is that listeners must call preventDefault() on mozbrowseropenwindow events or else we'll continue and the let platform try to open the url. If the app didn't handle it and the target is blank, the external app service will then try to handle it before finally just sending it off to our normal window.open handling?

On Android, the mozbrowseropenwindow stuff fails (the events actually aren't even sent because topWindow->GetFrameElement bails at the chrome-content boundry), leading to the far path.

On B2G (and other platforms that want to support this) we just need to call event.preventDefault() (to be done in gaia land I guess?) if they're going to handle the event. If b2g eventually implemented the externalHelperServices (to use MozActivities?) they can drop their handling of _blank in apps as well... (that handling would have to move earlier in the isBrowserOrApp block, but I put it later here so that (hopefully?) tests will keep passing on desktop...).

Comment 38

[Justin Lebar (not reading bugmail)]

> How about something like this?

Now we're talking!  I can get behind this; it's pretty similar to something we're doing with context menus -- if Gaia doesn't preventDefault() on the contextmenu event, we will fire a click instead (bug 864382).

> (that handling would have to move earlier in the isBrowserOrApp block, but I put it later here so 
> that (hopefully?) tests will keep passing on desktop...).

I'd rather move it earlier; that seems like the sane way to do it.  I don't totally understand your concern about letting the tests pass on desktop.  As written, we'll try the URL handler service first, and do the mozbrowser stuff only if that fails; it seems like if we want to make the tests work on desktop, we should do the mozbrowser stuff first and then try the URL handler service second, right?

We'll want a test for this behavior, if that's possible.

A few other nits, keeping in mind that this was an f? and not an r?:

* >+      *aWindowIsNew = windowOpenState && BrowserElementParent::OPEN_WINDOW_ADDED;

Careful.  :)

* We don't use raw |long| as a datatype, where we can help it, because its size is different on different platforms.

* I think I'd prefer for OpenWindow to return an enum which can be one of:

    - Not preventDefault()'ed
    - preventDefault()'ed and iframe not inserted into the DOM
    - iframe inserted into DOM (maybe preventDefault()'ed, maybe not; doesn't matter)

It seems like these are the three cases we care about.

* >+  nsEventStatus aStatus;

We don't use the "a" prefix for local variables.

Comment 39

[Wesley Johnston (:wesj)]

Attachment: Patch

The desktop tests pass fine with this (and found some bugs for me). Pushing to try as well...

I've been digging around trying to see if there are ways to test the new behaviours too, but haven't found any good method for doing it yet.... still looking. Wanted to put the patch up in the meantime.

Comment 40

[Wesley Johnston (:wesj)]

https://tbpl.mozilla.org/?tree=Try&rev=27674e6b56e9

Comment 41

[Justin Lebar (not reading bugmail)]

Not totally done with this review, but my battery is about to die.

>diff --git a/dom/browser-element/BrowserElementParent.h b/dom/browser-element/BrowserElementParent.h
>--- a/dom/browser-element/BrowserElementParent.h
>+++ b/dom/browser-element/BrowserElementParent.h

>@@ -30,16 +30,32 @@ struct Size;
>  * functionality which must be written in C++.
>  *
>  * We don't communicate with the JS code that lives in BrowserElementParent.js;
>  * the JS and C++ parts are completely separate.
>  */
> class BrowserElementParent
> {
> public:
>+
>+  /**
>+   * Possible results from a window.open call.
>+   * ADDED - The frame was added to a document (i.e. handled by the receiver).

Please mention preventDefault here, since it's not obvious how that interacts
with this.

>+   * IGNORED - The frame was not added to a document and the receiver didn't
>+   *         call prevent default to prevent the platform from handling the open call.

s/prevent default/preventDefault()/

>+   * CANCELLED The frame was not added to a document, but the receiver still
>+   *         wants to prevent the platform from handling the load

Please mention preventDefault here, to contrast with IGNORED.

Please be consistent with the alignment and use of dashes.

>+  enum OPEN_WINDOW_RESULT {
>+    ADDED,
>+    IGNORED,
>+    CANCELLED,
>+  };

Style these days is to use StudlyCaps for enum name, then PREFIXED_UPPER_CASE
for enum values.  So something like

   enum OpenWindowResult {
     OPEN_WINDOW_ADDED,
     OPEN_WINDOW_IGNORED,
     OPEN_WINDOW_CANCELLED
   };

>@@ -58,39 +74,42 @@ public:
>    *    set aPopupTabParent's frame element to event.detail.frameElement.
>    *    Otherwise, we return false.
>    *
>    * @param aURL the URL the new window should load.  The empty string is
>    *             allowed.
>    * @param aOpenerTabParent the TabParent whose TabChild called window.open.
>    * @param aPopupTabParent the TabParent inside which the opened window will
>    *                        live.
>-   * @return true on success, false otherwise.  Failure is not (necessarily)
>-   *         an error; it may indicate that the embedder simply rejected the
>-   *         window.open request.
>+   * @return an OPEN_WINDOW_RESULT that describes whether the browser added the
>+   *         frame to a document or whether they called preventDefault to prevent

s/browser/embedder/ -- that's what we call the window which contains the
mozbrowser.  Also, I think you want s/they/it/ and s/or/and/.

>+   *         the platform from handling the open request
>    */
>-  static bool
>+  static OPEN_WINDOW_RESULT
>   OpenWindowOOP(dom::TabParent* aOpenerTabParent,
>                 dom::TabParent* aPopupTabParent,
>                 const nsAString& aURL,
>                 const nsAString& aName,
>                 const nsAString& aFeatures);
> 
>   /**
>    * Handle a window.open call from an in-process <iframe mozbrowser>.
>    *
>    * As with OpenWindowOOP, we return true if the window.open request
>    * succeeded, and return false if the embedder denied the request.
>    *
>    * (These parameter types are silly, but they match what our caller has in
>    * hand.  Feel free to add an override, if they are inconvenient to you.)
>    *
>    * @param aURI the URI the new window should load.  May be null.
>+   * @return an OPEN_WINDOW_RESULT that describes whether the browser added the
>+   *         frame to a document or whether they called preventDefault to prevent
>+   *         the platform from handling the open request

id.

>diff --git a/dom/browser-element/BrowserElementParent.cpp b/dom/browser-element/BrowserElementParent.cpp
>--- a/dom/browser-element/BrowserElementParent.cpp
>+++ b/dom/browser-element/BrowserElementParent.cpp

>@@ -89,106 +91,123 @@ DispatchCustomDOMEvent(Element* aFrameEl
> 
>   nsCOMPtr<nsIWritableVariant> detailVariant = new nsVariant();
>   nsresult rv = detailVariant->SetAsISupports(aDetailValue);
>   NS_ENSURE_SUCCESS(rv, false);
>   nsCOMPtr<nsIDOMCustomEvent> customEvent = do_QueryInterface(domEvent);
>   NS_ENSURE_TRUE(customEvent, false);
>   customEvent->InitCustomEvent(aEventName,
>                                /* bubbles = */ true,
>-                               /* cancelable = */ false,
>+                               /* cancelable = */ true,
>                                detailVariant);
>   customEvent->SetTrusted(true);
>   // Dispatch the event.
>   nsEventStatus status = nsEventStatus_eIgnore;
>   rv = nsEventDispatcher::DispatchDOMEvent(aFrameElement, nullptr,
>                                            domEvent, presContext, &status);
>+  aStatus = status;
>   return NS_SUCCEEDED(rv);
> }
> 
> /**
>  * Dispatch a mozbrowseropenwindow event to the given opener frame element.
>  * The "popup iframe" (event.detail.frameElement) will be |aPopupFrameElement|.
>  *
>  * Returns true iff there were no unexpected failures and the window.open call
>  * was accepted by the embedder.
>  */
>-bool
>+BrowserElementParent::OPEN_WINDOW_RESULT
> DispatchOpenWindowEvent(Element* aOpenerFrameElement,
>                         Element* aPopupFrameElement,
>                         const nsAString& aURL,
>                         const nsAString& aName,
>                         const nsAString& aFeatures)

Feel free to move this into BrowserElementParent, if you want.  Then the
scoping on the enum type would be less annoying...

>+  nsEventStatus status;
>   bool dispatchSucceeded =
>     DispatchCustomDOMEvent(aOpenerFrameElement,
>                            NS_LITERAL_STRING("mozbrowseropenwindow"),
>-                           detail);
>+                           detail, status);
> 
>-  // If the iframe is not in some document's DOM at this point, the embedder
>-  // has "blocked" the popup.
>-  return (dispatchSucceeded && aPopupFrameElement->IsInDoc());
>+  BrowserElementParent::OPEN_WINDOW_RESULT res =
>+      BrowserElementParent::OPEN_WINDOW_RESULT::IGNORED;

Enum values have the scope of their container, so we usually write this as

  BrowserElementParent::OPEN_WINDOW_IGNORED.

I kind of feel like what you've done is cleaner than this, but maybe it's
offset by the fact it's non-idiomatic and that we can't keep people from doing
BrowserElementParent::IGNORED (which makes no sense) without |enum class|,
which we don't have everywhere (and for which our workaround, in
mfbt/TypedEnum.h, is insufficient: It doesn't work with enums nested in a
class).

>+  if (dispatchSucceeded) {
>+
>+    if (aPopupFrameElement->IsInDoc()) {

Super-nit: No newline here (unless you're going to put one before the matching
close brace, which I don't think we need.

>+      res = BrowserElementParent::OPEN_WINDOW_RESULT::ADDED;

You can just have an early return here.

>+    } else if (status == nsEventStatus_eConsumeNoDefault) {
>+      // If the frame was not added to a document, report to callers whether
>+      // preventDefault was called on or not
>+      res = BrowserElementParent::OPEN_WINDOW_RESULT::CANCELLED;

And here.

>+    }
>+  }
>+
>+  return res;

And then this can be return IGNORED.

>-/*static*/ bool
>+/*static*/
>+BrowserElementParent::OPEN_WINDOW_RESULT
> BrowserElementParent::OpenWindowOOP(TabParent* aOpenerTabParent,
>                                     TabParent* aPopupTabParent,
>                                     const nsAString& aURL,
>                                     const nsAString& aName,
>                                     const nsAString& aFeatures)
> {
>   // Create an iframe owned by the same document which owns openerFrameElement.
>   nsCOMPtr<Element> openerFrameElement =
>     do_QueryInterface(aOpenerTabParent->GetOwnerElement());
>-  NS_ENSURE_TRUE(openerFrameElement, false);
>+  NS_ENSURE_TRUE(openerFrameElement,
>+                 BrowserElementParent::OPEN_WINDOW_RESULT::IGNORED);

Don't need to scope the enum as BrowserElementParent::, since we're in BEP.

>   nsRefPtr<HTMLIFrameElement> popupFrameElement =
>     CreateIframe(openerFrameElement, aName, /* aRemote = */ true);
> 
>   // Normally an <iframe> element will try to create a frameLoader when the
>   // page touches iframe.contentWindow or sets iframe.src.
>   //
>   // But in our case, we want to delay the creation of the frameLoader until
>   // we've verified that the popup has gone through successfully.  If the popup
>   // is "blocked" by the embedder, we don't want to load the popup's url.
>   //
>   // Therefore we call DisallowCreateFrameLoader() on the element and call
>   // AllowCreateFrameLoader() only after we've verified that the popup was
>   // allowed.
>   popupFrameElement->DisallowCreateFrameLoader();
> 
>-  bool dispatchSucceeded =
>+  BrowserElementParent::OPEN_WINDOW_RESULT opened =

id.

>@@ -200,50 +219,53 @@ BrowserElementParent::OpenWindowInProces
>+  OPEN_WINDOW_RESULT opened =
>     DispatchOpenWindowEvent(openerFrameElement, popupFrameElement,
>                             NS_ConvertUTF8toUTF16(spec),
>                             aName,
>                             NS_ConvertUTF8toUTF16(aFeatures));
>-  if (!dispatchSucceeded) {
>-    return false;
>+
>+  if (opened == BrowserElementParent::OPEN_WINDOW_RESULT::ADDED) {

id.

>+    // Return popupFrameElement's window.
>+    nsCOMPtr<nsIFrameLoader> frameLoader;
>+    popupFrameElement->GetFrameLoader(getter_AddRefs(frameLoader));
>+    NS_ENSURE_TRUE(frameLoader, BrowserElementParent::OPEN_WINDOW_RESULT::IGNORED);

id.

>+    nsCOMPtr<nsIDocShell> docshell;
>+    frameLoader->GetDocShell(getter_AddRefs(docshell));
>+    NS_ENSURE_TRUE(docshell, BrowserElementParent::OPEN_WINDOW_RESULT::IGNORED);

id.

>+    nsCOMPtr<nsIDOMWindow> window = do_GetInterface(docshell);
>+    window.forget(aReturnWindow);
>+  } else {
>+    return opened;

We prefer early returns to later returns and more nested code.  So please invert this:

  if (opened != ADDED) {
    return opened;
  }

  // no else
  ...

>-  // Return popupFrameElement's window.
>-  nsCOMPtr<nsIFrameLoader> frameLoader;
>-  popupFrameElement->GetFrameLoader(getter_AddRefs(frameLoader));
>-  NS_ENSURE_TRUE(frameLoader, false);
>-
>-  nsCOMPtr<nsIDocShell> docshell;
>-  frameLoader->GetDocShell(getter_AddRefs(docshell));
>-  NS_ENSURE_TRUE(docshell, false);
>-
>-  nsCOMPtr<nsIDOMWindow> window = do_GetInterface(docshell);
>-  window.forget(aReturnWindow);
>-  return !!*aReturnWindow;
>+  return !!*aReturnWindow ? opened : BrowserElementParent::OPEN_WINDOW_RESULT::IGNORED;

If we fail to create a window here (aReturnWindow is null), I think we should
return CANCLLED, not IGNORED.  We don't want accidental calls into the platform
to open windows.

Anyway we shouldn't ever hit this edge case, I think.

>@@ -264,19 +286,20 @@ NS_IMETHODIMP DispatchAsyncScrollEventRu
> {
>   nsIDOMElement* element = mTabParent->GetOwnerElement();
>   nsCOMPtr<Element> frameElement = do_QueryInterface(element);
>   // Create the event's detail object.
>   nsRefPtr<nsAsyncScrollEventDetail> detail =
>     new nsAsyncScrollEventDetail(mContentRect.x, mContentRect.y,
>                                  mContentRect.width, mContentRect.height,
>                                  mContentSize.width, mContentSize.height);
>+  nsEventStatus aStatus;

s/aStatus/status/.

>diff --git a/xpfe/appshell/src/nsContentTreeOwner.cpp b/xpfe/appshell/src/nsContentTreeOwner.cpp
>--- a/xpfe/appshell/src/nsContentTreeOwner.cpp
>+++ b/xpfe/appshell/src/nsContentTreeOwner.cpp

>@@ -817,24 +820,47 @@ nsContentTreeOwner::ProvideWindow(nsIDOM
>   // If aParent is inside an <iframe mozbrowser> and this isn't a request to
>   // open a modal-type window, we're going to create a new <iframe mozbrowser>
>   // and return its window here.
>   nsCOMPtr<nsIDocShell> docshell = do_GetInterface(aParent);
>   if (docshell && docshell->GetIsInBrowserOrApp() &&
>       !(aChromeFlags & (nsIWebBrowserChrome::CHROME_MODAL |
>                         nsIWebBrowserChrome::CHROME_OPENAS_DIALOG |
>                         nsIWebBrowserChrome::CHROME_OPENAS_CHROME))) {
>-    *aWindowIsNew =
>+
>+    BrowserElementParent::OPEN_WINDOW_RESULT opened =
>       BrowserElementParent::OpenWindowInProcess(aParent, aURI, aName,
>                                                 aFeatures, aReturn);
> 
>-    // If OpenWindowInProcess failed (perhaps because the embedder blocked the
>+    // If OpenWindowInProcess handled the open (by opening it or blocking the
>     // popup), tell our caller not to proceed trying to create a new window
>     // through other means.
>-    return *aWindowIsNew ? NS_OK : NS_ERROR_ABORT;
>+    if (opened != BrowserElementParent::OPEN_WINDOW_RESULT::IGNORED) {
>+      return opened == BrowserElementParent::OPEN_WINDOW_RESULT::ADDED ?
>+              NS_OK : NS_ERROR_ABORT;
>+    }
>+
>+    // If we're in an app and the target is _blank, send the url to the OS

s/an app/a browser or an app/ -- see the if statement above.

>+    if (aName.LowerCaseEqualsLiteral("_blank")) {
>+      nsCOMPtr<nsIExternalURLHandlerService> exUrlServ(
>+                        do_GetService(NS_EXTERNALURLHANDLERSERVICE_CONTRACTID));
>+      if (exUrlServ) {
>+
>+        nsCOMPtr<nsIHandlerInfo> info;
>+        bool found;
>+        exUrlServ->GetURLHandlerInfoFromOS(aURI, &found, getter_AddRefs(info));
>+  
>+        if (info && found) {
>+          info->LaunchWithURI(aURI, nullptr);
>+          return NS_ERROR_ABORT;
>+        }
>+
>+      }
>+    }
>+
>   }
> 
>   // the parent window is fullscreen mode or not.
>   bool isFullScreen = false;
>   if (aParent) {
>     aParent->GetFullScreen(&isFullScreen);
>   }
>

Comment 42

[Justin Lebar (not reading bugmail)]

>   nsEventStatus status = nsEventStatus_eIgnore;
>   rv = nsEventDispatcher::DispatchDOMEvent(aFrameElement, nullptr,
>                                            domEvent, presContext, &status);
>+  aStatus = status;
>   return NS_SUCCEEDED(rv);

Can we just use aStatus directly instead of introducing a new variable?

>diff --git a/xpfe/appshell/src/nsContentTreeOwner.cpp b/xpfe/appshell/src/nsContentTreeOwner.cpp
>--- a/xpfe/appshell/src/nsContentTreeOwner.cpp
>+++ b/xpfe/appshell/src/nsContentTreeOwner.cpp

>@@ -817,24 +820,47 @@ nsContentTreeOwner::ProvideWindow(nsIDOM
>   // If aParent is inside an <iframe mozbrowser> and this isn't a request to
>   // open a modal-type window, we're going to create a new <iframe mozbrowser>
>   // and return its window here.
>   nsCOMPtr<nsIDocShell> docshell = do_GetInterface(aParent);
>   if (docshell && docshell->GetIsInBrowserOrApp() &&
>       !(aChromeFlags & (nsIWebBrowserChrome::CHROME_MODAL |
>                         nsIWebBrowserChrome::CHROME_OPENAS_DIALOG |
>                         nsIWebBrowserChrome::CHROME_OPENAS_CHROME))) {
>-    *aWindowIsNew =
>+
>+    BrowserElementParent::OPEN_WINDOW_RESULT opened =
>       BrowserElementParent::OpenWindowInProcess(aParent, aURI, aName,
>                                                 aFeatures, aReturn);
> 
>-    // If OpenWindowInProcess failed (perhaps because the embedder blocked the
>+    // If OpenWindowInProcess handled the open (by opening it or blocking the
>     // popup), tell our caller not to proceed trying to create a new window

Nit: s/it //

>     // through other means.
>-    return *aWindowIsNew ? NS_OK : NS_ERROR_ABORT;
>+    if (opened != BrowserElementParent::OPEN_WINDOW_RESULT::IGNORED) {
>+      return opened == BrowserElementParent::OPEN_WINDOW_RESULT::ADDED ?

Enum scoping (though I wish we could do it this way, which is much better).

>+              NS_OK : NS_ERROR_ABORT;
>+    }
>+
>+    // If we're in an app and the target is _blank, send the url to the OS
>+    if (aName.LowerCaseEqualsLiteral("_blank")) {

I think you need to check that we're actually in an app; the if statement above
is checking browser-or-app.

>+      nsCOMPtr<nsIExternalURLHandlerService> exUrlServ(
>+                        do_GetService(NS_EXTERNALURLHANDLERSERVICE_CONTRACTID));

You can just indent this two spaces.  You don't even have to feel bad about
weird indenting if you use nsCOMPtr<nsIFoo> foo = bar instead of
nsCOMPtr<nsIFoo> foo(bar).

>+      if (exUrlServ) {
>+
>+        nsCOMPtr<nsIHandlerInfo> info;
>+        bool found;
>+        exUrlServ->GetURLHandlerInfoFromOS(aURI, &found, getter_AddRefs(info));
>+  
>+        if (info && found) {
>+          info->LaunchWithURI(aURI, nullptr);
>+          return NS_ERROR_ABORT;
>+        }

Maybe you could test this by mocking up a new handler?

Comment 43

[Justin Lebar (not reading bugmail)]

Comment on attachment 750184 [details] [diff] [review]
Patch

This looks good, but I'd like to have another look.

And of course we need tests.

Comment 44

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Sorry for the delay. Got distracted with some other things. Sending this with some new tests (coming in a second patch) to try, but uploading now (i.e. you may want to wait a bit before reviewing to see how that goes...)

Comment 45

[Wesley Johnston (:wesj)]

Attachment: Tests

Tests

Comment 46

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Grrr. Forgot to qref.

Comment 47

[Wesley Johnston (:wesj)]

and the try push so I don't lose it:

https://tbpl.mozilla.org/?tree=Try&rev=89fc1c17ea3f

Comment 48

[Wesley Johnston (:wesj)]

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

I think I've made try even madder....

Comment 49

[Jen Fong-Adwent [:ednapiranha]]

Any updates on this? Installed apps on Android from Firefox Beta using Persona don't work correctly :)

Comment 50

[Wesley Johnston (:wesj)]

Sorry. This broke a bunch of tests that I wasn't able to reproduce locally. Still looking into it.

Comment 53

[Wesley Johnston (:wesj)]

Attachment: Patch

Sorry for the insane delay here. I hit some test failures I couldn't reproduce easily, and this fell off my radar. Justin, I have no idea if you're still doing reviews or not, but if you are please look. Otherwise, I added bz as well since he's reviewed almost every other patch in here :)

Comment 54

[Wesley Johnston (:wesj)]

Attachment: Fixup tests

This is just fixing up tests that would now fail because they assumed the window would not be opened by the OS.

Comment 55

[Wesley Johnston (:wesj)]

Attachment: WIP Test

This is a test for this feature. It works and passes, but causes some failures in other tests, I'm guessing because the temporary component registration/unregistration is not quite right.

https://tbpl.mozilla.org/?tree=Try&rev=b7636fb66dc3

To get them to reproduce locally I think I have to run the entire mochitest-chrome locally, which always runs into other problems and dies :) I'll keep pinging at it. Also need to fixup gaia before this lands. Disabling just it made my try runs green again though:

https://tbpl.mozilla.org/?tree=Try&rev=4ffcbfdec2fc

Comment 56

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 801032 [details] [diff] [review]
Patch

Yes, this is way nicer!

>+                       nsEventStatus &aStatus)

I'd prefer this to be an nsEventStatus* to match our other event-dispatch code.

>+BrowserElementParent::DispatchOpenWindowEvent(Element* aOpenerFrameElement,
>                         Element* aPopupFrameElement,
>                         const nsAString& aURL,
>                         const nsAString& aName,
>                         const nsAString& aFeatures)

Fix the indent?

r=me with the nits.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 801033 [details] [diff] [review]
Fixup tests

r=me

Comment 58

[Wesley Johnston (:wesj)]

Attachment: Gaia Patch

Dale, I saw your name on some of this work, but if you know a better reviewer feel free to move this.

With these patches, Gecko is altered so that if you don't call preventDefault on these events, they'll fall through and be sent to Gecko's normal window.open code paths:

http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsContentTreeOwner.cpp#845

To maintain its old behavior, Gaia should just always call preventDefault().

https://github.com/mozilla-b2g/gaia/pull/12007

Comment 59

[Dale Harvey (:daleharvey)]

Comment on attachment 801205 [details] [diff] [review]
Gaia Patch

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

This looks good, thanks for the fix

Comment 60

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd53e981282f

This is blocking some other things. Landed right now, will try to get the tests done this week.

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bd53e981282f