1260535 - Update the UI of the tab when window.open() is executed from a container

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Andrea Marchesini [:baku]]

Attachment: patch 1 - non-e10s

Currently we open a window in the correct compartment, but without the proper UI shown.
The patch here notifies the tabbrowser that a new toplevel window has been created and it sets the userContextId attributes in the browser XULElement.

It doesn't work with e10s yet because we need an additional step in platform, I guess. I'm working on it.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch 2 - e10s

Comment 2

[Andrea Marchesini [:baku]]

Attachment: patch 2 - e10s

Comment 3

[Marco Castelluccio [:marco]]

Comment on attachment 8736001 [details] [diff] [review]
patch 1 - non-e10s

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

I'm not the right person to review this patch. Did you mean :mak?

Comment 4

[:Gijs (he/him)]

Comment on attachment 8736001 [details] [diff] [review]
patch 1 - non-e10s

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

::: browser/base/content/tabbrowser.xml
@@ +4282,5 @@
> +              if (!window) {
> +                break;
> +              }
> +
> +              let tab = this._getTabForContentWindow(window);

This might throw for non-browser-content-windows in non-e10s (which will also fire this observer notification), and it's not necessary in e10s mode because of patch #2, AIUI. So I think this needs both a bail-out if we're in e10s mode, and a check that this is actually a content window. It might be good to just do:

if (!window || gMultiProcessBrowser) {
  return;
}

let browser = window.QueryInterface(Ci.nsIInterfaceRequestor)
                    .getInterface(Ci.nsIDocShell)
                    .chromeEventHandler;

if (!browser || browser.ownerDocument != document) {
  return;
}

and then just call this.getTabForBrowser(browser);

Comment 5

[Andrea Marchesini [:baku]]

Attachment: ux2.patch

It's easy to apply the comments in 1 single patch than 2.
Gijs, can you review the tabbed browser part of the patch? (also the rest if you want)
Blake, for you there is all the rest... Thanks!

Comment 6

[:Gijs (he/him)]

Comment on attachment 8736710 [details] [diff] [review]
ux2.patch

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

Err... so I hadn't actually looked at this part yet. Now I'm confused. The tabbrowser.xml observer only lives in the parent process. The notification for the new content window, presumably, is being fired in the content process, as it has the content window as a subject and you can't do that in the parent process because the window doesn't exist there. AIUI, the way this is written we will never update the tab's userContextId in the parent. What am I missing?


Really, this stuff could do with tests. :-\

