1058116 - [e10s] New tabs opened from links in existing tabs should appear immediately to the right of the current tab

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Dave Townsend [:mossop]]

If a link in a page opens a new tab using target="_blank" the tab should appear immediately to the right of the current tab, instead it appears at the end of the tab strip.

Comment 1

[caralu1974]

Yes it happens as well on Linux and is a little disgusting.

Application Basics
------------------

Name: Firefox
Version: 35.0a1
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:35.0) Gecko/20100101 Firefox/35.0
Multiprocess Windows: 0/1

Crash Reports for the Last 3 Days
---------------------------------

All Crash Reports

Extensions
----------

Name: Adblock Plus
Version: 2.6.4
Enabled: true
ID: {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}

Name: Add-on Compatibility Reporter
Version: 2.0.4
Enabled: true
ID: [e-mail]

Name: Diccionario de Español/México
Version: 1.1.3
Enabled: true
ID: [e-mail]

Name: Disconnect
Version: 3.14.0
Enabled: true
ID: [e-mail]

Name: DownloadHelper
Version: 4.9.23
Enabled: true
ID: {b9db16a4-6edc-47ec-a1f4-b86292ed211d}

Name: DownThemAll!
Version: 2.0.17
Enabled: true
ID: {DDC359D1-844A-42a7-9AA1-88A850A938A8}

Name: gTranslate
Version: 0.9
Enabled: true
ID: {aff87fa2-a58e-4edd-b852-0a20203c1e17}

Name: HTTPS-Everywhere
Version: 5.0development.0
Enabled: true
ID: [e-mail]

Name: MEGA
Version: 2.0.186
Enabled: true
ID: [e-mail]

Name: Mozilla Archive Format
Version: 3.0.2
Enabled: true
ID: {7f57cf46-4467-4c2d-adfa-0cba7c507e54}

Name: Privacy Badger Firefox
Version: 0.1.4
Enabled: true
ID: jid1-MnnxcxisBPnSXQ@jetpack

Name: Spanish (Venezuela) spell check dictionary
Version: 1.1.17
Enabled: true
ID: [e-mail]

Name: Speed Dial
Version: 0.9.6.16
Enabled: true
ID: {64161300-e22b-11db-8314-0800200c9a66}

Name: YouTube ALL HTML5
Version: 2.1.3
Enabled: true
ID: jid1-qj0w91o64N7Eeg@jetpack

Name: Classic Theme Restorer
Version: 1.2.3
Enabled: false
ID: ClassicThemeRestorer@ArisT2Noia4dev

Name: Hide My Ass Proxy Extension
Version: 1.2.7
Enabled: false
ID: [e-mail]

Name: Nightly Tester Tools
Version: 3.7
Enabled: false
ID: {8620c15f-30dc-4dba-a131-7c5d20cf4a29}

Name: Oxygen KDE Options
Version: 3.7
Enabled: false
ID: {c2a3f51e-2920-4eab-9008-1bcb44d21d57}

Name: Personal Menu
Version: 6.2.0
Enabled: false
ID: [e-mail]

Name: QuickJava
Version: 2.0.4
Enabled: false
ID: {E6C1199F-E687-42da-8C24-E7770CC3AE66}

Graphics
--------

Adapter Description: nouveau -- Gallium 0.4 on NVA8
Device ID: Gallium 0.4 on NVA8
Driver Version: 3.0 Mesa 10.2.6
GPU Accelerated Windows: 0/1 Basic
Vendor ID: nouveau
WebGL Renderer: nouveau -- Gallium 0.4 on NVA8
windowLayerManagerRemote: false
AzureCanvasBackend: cairo
AzureContentBackend: cairo
AzureFallbackCanvasBackend: none
AzureSkiaAccelerated: 0

Important Modified Preferences
------------------------------

