Bug 70836 – [chromium] add API methods to support renderer swaps

• Component: WebKit API
• Status: RESOLVED WONTFIX

[Karl Koscher]

Chromium now "swaps out" the current renderer on a cross-site request instead of destroying it. This allows the renderer to be swapped back in if the user navigates back to the original site, preserving window.opener (which would be null if we just did another process swap -- see http://crbug.com/65953). To perform a swap out, we need to unload the current document in the renderer to prevent any scripts or plugins from continuing to execute. Currently, this is done with a hack where the renderer is navigated to about:swappedout and spurious IPC messages generated by this navigation are filtered out. However, a cleaner way of swapping out a renderer is to just unload the current document, but no WebKit API exists for doing so. Adding a swapOut method to Chromium's WebFrame that would unload the current document would allow us to simplify the swap out code.

[Karl Koscher]

Created attachment 112395 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Charles Reis]

For reference, the Chrome side of this issue is http://crbug.com/101393.  Once this part lands, we can simplify the logic in Chrome for swapping out a renderer (such as removing the IPC message filtering).

[James Robinson]

Comment on attachment 112395 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112395&action=review

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

we don't do this, leave the date alone

[Darin Fisher (:fishd, Google)]

Comment on attachment 112395 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112395&action=review

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1059
> +void WebFrameImpl::swapOut()

I think this function belongs in the Chromium repository instead.  We should just
expose the necessary primitives through the WebKit API here.  For example, I think
it makes sense to have a WebDocument::create() function and a WebFrame::setDocument
function.  Perhaps WebFrame::setDocument can call m_frame->loader()->clear() before
changing the document?

Basically, the concept of "swapping out" doesn't belong in WebKit.

[Karl Koscher]

> I think this function belongs in the Chromium repository instead.  We should just
> expose the necessary primitives through the WebKit API here.  For example, I think
> it makes sense to have a WebDocument::create() function and a WebFrame::setDocument
> function.  Perhaps WebFrame::setDocument can call m_frame->loader()->clear() before
> changing the document?
> 
> Basically, the concept of "swapping out" doesn't belong in WebKit.