::: browser/base/content/tabbrowser.xml
@@ +4288,5 @@
> +                                  .getInterface(Ci.nsIDocShell)
> +                                  .chromeEventHandler;
> +
> +              if (!browser || browser.ownerDocument != document) {
> +                return;

Oh, I'd not realized when commenting that we were in a switch and everywhere else was using break; Let's do that instead (ie break; rather than return;)

@@ +4299,5 @@
> +
> +              let originAttributes;
> +              try {
> +                originAttributes = JSON.parse(aData);
> +              } catch (e) {

This feels like it should report the exception - we shouldn't be getting unparsable data as part of this notification.

Comment 7

[Andrea Marchesini [:baku]]

> What am I missing?

This is actually wrong. We dispatch the notification in the parent and in the child process as well.
This is the reason why this patch works. The tabbrowser.xml receives the notification but the window (docShell) doesn't have the userContextId set and I have to pass it as argument.

> > +              if (!browser || browser.ownerDocument != document) {
> > +                return;

Right. To be fixed :)

> This feels like it should report the exception - we shouldn't be getting
> unparsable data as part of this notification.

I fix these 2 issues and then I submit a new patch for a second review if this is OK for you.

Comment 8

[:Gijs (he/him)]

(In reply to Andrea Marchesini (:baku) from comment #7)
> > What am I missing?
> 
> This is actually wrong. We dispatch the notification in the parent and in
> the child process as well.

So is window (subject of the notification) a CPOW in the notification in the parent process? Because then the changes I made you make in comment #4 break your patch. You'll need to keep using getTabForContentWindow. But that also feels pretty ugly. :-\

Comment 9

[Andrea Marchesini [:baku]]

Attachment: ux2.patch

Comment 10

[Andrea Marchesini [:baku]]

> So is window (subject of the notification) a CPOW in the notification in the
> parent process? Because then the changes I made you make in comment #4 break
> your patch. You'll need to keep using getTabForContentWindow. But that also
> feels pretty ugly. :-\


So... I don't know if it's CPOW. but with your comments the patch still works.
If there is anything I can do to check it more, let me know.

Comment 11

[:Gijs (he/him)]

(In reply to Andrea Marchesini (:baku) from comment #10)
> > So is window (subject of the notification) a CPOW in the notification in the
> > parent process? Because then the changes I made you make in comment #4 break
> > your patch. You'll need to keep using getTabForContentWindow. But that also
> > feels pretty ugly. :-\
> 
> 
> So... I don't know if it's CPOW. but with your comments the patch still
> works.
> If there is anything I can do to check it more, let me know.

How can I test this patch? Also, would you consider writing an automated test?

Comment 12

[Andrea Marchesini [:baku]]

1. pref: privacy.userContext.enabled to true
2. open about:preferences and mark to false 'Open new window in tab instead'
3. open a new container tab from the File menu.
4. Load a page with: <a href="something" target="foo">click me</a>
5. clicking on 'click me' you should see a new window with a new tab from the same container.

Comment 13

[:Gijs (he/him)]

Comment on attachment 8736965 [details] [diff] [review]
ux2.patch

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

So I talked about this a bunch with mconley. Long story short, while this works today, it works because in e10s, the initial tab we create is same (ie parent) process, not in the child process. That's likely to change sooner rather than later for performance reasons. Such a change will break this functionality as written.

The other thing is that in the patch as written, we're adding an observer for every window, which ends up not being very effective.

So instead, can you please use create a JSM that listens for this message, import that from tab-content.js, and from that JSM send a message to the parent using sendAsyncMessage? Then tabbrowser.xml can listen for that message and set the attribute on the corresponding tab. That'll work in both e10s and non-e10s, and it won't break if we change how the initial tab in a new window behaves.

Finally, please please please write a browser chrome test for this. Especially because this is preffed off by default, we really need tests to ensure we don't regress functionality.

Comment 14

[Andrea Marchesini [:baku]]

> So instead, can you please use create a JSM that listens for this message,
> import that from tab-content.js, and from that JSM send a message to the
> parent using sendAsyncMessage? Then tabbrowser.xml can listen for that
> message and set the attribute on the corresponding tab. That'll work in both
> e10s and non-e10s, and it won't break if we change how the initial tab in a
> new window behaves.

This doesn't seem the correct approach because when tabbrowser.xml receives the communication, it doesn't know which window should have this userContextId. Definitely it's not aMessage.target, because that one is not the new toplevel window.
And I cannot send the window cross thread.

I also tried to do:

    let browser = aSubject.QueryInterface(Ci.nsIInterfaceRequestor)                                              
                          .getInterface(Ci.nsIWebNavigation)                                                     
                          .QueryInterface(Ci.nsIDocShell)                                                        
                          .chromeEventHandler;
    browser.messageManager.sendAsyncMessage(...);

but the message is not received by tabbrowser.xml.

Am I missing something?

Comment 15

[:Gijs (he/him)]

(In reply to Andrea Marchesini (:baku) from comment #14)
> > So instead, can you please use create a JSM that listens for this message,
> > import that from tab-content.js, and from that JSM send a message to the
> > parent using sendAsyncMessage? Then tabbrowser.xml can listen for that
> > message and set the attribute on the corresponding tab. That'll work in both
> > e10s and non-e10s, and it won't break if we change how the initial tab in a
> > new window behaves.
> 
> This doesn't seem the correct approach because when tabbrowser.xml receives
> the communication, it doesn't know which window should have this
> userContextId. Definitely it's not aMessage.target, because that one is not
> the new toplevel window.

It should be. I'm very confused. We're effectively still doing what your patch is doing, but the observer of the observer service notification now lives in the content process.

The new toplevel window is created in the new tab in the new window, so the new window's tabbrowser (not the old window's!) will receive the message you're sending, and the <browser> in question should correspond to aMessage.target.

Oh - maybe you're not sending the async message from the JSM using the "right" message manager? When you get the observer service notification, you should be able to QI the window:

let messageManager = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
                                    .getInterface(Ci.nsIDocShell)
                                    .QueryInterface(Ci.nsIInterfaceRequestor)
                                    .getInterface(Ci.nsIContentFrameMessageManager);


You should be able to call sendAsyncMessage on that, and then aMessage.target should be correct.

Comment 16

[Andrea Marchesini [:baku]]

> You should be able to call sendAsyncMessage on that, and then
> aMessage.target should be correct.

Ok. I found the reason why it doesn't work. toplevel-window-ready is not dispatched on e10s in the content process but always in the parent process. Having the JSM to listen for this will not fix the issue.

Can we go back to the original patch? Or do you want to help me with a different solution?
Thanks!

Comment 17

[:Gijs (he/him)]

(In reply to Andrea Marchesini (:baku) from comment #16)
> > You should be able to call sendAsyncMessage on that, and then
> > aMessage.target should be correct.
> 
> Ok. I found the reason why it doesn't work. toplevel-window-ready is not
> dispatched on e10s in the content process but always in the parent process.
> Having the JSM to listen for this will not fix the issue.

This doesn't really make sense. The content window is (eventually) created in the content process. How is that notification sent to the parent process?

As noted in comment #13, I think the existing solution will not work in the long term, and I'd rather we build something that we won't have to change within the next month or so, but I defer to mconley.

Comment 18

[Andrea Marchesini [:baku]]

> This doesn't really make sense. The content window is (eventually) created
> in the content process. How is that notification sent to the parent process?

The top window is created on the parent process, not in the child process.
Child process, as far as I know, doesn't have any 'toplevel' window.

We don't dispatch 'toplevel-window-ready' for each new window.

Comment 19

[:Gijs (he/him)]

(In reply to Andrea Marchesini (:baku) from comment #18)
> > This doesn't really make sense. The content window is (eventually) created
> > in the content process. How is that notification sent to the parent process?
> 
> The top window is created on the parent process, not in the child process.
> Child process, as far as I know, doesn't have any 'toplevel' window.
> 
> We don't dispatch 'toplevel-window-ready' for each new window.

But you're using the chrome event handler of the window you're getting as the browser that's in the <tabbrowser>. How can that possibly work if toplevel-window-ready passes the chrome window?

Note also that as-is, if you have 2 windows open and you open a 3rd, the code will run twice... that also seems wrong. :-\

Comment 20

[Andrea Marchesini [:baku]]

Attachment: ux.patch

Comment 21

[:Gijs (he/him)]

Comment on attachment 8738608 [details] [diff] [review]
ux.patch

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

This is neat! I have quite some nits, sorry about that. I can feel the r+ coming, promise! :-)

::: browser/base/content/tabbrowser.xml
@@ +4210,2 @@
>                  return undefined;
> +              }

Nit: please don't brace this existing code, so as to leave blame the same.

@@ +4283,5 @@
>                }
>                break;
> +            case "toplevel-window-ready":
> +              let window = aSubject.QueryInterface(Ci.nsIDOMWindow);
> +              if (!window) {

Please rename this variable to "win", and also check if win == window, and bail if this is not the case.

@@ +4318,5 @@
>            Services.obs.addObserver(this, "live-resize-start", false);
>            Services.obs.addObserver(this, "live-resize-end", false);
>  
> +          if (!gMultiProcessBrowser) {
> +            Services.obs.addObserver(this, "toplevel-window-ready", false);

We probably can't rely on all new window links to be opened in remote browsers even if we're using e10s, so I would do this unconditionally, and remove the observer in the browser:init case if still present (this likely means you'll need a <field> to keep track of whether it's been added or not).

We should also make sure we remove it in the destructor or unload handler (check what we do for the other observers?) to avoid leaking the world if the window gets closed before toplevel-window-ready fires.

::: browser/components/contextualidentity/test/browser/browser_windowOpen.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */

You don't need this - tests are public domain by default.

Instead, please add "use strict";

@@ +8,5 @@
> +add_task(function* setup() {
> +  SpecialPowers.pushPrefEnv({"set": [
> +    ["privacy.userContext.enabled", true],
> +    ["browser.link.open_newwindow", 2],
> +  ]});

I believe this is async? I'm used to seeing things like:

yield new Promise((resolve) => {
  SpecialPowers.pushPrefEnv({"set": prefs}, resolve);
});

@@ +14,5 @@
> +
> +add_task(function* cleanup() {
> +  // make sure we don't leave any prefs set for the next tests
> +  registerCleanupFunction(function() {
> +    SpecialPowers.popPrefEnv();

We don't need to do this - pushPrefEnv takes care of this.

@@ +28,5 @@
> +  yield BrowserTestUtils.browserLoaded(browser);
> +
> +  info("Opening a new window from this tab...");
> +  ContentTask.spawn(browser, { url: BASE_URI }, function(opts) {
> +    content.window.newWindow = content.window.open(opts.url, "foobar");

Nit: pass BASE_URI as the arg, rename opts to "url" and use that directly. Doesn't need to be an object, just a plain string works.

Nit: please use "_blank" instead of "foobar"

@@ +31,5 @@
> +  ContentTask.spawn(browser, { url: BASE_URI }, function(opts) {
> +    content.window.newWindow = content.window.open(opts.url, "foobar");
> +  });
> +
> +  let window = yield BrowserTestUtils.waitForNewWindow();

Nit: please don't shadow the "window" global. Name the new thing newWindow or something.

@@ +35,5 @@
> +  let window = yield BrowserTestUtils.waitForNewWindow();
> +  let newTab = window.gBrowser.selectedTab;
> +
> +  let newBrowser = window.gBrowser.getBrowserForTab(newTab);
> +  yield BrowserTestUtils.browserLoaded(newBrowser);

You can just use:

yield BrowserTestUtils.browserLoaded(newTab.linkedBrowser);

without relying on getBrowserForTab.

@@ +41,5 @@
> +
> +  info("Closing the new window...");
> +  yield ContentTask.spawn(browser, null, function(opts) {
> +    content.window.newWindow.close();
> +  });

Instead of these 3 lines please use:

yield BrowserTestUtils.closeWindow(newWindow);

@@ +43,5 @@
> +  yield ContentTask.spawn(browser, null, function(opts) {
> +    content.window.newWindow.close();
> +  });
> +
> +  gBrowser.removeTab(tab);

likewise, please use:

yield BrowserTestUtils.removeTab(tab);

to avoid issues with the (async) tab/window closing not having completed by the time the test finishes.

::: toolkit/content/browser-child.js
@@ +632,5 @@
>  var outerWindowID = content.QueryInterface(Ci.nsIInterfaceRequestor)
>                             .getInterface(Ci.nsIDOMWindowUtils)
>                             .outerWindowID;
> +
> +var userContextId = content.document.nodePrincipal.originAttributes.userContextId;

I'd like mconley to look at this in case it needs some nullchecking.

@@ +635,5 @@
> +
> +var userContextId = content.document.nodePrincipal.originAttributes.userContextId;
> +
> +sendAsyncMessage("Browser:Init", {outerWindowID: outerWindowID,
> +                                  userContextId: userContextId});

In ES6 you can just do:

sendAsyncMessage("Browser:Init", {outerWindowID, userContextId});

(also ugh at ID vs. Id...)

Comment 22

[Andrea Marchesini [:baku]]

Attachment: ux.patch

Comment 23

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

Clearing needinfo - I'll be helping to review this.

Comment 24

[:Gijs (he/him)]

Comment on attachment 8738637 [details] [diff] [review]
ux.patch

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

::: browser/base/content/tabbrowser.xml
@@ +4287,5 @@
>                }
>                break;
> +            case "toplevel-window-ready":
> +              let win = aSubject.QueryInterface(Ci.nsIDOMWindow);
> +              if (!win || win == window) {

OK, so the bad news is that I misunderstood what is going on here. We *do* get a content window (not a chrome window), because you're still passing it to _getTabForContentWindow and not getting null back. So comparing with |window| is pointless.

The other bad news is that when we open a new window with ctrl-n or whatever the platform-appropriate shortcut is, I'm seeing the observer notification coming in *before* we attach the listener. So this code doesn't get hit at all in that case, and we still end up with N listeners (for N windows) that don't go away. :-\

I tried looking at how to fix that but couldn't really see an easy way. document.readyState is "uninitialized" in both cases. The difference in order is very surprising to me, and I'm not sure how to deal with it. We should clearly set the usercontextid on the tab as soon as possible, but equally, I'm not sure how to do that consistently in the non-e10s case. Maybe mconley can help...

Comment 25

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

Sorry for the delay - catching up now. Starting to review...

Comment 26

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

Comment on attachment 8738637 [details] [diff] [review]
ux.patch

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

I think this makes a lot of sense - more sense than what I was originally suggesting.

I do think, however, that there's a way we can have a common path for both e10s and non-e10s, if we add an async message to either tab-content.js or content.js that tabbrowser.xml listens to for setting the userContextId attribute on tabs.

::: browser/components/contextualidentity/test/browser/browser_windowOpen.js
@@ +2,5 @@
> +
> +const BASE_URI = "http://mochi.test:8888/browser/browser/components/"
> +  + "contextualidentity/test/browser/empty_file.html";
> +
> +add_task(function* setup() {

I see something like this already being used in browser_windowName.js - I wonder if it's worth putting this out in a head.js file.

@@ +12,5 @@
> +  });
> +});
> +
> +
> +add_task(function* test() {

Thanks for the test! Can you add a comment at the top documenting what exactly it's testing?

::: toolkit/content/browser-child.js
@@ +632,5 @@
>  var outerWindowID = content.QueryInterface(Ci.nsIInterfaceRequestor)
>                             .getInterface(Ci.nsIDOMWindowUtils)
>                             .outerWindowID;
> +
> +var userContextId = content.document.nodePrincipal.originAttributes.userContextId;

Yes! This is a far better solution to the problem than what I was originally suggesting (going through that window watcher gunk, setting arguments, etc).

We actually might want to drop this message in tab-content.js[1] so that we receive it for both e10s and non-e10s. That way, I don't think you need the toplevel-window-ready observer, and we have a single code path.

[1]: tab-content.js is the right place if we only care about user contexts in browser tabs. If we care about them across other browsers (ex: sidebar browsers) then we might want to drop it in browser/base/content/content.js instead.

Comment 27

[Andrea Marchesini [:baku]]

> I do think, however, that there's a way we can have a common path for both
> e10s and non-e10s, if we add an async message to either tab-content.js or
> content.js that tabbrowser.xml listens to for setting the userContextId
> attribute on tabs.

tab-content.js and content.js run in the content process, do they?
If yes, this would not work because I need to set the attribute in the tab xulElement in the parent process.

Comment 28

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

(In reply to Andrea Marchesini (:baku) from comment #27)
> > I do think, however, that there's a way we can have a common path for both
> > e10s and non-e10s, if we add an async message to either tab-content.js or
> > content.js that tabbrowser.xml listens to for setting the userContextId
> > attribute on tabs.
> 
> tab-content.js and content.js run in the content process, do they?
> If yes, this would not work because I need to set the attribute in the tab
> xulElement in the parent process.

They _can_ run in the content process, but don't have to. tab-content.js and content.js are framescripts that are loaded in browsers whether or not they're loaded remotely (unlike browser-child.js, which is loaded in only remote browsers).

tab-content.js is specifically for tabbrowser tab <xul:browser>'s, and content.js is for <xul:browser>'s in general.

If you can get at the userContextId from within one of those scripts, you can message the parent, which can handle the message in tabbrowser.xml (and if that's the only place where we need to support it, then tab-content.js is probably the one you want), and then the tabbrowser.xml handler (which runs in the parent) can set the attribute.

This has the advantage of working seamlessly for both remote and non-remote browsers.

Comment 29

[Andrea Marchesini [:baku]]

Attachment: ux.patch

Comment 30

[Andrea Marchesini [:baku]]

A new patch. Can you review it?

Comment 31

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

Comment on attachment 8740613 [details] [diff] [review]
ux.patch

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

Almost there - just some dead code to remove.

::: browser/base/content/tab-content.js
@@ +863,5 @@
> +    // Just because we cannot change the userContextId from an active docShell,
> +    // we don't need to check DOMContentLoaded again.
> +    this.uninit();
> +    let userContextId = content.document.nodePrincipal.originAttributes.userContextId;
> +    sendAsyncMessage("FirstContentLoaded", { userContextId });

Let's make this Browser:FirstContentLoaded.

::: browser/base/content/tabbrowser.xml
@@ +4290,5 @@
> +              if (tab && data.userContextId) {
> +                tab.setUserContextId(data.userContextId);
> +              }
> +
> +              if (data.userContextId) {

Dead code?

::: toolkit/content/browser-child.js
@@ +632,5 @@
>  var outerWindowID = content.QueryInterface(Ci.nsIInterfaceRequestor)
>                             .getInterface(Ci.nsIDOMWindowUtils)
>                             .outerWindowID;
> +
> +var userContextId = content.document.nodePrincipal.originAttributes.userContextId;

This change is probably not necessary anymore.

Comment 32

[Andrea Marchesini [:baku]]

Attachment: ux.patch

Comment 33

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

Comment on attachment 8740640 [details] [diff] [review]
ux.patch

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

Looks good! Thanks!

::: browser/base/content/tab-content.js
@@ +849,5 @@
>  RefreshBlocker.init();
>  
> +var UserContextIdNotifier = {
> +  init() {
> +    addEventListener("DOMContentLoaded", this, false);

Event listeners are non-capturing by default, so we don't need that last argument.

Comment 34

[Pulsebot]

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

Comment 35

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

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