961867 - [e10s] Clicking on a link in an app tab should open it on the left side of the tabs bar

Status: VERIFIED FIXED

(Firefox :: General, defect, P3)

Description

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

For this to work, we'll need to change how isAppTab is set in tabbrowser.xml (currently it's disabled) and also ensure that nsIWebBrowserChrome3::OnBeforeLinkTraversal is called correctly somewhere. I'm not sure what the best way is to implement this, or whether it's best for isAppTab to live in the parent, the child, or both.

Comment 2

[Dave Townsend [:mossop]]

Ended up tinkering around in code related to this anyway so might as well take this.

Comment 3

[Dave Townsend [:mossop]]

Attachment: WIP

This patch works but I'm not sure the architecture is right and I'd appreciate some feedback.

We need something like nsContentTreeOwner's XULBrowserWindow but for content processes. I added a webBrowserChrome property to TabChild but with some tabs being remote and some not that leads to onBeforeLinkTraversal sometimes being called through TabChild and sometimes through nsContentTreeOwner. To avoid code duplication I added a way for the in-process frame script to register its WebBrowserChrome with the main XULBrowserWindow so it can pass off calls to the right object. It works but it seems a little hacky.

Another option might be to just have nsDocShell call onBeforeLinkTraversal on something else before going to the treeOwner. Something at the level of sameTypeRootTreeItem would make sense, I could add the webBrowserChrome property to the docshell there but I'm guessing you folks might have a better idea for me.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I would really rather not add more possible codepaths for this stuff on the docshell level.  Things are already complicated enough with the whole treeowner/windowprovider mess.

Comment 5

[Dave Townsend [:mossop]]

I guess the easier option to remove code duplication would be to move the onBeforeLinkTraversal code into a module that both browser.js and content.js import

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8478477 [details] [diff] [review]
WIP

A cleaner patch is on the way here.

Comment 7

[Dave Townsend [:mossop]]

Attachment: TabChild patch

This patch allows something in a frame script to register a webBrowserChrome object with TabChild that it can pass calls to onBeforeLinkTraversal to. This is similar to how we register in the chrome side with XULBrowserWindow.

Comment 8

[Dave Townsend [:mossop]]

Attachment: Frontend patch

This passes the isAppTab setting through to the frame script and responds to onBeforeLinkTraversal as necessary. We can enable a bunch of tests in e10s mode with this :)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8480780 [details] [diff] [review]
TabChild patch

