Bug 70836 - [chromium] add API methods to support renderer swaps
Summary: [chromium] add API methods to support renderer swaps
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://crbug.com/101393
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-25 12:24 PDT by Karl Koscher
Modified: 2012-04-19 16:54 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.35 KB, patch)
2011-10-25 13:51 PDT, Karl Koscher
no flags Details | Formatted Diff | Diff
Patch (5.04 KB, patch)
2011-11-03 16:38 PDT, Karl Koscher
no flags Details | Formatted Diff | Diff
Patch (4.97 KB, patch)
2011-11-16 15:28 PST, Karl Koscher
ojan: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Karl Koscher 2011-10-25 12:24:08 PDT
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.
Comment 1 Karl Koscher 2011-10-25 13:51:29 PDT
Created attachment 112395 [details]
Patch
Comment 2 WebKit Review Bot 2011-10-25 13:57:36 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Charles Reis 2011-10-25 13:58:28 PDT
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).
Comment 4 James Robinson 2011-10-25 17:39:22 PDT
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
Comment 5 Darin Fisher (:fishd, Google) 2011-10-25 23:45:02 PDT
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.
Comment 6 Karl Koscher 2011-10-28 11:05:06 PDT
> 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!
Comment 7 Darin Fisher (:fishd, Google) 2011-10-31 11:30:09 PDT
(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.
Comment 8 Karl Koscher 2011-10-31 13:46:54 PDT
(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...
Comment 9 Karl Koscher 2011-11-03 16:38:21 PDT
Created attachment 113578 [details]
Patch
Comment 10 Karl Koscher 2011-11-03 16:40:58 PDT
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.
Comment 11 Darin Fisher (:fishd, Google) 2011-11-16 14:50:32 PST
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.
Comment 12 Karl Koscher 2011-11-16 15:25:26 PST
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.
Comment 13 Karl Koscher 2011-11-16 15:28:18 PST
Created attachment 115461 [details]
Patch
Comment 14 Darin Fisher (:fishd, Google) 2011-11-18 15:04:40 PST
(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.).
Comment 15 Karl Koscher 2011-12-01 13:17:22 PST
(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.
Comment 16 Ojan Vafai 2012-04-19 16:44:16 PDT
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.
Comment 17 Charles Reis 2012-04-19 16:54:48 PDT
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.