567058 - e10s: De-RPC createWindow

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Josh Matthews [:jdm]]

The createWindow IPDL message needs to be non-RPC to stop Necko from breaking.  There are some complications, though:

* RecvcreateWindow ends up sending a PIFrameEmbedding constructor over the wire, meaning that simply making createWindow sync is unworkable.
* There's no obvious way to create the PIFrameEmbedding actor first before sending createWindow, as the actor is created by nsFrameLoader very far removed from the receipt of the IPDL message
* Any attempt I've made to make createWindow asynchronous, followed by the parent pushing the result to the child or the child polling the parent for the result just ends up re-implementing RPC semantics

I'd love for somebody to show me something obvious that I've missed.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I don't think you've missed anything; that's why this message is RPC.

Why is it being RPC a problem for necko?

Comment 2

[Josh Matthews [:jdm]]

Any RPC messages can break necko invariants by causing messages to be processed before they're expected.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  This is the never-answered "RPC messages considered harmful?" thread in .dom?

Note the "RFC: Electrolysis manifesto: Chrome never blocks on content" in .dom, though.  In particular starting with the "Fri, 23 Apr 2010 14:47:28 -0500" mail from cjones...  Is there an existing bug on the work he mentioned there?

Comment 4

[Josh Matthews [:jdm]]

There is no prior work on this.  cjones and I have been talking, and his original idea of using the PIFrameEmbedding constructor instead of createWindow is untenable.  The way I'm looking to solve this now is via an AttachTo(TabParent*) method on nsFrameLoader, and passing a flag to the right places to indicate that we don't actually want to create a new actor in TryNewProcess().

Comment 5

[Benjamin Smedberg]

Assuming that "the constructor problem" is solved, this message can be sync instead of RPC, right? Is there a bug on solving the constructor problem, which will also affect sync message manager and jetpack messages?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> this message can be sync instead of RPC

I believe it can, yes.

I'm not quite following comment 4.  What exactly is the proposal?  Note that the current createWindow caller needs to get an nsIDOMWindow out of the call, period.  Would that still be the case?

Comment 7

[Josh Matthews [:jdm]]

cjones suggested that nsFrameLoader grow an AttachTo(TabParent*) method which would set mChildProcess to the argument, and do all of the work that TryNewProcess currently does after creating a TabParent.  TryNewProcess would get an argument indicating that it shouldn't create a TabParent, and this flag would be passed magically through the path that RecvcreateWindow takes.  The child would create a PIFrameEmbedding actor, and pass it with the createWindow message.

There's no bug about the constructor work right now; cjones was thinking out loud about how to fix that last night but didn't sound hopeful.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

"The constructor problem" is fixable but I'm not sure it's the right solution.  The problem IMHO is that we're multiplexing two APIs with different requirements over the same mechanism.  The current code expects the frame construction to be requested by chrome, and the PIFrame to be created as a side effect, asynchronous wrt content.  The createWindow() code tries to reuse this mechanism very carefully so that nothing significant happens between creating the frame and returning its reference to content.  I think this use case should be promoted to a first-class API.

Comment 9

[Josh Matthews [:jdm]]

Attachment: WIP

This is the skeleton of what I have in mind.  I cannot figure out how to pass the correct flag from TabParent::CreateWindow to nsFrameLoader::TryNewProcess, however.  bz, smaug, any suggestions?

Comment 10

[Josh Matthews [:jdm]]

Also, I'm not going to be able to work on this for a few days, so if this is a high priority and somebody wants to run with it, then by all means do so.

Comment 11

[Boris Zbarsky [:bzbarsky]]

What flag are we trying to pass?

Comment 12

[Josh Matthews [:jdm]]

We want an indication that TryNewProcess should not actually call CreateTab, as the correct TabParent has already been created and will be attached to the frameloader shortly via the new AttachTo method.

Comment 13

[Josh Matthews [:jdm]]

I don't expect to have any time to work on this in the next two weeks, so I would feel grand if anybody wants to take it.

Comment 14

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

I could try to look at this before the summit. Probably on Thursday.

Comment 15

[Josh Matthews [:jdm]]

This should almost certainly block, as any RPC can totally screw Necko (see bug 559200).

Comment 16

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

And sorry, I didn't have time for this before summit after all and was on
vacation after that.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> We want an indication that TryNewProcess should not actually call CreateTab