accessibility.typeaheadfind: true
accessibility.typeaheadfind.flashBar: 0
browser.cache.disk.capacity: 348160
browser.cache.disk.smart_size_cached_value: 358400
browser.cache.disk.smart_size.first_run: false
browser.cache.disk.smart_size.use_old_max: false
browser.cache.frecency_experiment: 2
browser.places.importBookmarksHTML: false
browser.places.smartBookmarksVersion: 7
browser.sessionstore.upgradeBackup.latestBuildID: 20140912030202
browser.startup.homepage: https://encrypted.google.com/
browser.startup.homepage_override.buildID: 20140912030202
browser.startup.homepage_override.mstone: 35.0a1
dom.mozApps.used: true
extensions.lastAppVersion: 35.0a1
media.gmp-gmpopenh264.lastUpdate: 1405959171
media.gmp-gmpopenh264.path: /home/caralu74/.mozilla/firefox/1gj32y6s.default/gmp-gmpopenh264
media.gmp-gmpopenh264.version: 1.0
media.gmp-manager.lastCheck: 1410555690
network.cookie.cookieBehavior: 1
network.cookie.prefsMigrated: true
places.database.lastMaintenance: 1410556595
places.history.expiration.transient_current_max_pages: 40801
plugin.disable_full_page_plugin_for_types: application/pdf
plugin.importedState: true
privacy.cpd.extensions-dta: true
privacy.cpd.offlineApps: true
privacy.cpd.siteSettings: true
privacy.donottrackheader.enabled: true
privacy.sanitize.migrateFx3Prefs: true
storage.vacuum.last.index: 1
storage.vacuum.last.places.sqlite: 1409586159

Important Locked Preferences
----------------------------

JavaScript
----------

Incremental GC: true

Accessibility
-------------

Activated: false
Prevent Accessibility: 0

Library Versions
----------------

NSPR
Expected minimum version: 4.10.7
Version in use: 4.10.7

NSS
Expected minimum version: 3.17.1 Basic ECC Beta
Version in use: 3.17.1 Basic ECC Beta

NSSSMIME
Expected minimum version: 3.17.1 Basic ECC Beta
Version in use: 3.17.1 Basic ECC Beta

NSSSSL
Expected minimum version: 3.17.1 Basic ECC Beta
Version in use: 3.17.1 Basic ECC Beta

NSSUTIL
Expected minimum version: 3.17.1 Beta
Version in use: 3.17.1 Beta

Experimental Features
---------------------

Comment 3

[Steven MacLeod [:smacleod]]

Talked to mconley in irc and I'm going to take this bug for the iteration.

Comment 4

[Tomasz Kołodziejski [:tomasz]]

I'll investigate this bug. Just for reference, mconley pointed me to http://hg.mozilla.org/mozilla-central/file/a255a234946e/browser/base/content/browser.js#l4303 as a starting point.

Comment 5

[Tomasz Kołodziejski [:tomasz]]

So to get things going here is the investigation (great thanks to :mconley):

* _blank is clicked
* magic is happening. It calls
* http://hg.mozilla.org/mozilla-central/file/c0ddb1b098ec/dom/ipc/TabChild.cpp#l1413 that calls
* http://hg.mozilla.org/mozilla-central/file/c0ddb1b098ec/dom/ipc/TabParent.cpp#l433 is calling OpenURIInFrame with nullptr as aOpener
* which is then handled in http://hg.mozilla.org/mozilla-central/file/a255a234946e/browser/base/content/browser.js#l4303
* _openURIInNewTab is called (http://hg.mozilla.org/mozilla-central/file/a255a234946e/browser/base/content/browser.js#l4220) with aOpener that's null and so referrer in loadOneTab is null
* it is passed to http://hg.mozilla.org/mozilla-central/file/c0ddb1b098ec/browser/base/content/tabbrowser.xml#l1357 and then
* http://hg.mozilla.org/mozilla-central/file/c0ddb1b098ec/browser/base/content/tabbrowser.xml#l1515 
* then code doing tab insertion (or shuffling, because originally the tab is inserted at the end): http://hg.mozilla.org/mozilla-central/file/c0ddb1b098ec/browser/base/content/tabbrowser.xml#l1722

So pretty much to activate this mechanism only the presence of referrer is necessary in that place. But there's code that actually needs aOpener (aka referrer), which is http://hg.mozilla.org/mozilla-central/file/a255a234946e/browser/base/content/browser.js#l4227. The only other place that uses referrerURI is http://hg.mozilla.org/mozilla-central/file/c0ddb1b098ec/browser/base/content/tabbrowser.xml#l1712. But I never saw the need for the window.

So it looks like it's enough to know the referrer's URI + whether it's a private window. We have aBaseURI (that is referrer's URI) in AnswerCreateWindow but I don't know (yet?) how do we get whether the window is private. I also don't know whether that's how we want to change this interface http://hg.mozilla.org/mozilla-central/file/a255a234946e/dom/interfaces/base/nsIBrowserDOMWindow.idl#l86.

