Bug 1251032 - Have ContentParent send RenderFrameInfo down when responding to the CreateWindow sync message
6:02 PM < & billm > mconley: I guess you would send the RenderFrameConstructor message earlier
6:02 PM < mconley > billm: or could I send it down from the parent just before responding to the CreateWindow in the parent?
6:02 PM < mconley > I forget if IPC lets me do that
6:03 PM < & billm > mconley: hmm, then it will arrive asynchronously, which is a problem
6:03 PM < & billm > (in the child)
6:03 PM < mconley > ah, gotcha
6:04 PM ⇐ ehsan quit (ehsan@66.207.193.21) A TLS packet with unexpected length was received.
6:04 PM < mconley > okay, fair enough - I'll send it earlier
6:04 PM < mconley > (from the child)
6:04 PM < & billm > mconley: actually, you can't send it earlier I don't think
6:04 PM < mconley > oh
6:04 PM < & billm > mconley: it relies on there being a frameloader: http://searchfox.org/mozilla-central/source/dom/ipc/TabParent.cpp#2638
6:04 PM bholley → bholley_away
6:04 PM < & billm > mconley: and I think that won't exist until CreateWindow finishes
6:04 PM ⇐ marcosc quit (marcosc@moz-29a.do1.52.113.IP) Ping timeout: 121 seconds
6:04 PM < & billm > yuck
6:05 PM < mconley > hrm
6:06 PM < mconley > this is exactly the sort of up-front stuff I wanted to hear about, so thanks. 😃

6:06 PM < & billm > mconley: you can make the constructor message for PRenderFrame have urgent priority (not sure if that's possible, but we could make it work)
6:07 PM < & billm > mconley: then you can return the PRenderFrame as a result of CreateWindow
6:07 PM < & billm > mconley: the urgent property means that the child will process the AllocPRenderFrame message while it's waiting for the CreateWindow reply
6:07 PM < & billm > mconley: that's horrible and we really need to rewrite this code, but I can't think of anything better that would be quick
6:08 PM < & billm > mconley: step 1 is to try to add prio(urgent) in front of the PRenderFrame constructor message in IPDL and see if that works

Okay, this is tricky. I really don’t want to disturb the ecosystem here, so I’m looking for something minimal that will give me what I need.

My WIP is kinda… invasive. Can I pare it down any?

What does it do exactly?

  1. Adds the RenderFrame to the set of things passed up to CreateWindow and adds TextureFactoryIdentifier and a layersId to what is returned from CreateWindow from the parent
  2. Publicly expose methods on TabParent to get at the FrameLoader and the RecvGetRenderFrameInfo method, along with the AddTabParentToTable method.
  3. Add an “Init” method to RenderFrameParent that allows us to endow an instance with information after it has been instantiated.
  4. Reorganize ContentChild so that it sends up a RenderFrame constructor first so that it can pass the RenderFrameChild up with SendCreateWindow. Needs to be able to handle failures, and send a delete for the RenderFrameChild if things go wrong.

I think the most egregious thing about this patch is how we expose things on TabParent, like the AddTabParentToTable, the RecvGetRenderFrameInfo, and the FrameLoader getter. I think what we should do instead of exposing these things is give TabParent a new public method for just this case, so that it can call those things internally. Otherwise, it’s TMI for callers.

So let’s see what the path is here...

From billm :

"
Here's a way to do this:

1. Send up a new RenderFrame before the CreateWindow call. We won't have a frameloader yet, so the AllocPRenderFrame code won't really do anything.
2. Pass the new PRenderFrame to CreateWindow. CreateWindow will fill in the RenderFrameParent with everything it normally gets from frameloader.
3. CreateWindow returns any other information needed, like the texture factory identifier and layer tree ID.

This is kinda hacky, but at least it doesn't rely on weird IPC gymnastics."

Wait a second. How come I can’t just have the CreateWindow constructor return the RenderFrame? Like… can’t I have ContentParent construct it after the forceInitialBrowserRemote thing, and then send it down? If not, why not?

Ah, because like billm pointed out, that would mean upping the priority of the PRenderFrame construction message so that it’s processed before the CreateWindow proceeds, and that’s scary. Messages should be processed in order where possible.

Okay, so that idea is out.

Here’s what it’s starting to look like:
Send construction message for RenderFrame first, before the FrameLoader even exists.
Create a TabParent::SetRenderFrame or TabParent::InitRenderFrame, passing it the RenderFrame that was sent from the Child up to us
Implement Set/InitRenderFrame to do the work that I didn’t want exposed in (2) above.

Okay, now I want to split this patch up to make it easier to review:

Remove layerId and TextureFactoryIdentifier from RenderFrameParent constructor
Add Init method to RenderFrameParent
Make it so that the child creates the RenderFrame _before_ the FrameLoader is available, and combine into PContent’s CreateWindow

Oranges and other test failure thingies:

Debug only
dom/browser-element/mochitest/test_browserElement_oop_OpenNamed.html

Failing this assertion: MOZ_ASSERT((!mDidFakeShow && aRenderFrame) || (mDidFakeShow && !aRenderFrame));
A || B == !A && !B

AHHHHH - I see what’s happening here. We’re entering the mozbrowser block above the part where I did all of the render frame manipulation. This thing:

if (aIframeMoz) {
MOZ_ASSERT(aTabOpener);
newChild->SendBrowserFrameOpenWindow(aTabOpener, NS_ConvertUTF8toUTF16(url),
name, NS_ConvertUTF8toUTF16(features),
aWindowIsNew);
} else {


From here: https://hg.mozilla.org/mozilla-central/file/d1d47ba19ce9/dom/ipc/ContentChild.cpp#l858



dom/browser-element/mochitest/test_browserElement_oop_FrameWrongURI.html

Debug and opt
dom/browser-element/mochitest/test_browserElement_inproc_OpenMixedProcess.html
dom/browser-element/mochitest/test_browserElement_oop_AppWindowNamespace.html