I guess you'd need to add API for this on nsIBrowserDOMWindow or whatever that thing is called, and a magic attribute on <browser>.  Or something.  That's the only communications channel in that direction right now.  :(

Comment 18

[Doug Turner (:dougt)]

it is not clear that we would block fennec 2.0 for this.  please renom if you think differently.

Comment 19

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

Attachment: provide-window WIP

I've started working on this. Here's a wip.

Comment 20

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

Attachment: refactor-provide-window

This patch refactors some the path that b2g takes and renames it. I'm going to make desktop use that code in the next patch.

Comment 21

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

Attachment: provide-window

This patch changes CreateWindow to work more like b2g. The new PBrowser is created by the child and passed to the parent. The parent stores it in a global variable and uses it to create the new <browser> element.

It's probably easiest to review this by starting in TabParent.h. That's where the comments are.

It would be nice to avoid the global variable, but it would be pretty hard. We can't pass the PBrowser down to the code that creates the new <browser> element since there are so many layers in between. Josh's patch attaches the PBrowser to the <browser> element only after the <browser> element has been created. That also requires passing something down (a flag instead of the whole PBrowser), and it needs to deal with issues where we want to use the PBrowser inside the <browser> creation code. Ultimately, I think the global variable is not such a bad thing.

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8535928 [details] [diff] [review]
provide-window

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

This looks great Bill!

::: dom/ipc/TabChild.cpp
@@ +1529,5 @@
> +    baseURI->GetSpec(baseURIString);
> +
> +    // We can assume that if content is requesting to open a window from a remote
> +    // tab, then we want to enforce that the new window is also a remote tab.
> +    features.Append(",remote");

AppendLiteral

::: dom/ipc/TabParent.cpp
@@ +397,5 @@
>    target->DispatchEvent(event, &dummy);
>    return true;
>  }
>  
> +struct TabParent::AutoUseNewTab

I'd mark this as MOZ_STACK_CLASS and MOZ_FINAL.

Also, you don't ever seem to touch the members from outside the constructor/destructor, so can this be made into a class so it default-hides those members?

@@ +411,5 @@
> +  ~AutoUseNewTab()
> +  {
> +    mNewTab->mSkipLoad = false;
> +
> +    if (TabParent::sNextTabParent) {

Can you MOZ_ASSERT(sNextTabParent == mNewTab) if it's non-null here?

Comment 23

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

Attachment: final-changes

It took some more work to get all the tests to pass for this patch. With this final patch everything is green.

The big problem is that all the LoadRemoteScript messages that the child used to process in the middle of the CreateWindow rpc are now processed afterwards. Consequently, it's possible for content JS to start running in the window before all frame scripts have been loaded. That's bad.

I think b2g solves this by preloading a bunch of scripts in each new tab. That won't really work for desktop Firefox because add-ons are going to need to load scripts in new tabs and they won't be able to change the preload file.

I solved this by queuing up all the scripts that should be loaded and then sending them down as part of the CreateWindow response.

The other problem is similar. We send down a Show message that includes the value for window.name. This used to be received in the middle of the CreateWindow rpc. Afterwards, some DOM code that's invoked during window.open would override window.name with the correct value. Now the Show message is received later. It sets window.name to the value of the browser's name attribute. This seems to be what b2g needs, but it's definitely wrong for desktop. So I just skipped that part for desktop.

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8547749 [details] [diff] [review]
final-changes

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

r=me with these:

::: dom/base/nsFrameLoader.cpp
@@ +2304,5 @@
>  
>  bool
>  nsFrameLoader::DoLoadFrameScript(const nsAString& aURL, bool aRunInGlobalScope)
>  {
> +  auto tabParent = static_cast<TabParent*>(GetRemoteBrowser());

Nit: I prefer |auto*| for pointers, but that's just me

::: dom/ipc/TabChild.cpp
@@ +1586,5 @@
>    newChild->DoFakeShow(scrolling, textureFactoryIdentifier, layersId, renderFrame);
>  
> +  for (size_t i = 0; i < frameScripts.Length(); i++) {
> +    FrameScriptInfo& info = frameScripts[i];
> +    newChild->RecvLoadRemoteScript(info.url(), info.runInGlobalScope());

You probably should handle this returning false somehow, at least breaking out of the loop I'd think? But probably just MOZ_CRASH'ing.

::: dom/ipc/TabParent.cpp
@@ +601,5 @@
> +    mDelayedFrameScripts.AppendElement(FrameScriptInfo(aURL, aRunInGlobalScope));
> +    return true;
> +  }
> +
> +  return PBrowserParent::SendLoadRemoteScript(aURL, aRunInGlobalScope);

Maybe assert here that mDelayedFrameScripts is empty?

Comment 25

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4a1171cc4dac
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/66e037e5ff7d

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a1171cc4dac
https://hg.mozilla.org/mozilla-central/rev/66e037e5ff7d
https://hg.mozilla.org/mozilla-central/rev/6295dbaa06de

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Backed out for being the likely cause of bug 1120879 and bug 1120898. I suspect bug 1115144 is also related to those bugs, so maybe these patches just exacerbated the underlying problem?
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ddc69b3693a

Comment 28

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/613d80a116b4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8798cd000e6b

Landed again now that bug 1121713 is fixed.

Comment 29

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

https://hg.mozilla.org/mozilla-central/rev/613d80a116b4
https://hg.mozilla.org/mozilla-central/rev/8798cd000e6b