Ideas :mconley?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

So, as tomasz has discovered, the big problem here seems to be that openURIInFrame (which is the mechanism that we use to open a new tab) requires an nsIDOMWindow for the opener argument. Unfortunately, TabParent has no access to the nsIDOMWindow for the opener, since the nsIDOMWindow is down in the child process, naturally.

This seems somewhat related to the discussion that went on in bug 537428 a ways back.

smaug - any thoughts now on how we should alter the nsIBrowserDOMWindow openURIInFrame to not require an nsIDOMWindow (while still allowing us to get a bead on whether the opener is "private", and its URI?). Is simply passing a boolean for private-ness and the URI directly oversimplifying the issue?

Comment 7

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

I don't think that is oversimplifying.
You may want to pass some state object which can be extended easily when needed.

Comment 8

[Tomasz Kołodziejski [:tomasz]]

Attachment: e10s-tab-open.patch

So here's the patch. I don't feel right about nsIOpenURIInFrameParams and openURIInFrame's arguments are kinda cumbersome.

Please have a look :mconley.

Oh, and there are some more usages of this openURIInFrame. I was told to ignore metro and but there's android stuff and chatWindow. I guess I should go and fix them. They pretty much don't care about the aOpener so that should be easy, however, I'm not sure how do I make sure I don't break them. I wasn't working with android code before.

And about uuid - do I have to change it? What's the reason for that?

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8514448 [details] [diff] [review]
e10s-tab-open.patch

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

Yeah, I think this looks pretty good. I do agree, however, that we should patch the other usages of openURIInFrame where we can.

> They pretty much don't care about the aOpener so that should be easy, however, I'm not sure how do I make sure I don't break them.
> I wasn't working with android code before.

Understood. So I'm actually a little concerned, because I looked at the Fennec usage of this interface, and they seem to rely on aOpener in order to determine a "parentId" for the opener. When we remove the aOpener like that, we kind of rob Fennec of the ability to get that ID... Guh. :/ However, I can't find any other place in platform that calls OpenURIInFrame besides TabParent, which only ever passes nullptr for aOpener... so who is calling that method?

::: dom/base/nsOpenURIInFrameParams.cpp
@@ +1,1 @@
> +#include "nsOpenURIInFrameParams.h"

Missing license.

@@ +1,5 @@
> +#include "nsOpenURIInFrameParams.h"
> +
> +NS_IMPL_ISUPPORTS(nsOpenURIInFrameParams, nsIOpenURIInFrameParams)
> +
> +nsOpenURIInFrameParams::nsOpenURIInFrameParams()

