1356922 - Replace the sNextTabParent setup with something more robust

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

[(no longer active)]

I don't know why sNextTabParent was implemented as a global variable.  I suppose it was technical debt left for someone else to deal with.  :/

This has been creating a lot of headache for me over in bug 1343728.  Originally I thought maybe I can extend the hack in various ways but none is ending up working well.

The core of the issue is that with my changes in bug 1343728, now the child process can quickly request the creation of several windows so the parent needs to be able to deal with several of these ongoing requests to arrive and make progress in any order.

I thought about this a little bit and we basically have one of the two cases here as far as I can tell, either the child has requested a new tab or a new window.  In the case where the child has requested a new tab, we can store the next tab parent on the new tab.  In the case where the child has requested a new window, we can store the next tab parent on the new nsXULWindow that we create before we spin the event loop.

In practice this involves a bit of piping the information through the system.  Also because the tab opening case goes through JS we can't deal with a TabParent* directly, so we use an ID that we store as an attribute instead.

I have a basic patch which works on a small test case, I'm going to try to push it to try and see what happens.

Comment 1

[(no longer active)]

Attachment: Part 1: Replace the next TabParent global pointer with per-window/tab next TabParent ID

This patch replaces the usage of sNextTabParent pointer to store the next
PBrowser parent actor to be used by the next frame loader with the
following information:

  * In the case where the content JS has requested a new tab, the ID of the
    next TabParent will be stored on the <xul:browser> element.
  * In the case where the content JS has requested a new window, the ID of
    the next TabParent will be stored on the created nsXULWindow.

Comment 2

[(no longer active)]

Attachment: Part 2: Remove TabParent::sNextTabParent

Comment 3

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

Comment on attachment 8858689 [details] [diff] [review]
Part 1: Replace the next TabParent global pointer with per-window/tab next TabParent ID

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

Thanks!

::: browser/base/content/tabbrowser.xml
@@ +1966,5 @@
>  
>              b.setAttribute("autoscrollpopup", this._autoScrollPopup.id);
>  
> +            if (aParams.nextTabParentId) {
> +              b.setAttribute("nextTabParentId", aParams.nextTabParentId.toString());

Since this is a "magic" attribute that Gecko cares about, can you add a comment about how our native layer is going to read this in and use it?

::: dom/ipc/ContentParent.cpp
@@ +1176,5 @@
>  
> +  if (aNextTabParentId) {
> +    if (TabParent* parent =
> +          sNextTabParents.GetAndRemove(aNextTabParentId).valueOr(nullptr)) {
> +      parent->SetOwnerElement(aFrameElement);

Should we assert that we don't already somehow have an owner element here?

::: toolkit/components/windowcreator/nsIWindowCreator2.idl
@@ +36,5 @@
>        @param aOpeningTab The TabParent that is trying to open this new chrome
>                           window. Can be nullptr.
>        @param aOpener The window which is trying to open this new chrome window.
>                       Can be nullptr
> +      @param aNextTabParentId The integer ID of the next tab parent actor to use.

Probably a good idea to mention that 0 is special (at least, it seems to be in nsFrameLoader.cpp, according to my read of this).

::: toolkit/components/windowwatcher/nsPIWindowWatcher.idl
@@ +102,5 @@
>     * @param aOpenerFullZoom
>     *        The current zoom multiplier for the opener tab. This is then
>     *        applied to the newly opened window.
> +   * @param aNextTabParentId
> +   *        The integer ID for the next tab parent actor.

Same as above, re: 0.

::: xpfe/appshell/nsIXULWindow.idl
@@ +115,5 @@
>     * @param aChromeFlags see nsIWebBrowserChrome
>     * @param aOpeningTab the TabParent that requested this new window be opened.
>     *                    Can be left null.
>     * @param aOpener The window which is requesting that this new window be opened.
> +   * @param aNextTabParentId The integer ID of the next tab parent actor to use.

Same as above, re: 0.

Comment 4

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

Comment on attachment 8858689 [details] [diff] [review]
Part 1: Replace the next TabParent global pointer with per-window/tab next TabParent ID

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

I have a suggestion for a different way to do this. Rather than pass around an ID, could you pass around an nsITabParent**? Instead of using an attribute on the <browser> you could use a property on nsIBrowser (like we do for sameProcessAsFrameLoader). Once we decided to use the nsITabParent**, we would null it out (so that we could set aWindowIsNew correctly).

I'm not sure this is better, but it seems more direct to me. Also, I might be missing some reason why it won't work. Please do as you wish.

Comment 5

[(no longer active)]

(In reply to Bill McCloskey (:billm) from comment #4)
> I have a suggestion for a different way to do this. Rather than pass around
> an ID, could you pass around an nsITabParent**? Instead of using an
> attribute on the <browser> you could use a property on nsIBrowser (like we
> do for sameProcessAsFrameLoader). Once we decided to use the nsITabParent**,
> we would null it out (so that we could set aWindowIsNew correctly).

So as I was trying to do this, I realized I don't know one part of how to do this: how to deal with the nsITabParent** value in JS.  I managed to trick the IDL compiler into allowing me to pass this as an arg to openURIInFrame() using |[ptr] native TabParentPtr(nsITabParent*)| but I also need to store this on the XBL binding etc, so I need to somehow deal with it in JS.  Maybe my XPConnect-fu isn't strong enough to figure out what you had in mind here.  :-)

Comment 6

[(no longer active)]

Maybe I should just land the patches that I have here?  There is still quite a bit of work remaining to get the patches for bug 
1343728 pass all of the tests, and it seems like focusing on that may be a better use of time.  I tried a bit more to get the nsITabParent** approach to work during the weekend without any luck.

Comment 7

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

(In reply to :Ehsan Akhgari (super long backlog, slow to respond, not reviewing for a while) from comment #6)
> Maybe I should just land the patches that I have here?  There is still quite
> a bit of work remaining to get the patches for bug 
> 1343728 pass all of the tests, and it seems like focusing on that may be a
> better use of time.  I tried a bit more to get the nsITabParent** approach
> to work during the weekend without any luck.

Sure, that's fine. I didn't think hard enough about this I guess.

Comment 8

[(no longer active)]

Thanks, neither did I in the beginning. :-)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61f63d09c780
Part 1: Replace the next TabParent global pointer with per-window/tab next TabParent ID; r=billm,mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/56c8ce11c629
Part 2: Remove TabParent::sNextTabParent; r=billm

Comment 10

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/61f63d09c780
https://hg.mozilla.org/mozilla-central/rev/56c8ce11c629

Comment 11

[Jorge Villalobos [:jorgev] (he/him)]

Looks like this caused bug 1359544.