> [scriptable, uuid(2eb3bc54-78bf-40f2-b301-a5b5b70f7da0)]
> interface nsITabChild : nsISupports
> {
>   readonly attribute nsIContentFrameMessageManager messageManager;
> 
>+  attribute nsIInterfaceRequestor webBrowserChrome;
>+
Why nsIInterfaceRequestor? Why don't you just use nsIWebBrowserChrome3?

> TabChild::GetInterface(const nsIID & aIID, void **aSink)
> {
>+    if (aIID.Equals(NS_GET_IID(nsIWebBrowserChrome3))) {
>+        if (mWBC)
>+            return mWBC->GetInterface(aIID, aSink);
>+        return NS_ERROR_NO_INTERFACE;
>+    }

2 spaces for indentation (I know the file isn't quite consistent), and
use {} always with if.

>+    nsCOMPtr<nsIInterfaceRequestor> mWBC;
I'm rather surprised if this doesn't cause leaks.
Rename the member variable to mWebBrowserChrome3 or some such and move it to
TabChildBase and add it to cycle collection.

(And please test on a debug build with XPCOM_MEM_LEAK_LOG=1 env variable to see you don't leak anything.)

Comment 11

[Dave Townsend [:mossop]]

(In reply to Olli Pettay [:smaug] from comment #10)
> Comment on attachment 8480780 [details] [diff] [review]
> TabChild patch
> 
> > [scriptable, uuid(2eb3bc54-78bf-40f2-b301-a5b5b70f7da0)]
> > interface nsITabChild : nsISupports
> > {
> >   readonly attribute nsIContentFrameMessageManager messageManager;
> > 
> >+  attribute nsIInterfaceRequestor webBrowserChrome;
> >+
> Why nsIInterfaceRequestor? Why don't you just use nsIWebBrowserChrome3?

Sorry I failed to explain my reasoning there. So over in bug 999239 I need to catch attempts to load URLs in remote tabs that shouldn't be remote. My proposal for doing so is to add a hook into docshell that let JS cancel attempts to load pages and then redirect them to the right process. I'm doing this through the same mechanism as here, XULBrowserWindow in the main process and mWebBrowserChrome in child processes.

> >+    nsCOMPtr<nsIInterfaceRequestor> mWBC;
> I'm rather surprised if this doesn't cause leaks.
> Rename the member variable to mWebBrowserChrome3 or some such and move it to
> TabChildBase and add it to cycle collection.
> 
> (And please test on a debug build with XPCOM_MEM_LEAK_LOG=1 env variable to
> see you don't leak anything.)

Attempting to figure this out, but running in debug with this doesn't show any leaks. Not sure why.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Dave Townsend [:mossop] from comment #11)

> > Why nsIInterfaceRequestor? Why don't you just use nsIWebBrowserChrome3?
> 
> Sorry I failed to explain my reasoning there. So over in bug 999239 I need
> to catch attempts to load URLs in remote tabs that shouldn't be remote. My
> proposal for doing so is to add a hook into docshell that let JS cancel
> attempts to load pages and then redirect them to the right process. I'm
> doing this through the same mechanism as here, XULBrowserWindow in the main
> process and mWebBrowserChrome in child processes.
That doesn't still answer to my question ;) Why you use type nsIInterfaceRequestor and not 
nsIWebBrowserChrome3?

> Attempting to figure this out, but running in debug with this doesn't show
> any leaks. Not sure why.
Well, add the member variables to CC anyway.

Comment 13

[Dave Townsend [:mossop]]

(In reply to Olli Pettay [:smaug] from comment #12)
> (In reply to Dave Townsend [:mossop] from comment #11)
> 
> > > Why nsIInterfaceRequestor? Why don't you just use nsIWebBrowserChrome3?
> > 
> > Sorry I failed to explain my reasoning there. So over in bug 999239 I need
> > to catch attempts to load URLs in remote tabs that shouldn't be remote. My
> > proposal for doing so is to add a hook into docshell that let JS cancel
> > attempts to load pages and then redirect them to the right process. I'm
> > doing this through the same mechanism as here, XULBrowserWindow in the main
> > process and mWebBrowserChrome in child processes.
> That doesn't still answer to my question ;) Why you use type
> nsIInterfaceRequestor and not 
> nsIWebBrowserChrome3?

Because I added the new function to a nsIWebBrowserChrome4 since that seems to have been the way to do things in the past so the object needed to have the option to implement both though. But maybe I should just add the function to nsIWebBrowserChrome3?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Even nsIWebBrowserChrome3 is rather bad.
We should just merge all nsIWebBrowserChromeX, and then let scripts to provide some functionality.
But ok for now, nsIWebBrowserChrome3 is fine, and if you want to add more functionality there, 
just add it.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

It is just seeing nsIInterfaceRequestor as a property type which is really odd.

Comment 16

[Dave Townsend [:mossop]]

Landed with the requested changes: https://hg.mozilla.org/integration/fx-team/rev/52b4f9600a14

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52b4f9600a14

Comment 18

[Dave Townsend [:mossop]]

Backed out for bug 1062522

https://hg.mozilla.org/integration/fx-team/rev/1917953b1f89

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

I.. um.. put it back in:

https://hg.mozilla.org/integration/fx-team/rev/8703c1895505

due to permanent bc-1 failures:

https://tbpl.mozilla.org/php/getParsedLog.php?id=47418182&tree=Fx-Team

495 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js | Waited too long for click-to-play notification -
496 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js | Should have a click-to-play notification in the tab in the new window -
498 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js | Waited too long for click-to-play notification -
499 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js | Should have a click-to-play notification in the initial tab again -
505 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js | Waited too long for click-to-play notification -
...

Comment 20

[Dave Townsend [:mossop]]

This was re-landed.

Comment 21

[Chris AtLee [:catlee]]

This seems to have regressed recently.

Comment 22

[Dave Townsend [:mossop]]

(In reply to Chris AtLee [:catlee] from comment #21)
> This seems to have regressed recently.

Please file a new bug.

Comment 23

[Chris AtLee [:catlee]]

filed bug 1080055

Comment 24

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue on a Nightly build before and after it regressed, verified on Windows 7 64bit, Ubuntu 14.04 64bit and Mac OS X 10.9.5 using latest Nightly.