This constructor and destructor are just empty... I could be wrong, but I think we can just omit them (omitting their defn's in nsOpenURIInFrameParams.h too)

@@ +26,5 @@
> +
> +/* attribute boolean isPrivate; */
> +NS_IMETHODIMP nsOpenURIInFrameParams::GetIsPrivate(bool *aIsPrivate)
> +{
> +  *aIsPrivate = isPrivate;

Always good to do NS_ENSURE_ARG_POINTER with the pointer you're given.

::: dom/base/nsOpenURIInFrameParams.h
@@ +1,1 @@
> +#include "nsIBrowserDOMWindow.h"

Missing license.

@@ +12,5 @@
> +  ~nsOpenURIInFrameParams();
> +
> +protected:
> +  /* additional members */
> +  nsString referrer;

mReferrer and mIsPrivate, is generally how we roll down in XPCOM-land.

::: dom/interfaces/base/nsIBrowserDOMWindow.idl
@@ +20,1 @@
>  [scriptable, uuid(699b8f60-2898-11e4-8c21-0800200c9a66)]

Yeah, we'll need to bump this uuid. It's a unique identifier representing a version of an interface.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

How margaret - do you happen to know how OpenURIInFrame is used by Fennec?

Comment 11

[:Margaret Leibovic]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #10)
> How margaret - do you happen to know how OpenURIInFrame is used by Fennec?

Here is our openURIInFrame implementation:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3099

It looks like the patch here is changing the aOpener parameter to explicitly include a referrer and an isPrivate flag. We would need to update our implementation, but it looks doable. The one challenge would be figuring out how to set the parent tab in the case of opening a new tab:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3067

Comment 12

[Tomasz Kołodziejski [:tomasz]]

Comment on attachment 8514448 [details] [diff] [review]
e10s-tab-open.patch

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

So my understanding is that in the clients I don't care about we can just use aOpener = null. It won't change their behavior since they're now just getting nullptr. I'll post a patch with that change and the things I'm mentioning below.

::: dom/base/nsOpenURIInFrameParams.cpp
@@ +1,1 @@
> +#include "nsOpenURIInFrameParams.h"

Done.

@@ +1,5 @@
> +#include "nsOpenURIInFrameParams.h"
> +
> +NS_IMPL_ISUPPORTS(nsOpenURIInFrameParams, nsIOpenURIInFrameParams)
> +
> +nsOpenURIInFrameParams::nsOpenURIInFrameParams()

It looks like I cannot remove the destructor:

static_assert failed "Reference-counted class nsOpenURIInFrameParams should not have a public destructor. Try to make this class's destructor non-public. If that is really not possible, you can whitelist this class by providing a HasDangerousPublicDestructor specialization for it."

But removing constructor works just fine.

@@ +26,5 @@
> +
> +/* attribute boolean isPrivate; */
> +NS_IMETHODIMP nsOpenURIInFrameParams::GetIsPrivate(bool *aIsPrivate)
> +{
> +  *aIsPrivate = isPrivate;

Done.

::: dom/base/nsOpenURIInFrameParams.h
@@ +1,1 @@
> +#include "nsIBrowserDOMWindow.h"

Added.

@@ +12,5 @@
> +  ~nsOpenURIInFrameParams();
> +
> +protected:
> +  /* additional members */
> +  nsString referrer;

Done.

::: dom/interfaces/base/nsIBrowserDOMWindow.idl
@@ +20,1 @@
>  [scriptable, uuid(699b8f60-2898-11e4-8c21-0800200c9a66)]

Done.

Comment 13

[Tomasz Kołodziejski [:tomasz]]

Attachment: e10s-tab-open.patch

So here's the updated patch. See comment 12.

Please have a look :mconley.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8514631 [details] [diff] [review]
e10s-tab-open.patch

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

r=me for the desktop browser bits. I think you'll want smaug to look at this too, and get sign-off from somebody from Fennec and a Metro contributor for the changes over there as well.

So, targeting:

smaug for moz.build, nsOpenURIInFrameParams.cpp/.h, nsIBrowserDOMWindow.idl, TabParent.cpp
ally for the small change in Metro's browser.js
margaret for the small change in Fennec's browser.js

::: dom/base/nsOpenURIInFrameParams.cpp
@@ +8,5 @@
> +NS_IMPL_ISUPPORTS(nsOpenURIInFrameParams, nsIOpenURIInFrameParams)
> +
> +nsOpenURIInFrameParams::~nsOpenURIInFrameParams()
> +{
> +  /* destructor code */

Again, I'm really not sure we need this - I'm reasonably certain I've added classes and not needed an empty destructor before.

::: dom/base/nsOpenURIInFrameParams.h
@@ +15,5 @@
> +  ~nsOpenURIInFrameParams();
> +
> +protected:
> +  /* additional members */
> +  nsString mReferrer;

I think these should be private. We're MOZ_FINAL here anyways.

Comment 15

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

Comment on attachment 8514631 [details] [diff] [review]
e10s-tab-open.patch


>+/* attribute DOMString referrer; */
>+NS_IMETHODIMP nsOpenURIInFrameParams::GetReferrer(nsAString & aReferrer)
NS_IMETHODIMP
nsOpenURIInFrameParams::GetReferrer(nsAString& aReferrer)


>+NS_IMETHODIMP nsOpenURIInFrameParams::SetReferrer(const nsAString & aReferrer)
NS_IMETHODIMP
nsOpenURIInFrameParams::SetReferrer(const nsAString& aReferrer)


>+NS_IMETHODIMP nsOpenURIInFrameParams::GetIsPrivate(bool *aIsPrivate)
NS_IMETHODIMP
nsOpenURIInFrameParams::GetIsPrivate(bool* aIsPrivate)

>+NS_IMETHODIMP nsOpenURIInFrameParams::SetIsPrivate(bool aIsPrivate)
NS_IMETHODIMP
nsOpenURIInFrameParams::SetIsPrivate(bool aIsPrivate)



>+protected:
>+  /* additional members */
You could just drop this part.


>+  nsString mReferrer;
>+  bool mIsPrivate;
Please add the default constructor where you initialize mIsPrivate to false.

Comment 16

[:Margaret Leibovic]

Comment on attachment 8514631 [details] [diff] [review]
e10s-tab-open.patch

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

::: mobile/android/chrome/content/browser.js
@@ +3096,5 @@
>      return browser ? browser.contentWindow : null;
>    },
>  
> +  openURIInFrame: function browser_openURIInFrame(aURI, aParams, aWhere, aContext) {
> +    let browser = this._getBrowser(aURI, null, aWhere, aContext);

We should file a follow-up bug to update our _getBrowser implementation, since we'd also want to support the referrer and the private mode flags:

http://hg.mozilla.org/mozilla-central/file/5999e92e89ff/mobile/android/chrome/content/browser.js#l3086
http://hg.mozilla.org/mozilla-central/file/5999e92e89ff/mobile/android/chrome/content/browser.js#l3122

As I mentioned in a previous comment, I was worried about this regressing setting the parent tab for new tabs, but am I correct in seeing that openURIInFrame is currently only ever called with a null aOpener parameter, so this wouldn't actually regress anything?

Please file a Firefox for Android::General bug to update the _getBrowser method to use aParams to set the referrerURI and isPrivate flags. Bonus points if you fix it, too! :)

Comment 17

[Allison Naaktgeboren :ally]

Comment on attachment 8514631 [details] [diff] [review]
e10s-tab-open.patch

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

::: browser/metro/base/content/browser.js
@@ -1079,5 @@
>      return browser ? browser.contentWindow : null;
>    },
>  
> -  openURIInFrame: function browser_openURIInFrame(aURI, aOpener, aWhere, aContext) {
> -    let browser = this._getBrowser(aURI, aOpener, aWhere, aContext);

this change is pretty trivial, but you have my blessing for it r+ for the metro change.

Comment 18

[Tomasz Kołodziejski [:tomasz]]

Comment on attachment 8514631 [details] [diff] [review]
e10s-tab-open.patch

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

I'll post an updated patch is a second.

:Margaret, that's exactly my understanding as well. Unless somebody calls that method in a sneaky JavaScript way like nsBrowserAccess["open" + "URI" + "In" + "Frame"](). If that's the case he'd better fix it. Created bug 1093921.

:smaug, done. It's funny that the automatically generated code is using wrong style. Maybe that's a bug.

:mconley, when I remove the empty destructor it fails with the following error:

 2:33.67 ld: warning: could not create compact unwind for _ffi_call_unix64: does not use RBP or RSP based frame
 2:33.67 Undefined symbols for architecture x86_64:
 2:33.67   "nsOpenURIInFrameParams::~nsOpenURIInFrameParams()", referenced from:
 2:33.67       nsOpenURIInFrameParams::Release() in Unified_cpp_dom_base6.o
 2:33.67 ld: symbol(s) not found for architecture x86_64
 2:33.67 clang: error: linker command failed with exit code 1 (use -v to see invocation)

So I'll keep the destructor implementation.

::: browser/metro/base/content/browser.js
@@ -1079,5 @@
>      return browser ? browser.contentWindow : null;
>    },
>  
> -  openURIInFrame: function browser_openURIInFrame(aURI, aOpener, aWhere, aContext) {
> -    let browser = this._getBrowser(aURI, aOpener, aWhere, aContext);

Thanks!

::: dom/base/nsOpenURIInFrameParams.cpp
@@ +8,5 @@
> +NS_IMPL_ISUPPORTS(nsOpenURIInFrameParams, nsIOpenURIInFrameParams)
> +
> +nsOpenURIInFrameParams::~nsOpenURIInFrameParams()
> +{
> +  /* destructor code */

When I removed implementation but not the definition (because assertion forcing me to make it private) it works.

::: dom/base/nsOpenURIInFrameParams.h
@@ +15,5 @@
> +  ~nsOpenURIInFrameParams();
> +
> +protected:
> +  /* additional members */
> +  nsString mReferrer;

Done.

::: mobile/android/chrome/content/browser.js
@@ +3096,5 @@
>      return browser ? browser.contentWindow : null;
>    },
>  
> +  openURIInFrame: function browser_openURIInFrame(aURI, aParams, aWhere, aContext) {
> +    let browser = this._getBrowser(aURI, null, aWhere, aContext);

:Margaret, that's exactly my understanding as well. Unless somebody calls that method in a sneaky JavaScript way like nsBrowserAccess["open" + "URI" + "In" + "Frame"](). If that's the case he'd better fix it. Created bug 1093921.

Comment 19

[Tomasz Kołodziejski [:tomasz]]

Attachment: e10s-tab-open.patch

Comment 20

[caralu1974]

Now when I open a new tab the original tab shows a "clock" turning and turning so I must press Esc to stop and show the page.

Comment 21

[Tomasz Kołodziejski [:tomasz]]

[e-mail], I've also noticed it. But I think it's unrelated to this bug (this bug is about placement of the new tab. Immediately after is not here like "show it right now" but "right after"). Can you file a new bug about it? (or search for more appropriate one if possible)

Comment 22

[caralu1974]

Yes you`r right. I will.

Comment 23

[Tomasz Kołodziejski [:tomasz]]

Attachment: e10s-tab-open.patch

Updated patch (it was missing some #include) and try run:

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

I'm pretty skeptic about the failure of R-e10s. It doesn't look like an intermittent anymore. I'll rerun it a couple of times and if it still fails I'll try to find a linux machine.

Comment 24

[Tomasz Kołodziejski [:tomasz]]

It looks like R-e10s is failing not because of my changes and it's a known failure. And so checkin-needed. Thanks for your help :mconley and all the reviewers!

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/5caf99f63059

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5caf99f63059

Comment 27

[Bogdan Maris, Desktop QA]

Reproduced the issue on old Nightly (2014-11-04), verified that the issue is fixed on Windows 7 64bit, Ubuntu 14.04 32bit and Mac OS X 10.9.5 using latest Nightly.