1116478 - Cmd+clicking a mailto link to open it in a new tab with gmail handling mailto links will leave an extra empty tab behind in e10s

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[(no longer active)]

STR:

1. Associate gmail with mailto: links.
2. Open the page in the URL field in a new tab.
3. Cmd+Click "test".

I get a tab opened to <https://mail.google.com/mail/u/0/?view=cm&fs=1&tf=1&source=mailto&to=[e-mail]> which is expected, and I also get a tab with a blank title opened and when I activate it, the location bar reads "mailto:[e-mail]".

Comment 1

[Jimmy Wang (:jimicy) - works on e10s stuff]

These are the code paths if you do right click context menu and select open in a new tab.
Command + click also follows this. Both call into openLinkIn.
================= e10s code path =================

##call stack (begin to end)
oncommand                                        [browser.xul:1]
nsContentMenu.prototype.openLinkInTab            [nsContextMenu.js:919]
openLinkIn                                       [utilityOverlay.js:345]
loadOneTab                                       [tabbrowser.xml:1417]
addTab                                           [tabbrowser.xml:1882]
loadURIWithFlags                                 [tabbrowser.xml:6190 "tabbrowser-remote-browser"]
_loadURIWithFlags                                [browser.js:12240]
RemoteWebNavigation.prototype.loadURIWithOptions [RemoteWebNavigation.jsm:82]
WebNavigation:LoadURI                            [browser-child.js:]
    + this.webNavigation.loadURIWithOptions(...). Goes through c++ code to call launchWithURI

Where in C++ calls launchWithURI?

nsExternalHelperAppService.cpp
> nsExternalHelperAppService::LoadURI(nsIURI *aURI, nsIInterfaceRequestor *aWindowContext)
The code sends only the uri if it is e10s. The aWindowContext sent is null.
> nsExternalHelperAppService::LoadURI(nsIURI *aURI,
>                                     nsIInterfaceRequestor *aWindowContext)
> {
>   NS_ENSURE_ARG_POINTER(aURI);
> 
>   if (XRE_IsContentProcess()) {
>     URIParams uri;
>     SerializeURI(aURI, uri);
> 
>     mozilla::dom::ContentChild::GetSingleton()->SendLoadURIExternal(uri);
>     return NS_OK;
>   }
> ...
>   if (!alwaysAsk && (preferredAction == nsIHandlerInfo::useHelperApp ||
>                      preferredAction == nsIHandlerInfo::useSystemDefault))
>     return handler->LaunchWithURI(uri, aWindowContext);
> }

launchWithURI: function nWHA__launchWithURI(aURI, aWindowContext) [nsWebHandlerApp.js:132]
    + this code hasn't been touched since 2007. The current event handling it does actually isn't actually right for e10s.
    + nsIBrowserDOMWindow.OPEN_NEW opens a new window with the mailto gmail link. this is why addTab gets called twice and why we get a blank tab with "mailto:[e-mail]" and then a tab with the gmail link. It doesn't open the mailto gmail link in the current tab that was opened.
>    // openURI
>    browserDOMWin.openURI(uriToSend,
>                          null, // no window.opener
>                          Ci.nsIBrowserDOMWindow.OPEN_DEFAULTWINDOW,
>                          Ci.nsIBrowserDOMWindow.OPEN_NEW);

This code path is not what we want. We need to send aWindowContext so that it will go into the if (aWindowContext) block.
> // if we have a window context, use the URI loader to load there
> if (aWindowContext) {
> 
>   // create a channel from this URI
>   var channel = ioService.newChannelFromURI2(uriToSend,
>                                              null,      // aLoadingNode
>                                              Services.scriptSecurityManager.getSystemPrincipal(),
>                                              null,      // aTriggeringPrincipal
>                                              Ci.nsILoadInfo.SEC_NORMAL,
>                                              Ci.nsIContentPolicy.TYPE_OTHER);
>   channel.loadFlags = Ci.nsIChannel.LOAD_DOCUMENT_URI;
> 
>   // load the channel
>   var uriLoader = Cc["@mozilla.org/uriloader;1"].
>                   getService(Ci.nsIURILoader);
>   // XXX ideally, whether to pass the IS_CONTENT_PREFERRED flag should be
>   // passed in from above.  Practically, the flag is probably a reasonable
>   // default since browsers don't care much, and link click is likely to be
>   // the more interesting case for non-browser apps.  See 
>   // <https://bugzilla.mozilla.org/show_bug.cgi?id=392957#c9> for details.
>   uriLoader.openURI(channel, Ci.nsIURILoader.IS_CONTENT_PREFERRED,
>                     aWindowContext);
>   return;
> }