I agree that it seems strange to put this in WebKit. However, this is just an interm step towards the long-term goal of getting postMessage to work across process boundaries (http://crbug.com/99202). The way we're thinking of implementing this is to point a swapped out WebCore::Frame to a proxy DOMWindow object that knows how to redirect postMessages (and possibly other allowed cross-origin calls) to chromium. It seems like the cleanest way to do this is to put the proxy DOMWindow in WebKit/chromium, which means that we need a way of constructing one from chromium. The plan was to have the swapOut method construct and install this proxy DOMWindow.

As strange as that may seem, it seems even stranger to add the proxy DOMWindow to chromium. The point of the chromium port in WebKit is to expose
a clean API between chromium and WebKit, and putting the proxy DOMWindow in
chromium would smear that clean boundary.

Of course, if you have other ideas on how to proceed, I'd certainly welcome them!

[Darin Fisher (:fishd, Google)]

(In reply to comment #6)
> [snip] The plan was to have the swapOut method construct and install this proxy DOMWindow.

Can you describe how that code looks?  It might help inform us about the best API entry points.

[Karl Koscher]

(In reply to comment #7)
> Can you describe how that code looks?  It might help inform us about the best API entry points.

I got something nearly up and running by doing the following:

1. Virtualize postMessage in the DOMWindow class.
2. Create a ProxyDOMWindow class in WebKit/chromium/src that extends DOMWindow. The class contains an identifier to reach the actual frame the proxy is for.
3. Extend swapOut to take the new frame identifier, create a ProxyDOMWindow with it, and set it to be the frame's window. 
4. Add a method to WebFrameClient.h to send a postMessage to a remote frame.
5. Override postMessage in ProxyDOMWindow to call the new method in WebFrameClient.
6. Add plumbing in Chromium to send the postMessage to the right process/frame.

The missing piece was to actually deliver the postMessage event. There were also some object lifetime issues to work out...

[Karl Koscher]

Created attachment 113578 [details]
Patch

[Karl Koscher]

As per the discussion with Charlie and Darin, I'm changing this to add API calls (clearDocument, setDocument, and WebDocument::create) that support this swapping out behavior from the chromium side.

[Darin Fisher (:fishd, Google)]

Comment on attachment 113578 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113578&action=review

I'm curious why you require these APIs.  In our meeting, we talked about simply truncating the data stream for a cross-origin document load.  Why did you decide not to go that route?

> Source/WebKit/chromium/public/WebDocument.h:75
> +    WEBKIT_EXPORT static WebDocument create(const WebString& MIMEType,

nit: mimeType <-- webkit style

> Source/WebKit/chromium/public/WebDocument.h:76
> +                                            const WebFrame*,

nit: I'd probably list the WebFrame parameter first since it is the container for the WebDocument.

> Source/WebKit/chromium/public/WebDocument.h:77
> +                                            const WebURL&,

By passing a WebURL here, it seems like it is possible for the WebFrame and the WebDocument to have different ideas about what URL is loaded in the frame.  Is that really helpful?  It seems like it could cause subtle bugs.  Another idea would be to infer the URL of the document from the frame.

> Source/WebKit/chromium/public/WebDocument.h:78
> +                                            bool inViewSourceMode);

nit: is it strictly necessary to pass the inViewSourceMode parameter here?  can't you just use WebFrame::enableViewSourceMode(true)?

> Source/WebKit/chromium/src/WebDocument.cpp:70
> +        MIMEType, frame ? static_cast<const WebFrameImpl*>(frame)->frame() : 0,

Do you have a use case for frame being null here?  Maybe we should require that it is non-null.

[Karl Koscher]

Comment on attachment 113578 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113578&action=review

We decided to implement swapping out this way because it lets us get rid of the renderer-side IPC message filtering. If we were to just send back an blank cross-site response, we'd still need to filter out all of the navigation messages the renderer would send.

>> Source/WebKit/chromium/public/WebDocument.h:75
>> +    WEBKIT_EXPORT static WebDocument create(const WebString& MIMEType,
> 
> nit: mimeType <-- webkit style

Fixed.

>> Source/WebKit/chromium/public/WebDocument.h:76
>> +                                            const WebFrame*,
> 
> nit: I'd probably list the WebFrame parameter first since it is the container for the WebDocument.

Fixed.

>> Source/WebKit/chromium/public/WebDocument.h:77
>> +                                            const WebURL&,
> 
> By passing a WebURL here, it seems like it is possible for the WebFrame and the WebDocument to have different ideas about what URL is loaded in the frame.  Is that really helpful?  It seems like it could cause subtle bugs.  Another idea would be to infer the URL of the document from the frame.

Where does the WebFrame keep the current URL? I couldn't find anywhere that it does...

>> Source/WebKit/chromium/public/WebDocument.h:78
>> +                                            bool inViewSourceMode);
> 
> nit: is it strictly necessary to pass the inViewSourceMode parameter here?  can't you just use WebFrame::enableViewSourceMode(true)?

Fixed.

>> Source/WebKit/chromium/src/WebDocument.cpp:70
>> +        MIMEType, frame ? static_cast<const WebFrameImpl*>(frame)->frame() : 0,
> 
> Do you have a use case for frame being null here?  Maybe we should require that it is non-null.

No, I don't. Fixed.

[Karl Koscher]

Created attachment 115461 [details]
Patch

[Darin Fisher (:fishd, Google)]

(In reply to comment #12)
> We decided to implement swapping out this way because it lets us get rid of the renderer-side IPC message filtering. If we were to just send back an blank cross-site response, we'd still need to filter out all of the navigation messages the renderer would send.

I see.


> >> Source/WebKit/chromium/public/WebDocument.h:77
> >> +                                            const WebURL&,
> > 
> > By passing a WebURL here, it seems like it is possible for the WebFrame and the WebDocument to have different ideas about what URL is loaded in the frame.  Is that really helpful?  It seems like it could cause subtle bugs.  Another idea would be to infer the URL of the document from the frame.
> 
> Where does the WebFrame keep the current URL? I couldn't find anywhere that it does...

Sorry, it is not so direct.  I was thinking of webFrame->dataSource()->request().url()

I'm curious what it means for us to change the Document without updating the DocumentLoader's state (URL, etc.).

[Karl Koscher]

(In reply to comment #14)
> Sorry, it is not so direct.  I was thinking of webFrame->dataSource()->request().url()
> 
> I'm curious what it means for us to change the Document without updating the DocumentLoader's state (URL, etc.).

I don't know yet, and that worries me. I'll look into that soon. I'm worried that keeping the DocumentLoader state in sync will cause various navigation events to fire, which is precisely why we wanted to move away from simply navigating to about:swappedout...

This is not a blocker for cross-process postMessage, though.

[Ojan Vafai]

Comment on attachment 115461 [details]
Patch

From the last comment, it sounds like this is stalled on understanding "what it means for us to change the Document without updating the DocumentLoader's state". r- for now to remove it from the review queue while that is figured out.

[Charles Reis]

Yes, I think we've decided not to do this, given the complexity of dealing with the DocumentLoader.  We'll stick with navigating the frame to a blank, unscriptable origin, since that's reasonably well understood at this point.