Closed Bug 1116478 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Content Processes, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
e10s m8+ ---
firefox45 --- fixed

People

(Reporter: ehsan, Assigned: mrbkap)

References

(Blocks 1 open bug, Regressed 1 open bug, )

Details

Attachments

(1 file, 1 obsolete file)

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=test@example.com> which is expected, and I also get a tab with a blank title opened and when I activate it, the location bar reads "mailto:test@example.com".
Blocks: 646552
Assignee: nobody → jimmyw22
Assignee: jimmyw22 → mrbkap
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:test@example.com" 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.
Assignee: mrbkap → blassey.bugs
Assignee: blassey.bugs → mrbkap
Attached patch Patch v1 (obsolete) — Splinter Review
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).
Attachment #8694532 - Flags: review?(wmccloskey)
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.
Attached patch Patch v2Splinter Review
Sure, I was debating between the two.
Attachment #8695153 - Flags: review?(wmccloskey)
Attachment #8694532 - Attachment is obsolete: true
Attachment #8694532 - Flags: review?(wmccloskey)
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?
Attachment #8695153 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/ed645e64d666
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Component: Tabbed Browser → DOM: Content Processes
Product: Firefox → Core
Target Milestone: Firefox 45 → ---
Depends on: 1253307
See Also: → 1554530
Regressions: 1554530
See Also: 1554530
You need to log in before you can comment on or make changes to this bug.