The only problem is
> uriLoader.openURI(channel, Ci.nsIURILoader.IS_CONTENT_PREFERRED, aWindowContext);
https://dxr.mozilla.org/mozilla-central/source/uriloader/base/nsURILoader.cpp#546

This expects aWindowContext to be a docshell.
m_originalContext is passed the value of aWindowContext
the code in uriLoader does various checks on security on the docshell object. so it doesn't seem to be easy to replace.

##second call to open a gmail link tab
nsBrowserAccess.prototype.openURI [browser.js:16073]
nsBrowserAcess.prototype._openURIInNewTab [browser.js:16000]
loadOneTab [tabbrowser.xml:1417]
addTab [tabbrowser.xml:1725]

In order to fix this. We need to send back aWindowContext or do something similar to nsWebHandlerApp.js so that it can load the gmail link in the tab that is opened by addTab instead of opening a new tab with the gmail link.
possibily look into TabChild.h as an alternative.

================= non-e10s code path =================
##call stack (begin to end)
oncommand                                        [browser.xul:1]
nsContentMenu.prototype.openLinkInTab            [nsContextMenu.js:919]
openLinkIn                                       [utilityOverlay.js:345]
loadOneTab                                       [tabbrowser.xml:1417]
addTab                                           [tabbrowser.xml:1882]
loadURIWithFlags                                 [tabbrowser.xml:6151 "tabbrowser-browser"]
_loadURIWithFlags                                [browser.js:12240]
    + this.webNavigation.loadURIWithOptions(...). Goes through c++ code to call launchWithURI

nsExternalHelperAppService.cpp
> nsExternalHelperAppService::LoadURI(nsIURI *aURI, nsIInterfaceRequestor *aWindowContext)
It sends back aWindowContext and uri.

launchWithURI: function nWHA__launchWithURI(aURI, aWindowContext) [nsWebHandlerApp.js:101]
    + uriLoader.openURI(channel, Ci.nsIURILoader.IS_CONTENT_PREFERRED, aWindowContext);
    + This opens mailto gmail link in the current opened tab.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1

Comment 1 is right on here. The nsIURILoader interface is a little weird: it basically takes a docshell to target its load. In e10s, if we start in the content process, we can't pass a docshell which is why we passed null for a while. This patch gives us a way to communicate to a parent-process nsIURILoader that it needs to ask a content process to load in the right tab (or browser really).

Comment 3

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

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

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

::: uriloader/base/nsURILoader.cpp
@@ +815,5 @@
> +  if (remote) {
> +    nsCOMPtr<nsIURI> uri;
> +    channel->GetURI(getter_AddRefs(uri));
> +
> +    remote->OpenURI(uri, aFlags);

I'm not sure, but might it be better to put this code in nsWebHandlerApp.js instead? This seems like a pretty generic path to be putting stuff in.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v2

Sure, I was debating between the two.

Comment 6

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

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

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

::: uriloader/base/nsURILoader.cpp
@@ +19,5 @@
>  #include "nsIChannel.h"
>  #include "nsIInterfaceRequestor.h"
>  #include "nsIInterfaceRequestorUtils.h"
>  #include "nsIProgressEventSink.h"
> +#include "nsIRemoteWindowContext.h"

No longer needed.

::: uriloader/exthandler/nsWebHandlerApp.js
@@ +87,5 @@
> +        // request to the correct process.
> +        aWindowContext.getInterface(Ci.nsIRemoteWindowContext)
> +                      .openURI(uriToSend, Ci.nsIURILoader.IS_CONTENT_PREFERRED);
> +        return;
> +      } catch (e if e.result == Cr.NS_NOINTERFACE) {

Let's not use this since it's not ES6. Can you instead re-throw if it doesn't match?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed645e64d66616906fdfd0a652894ed0f4f5ea5e
Bug 1116478 - Open web content handlers in the proper tab in e10s. r=billm

Comment 8

[Carsten Book [:Tomcat]]

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