Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11193051:
To fix the cross-site post submission bug.
|
Created:
8 years, 2 months ago by irobert Modified:
6 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTo fix the cross-site post submission bug.
BUG=101395
Patch Set 1 #Patch Set 2 : Fix FileRead Permission #
Total comments: 31
Patch Set 3 : Reuse ResourceRequestBody Struct #
Total comments: 41
Patch Set 4 : Fix Design #Patch Set 5 : Fix Structure and Tests #
Total comments: 14
Patch Set 6 : New OpenURL function and DataType Test #
Total comments: 4
Patch Set 7 : Fix Comments #
Total comments: 38
Patch Set 8 : Fix Android Test and Include_rules #Patch Set 9 : Fix Android API, Helper Function and Include_rules #Messages
Total messages: 23 (0 generated)
Prototype Only. Welcome to any suggestions on design
Hi Robert, I've been taking a look at this CL to come up to speed. I know it's not really ready yet for a review, but here are my comments from browsing what you've got up there right now. http://codereview.chromium.org/11193051/diff/17001/chrome/browser/ui/browser_... File chrome/browser/ui/browser_navigator.h (right): http://codereview.chromium.org/11193051/diff/17001/chrome/browser/ui/browser_... chrome/browser/ui/browser_navigator.h:211: std::vector<content::WebHTTPPOSTBodyParams> post_data; How is this construct different than ResourceRequestBody::Element? http://codereview.chromium.org/11193051/diff/17001/content/browser/renderer_h... File content/browser/renderer_host/render_view_host_delegate.h (right): http://codereview.chromium.org/11193051/diff/17001/content/browser/renderer_h... content/browser/renderer_host/render_view_host_delegate.h:433: std::vector<content::WebHTTPPOSTBodyParams> post_data) {} post_data probably shouldn't be passed by value http://codereview.chromium.org/11193051/diff/17001/content/browser/renderer_h... content/browser/renderer_host/render_view_host_delegate.h:441: std::vector<content::WebHTTPPOSTBodyParams> post_data) {} ditto http://codereview.chromium.org/11193051/diff/17001/content/browser/renderer_h... File content/browser/renderer_host/render_view_host_impl.cc (right): http://codereview.chromium.org/11193051/diff/17001/content/browser/renderer_h... content/browser/renderer_host/render_view_host_impl.cc:1380: true, /* is_post */ Is the bool param needed for this method which seems to imply POST already? http://codereview.chromium.org/11193051/diff/17001/content/browser/renderer_h... File content/browser/renderer_host/render_view_host_impl.h (right): http://codereview.chromium.org/11193051/diff/17001/content/browser/renderer_h... content/browser/renderer_host/render_view_host_impl.h:525: std::vector<content::WebHTTPPOSTBodyParams> post_data); ditto, probably don't want to pass by value http://codereview.chromium.org/11193051/diff/17001/content/browser/web_conten... File content/browser/web_contents/render_view_host_manager.cc (right): http://codereview.chromium.org/11193051/diff/17001/content/browser/web_conten... content/browser/web_contents/render_view_host_manager.cc:852: ChildProcessSecurityPolicyImpl* policy = Is the 'policy' thread safe? Just checking because i think this smart collection class gets accessed on the IO thread too. And if it is thread safe, any chance for raciness around when access is granted to the 'old' compared to when we're checking to migrate access to the 'new'? http://codereview.chromium.org/11193051/diff/17001/content/browser/web_conten... File content/browser/web_contents/web_contents_impl.cc (right): http://codereview.chromium.org/11193051/diff/17001/content/browser/web_conten... content/browser/web_contents/web_contents_impl.cc:1649: navigate_params.post_data = entry.post_data; another copy http://codereview.chromium.org/11193051/diff/17001/content/browser/web_conten... File content/browser/web_contents/web_contents_impl.h (right): http://codereview.chromium.org/11193051/diff/17001/content/browser/web_conten... content/browser/web_contents/web_contents_impl.h:487: std::vector<content::WebHTTPPOSTBodyParams> post_data) OVERRIDE; ditto pass by value http://codereview.chromium.org/11193051/diff/17001/content/public/common/fram... File content/public/common/frame_navigate_params.h (right): http://codereview.chromium.org/11193051/diff/17001/content/public/common/fram... content/public/common/frame_navigate_params.h:39: // TODO: Will Move to a new struct only for headers Move the |method| data member too? http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... File content/renderer/render_view_impl.cc (right): http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... content/renderer/render_view_impl.cc:1106: content::old::GuestToEmbedderChannel* embedder_channel = oh... interesting... this is a direct channel btwn two renderers or is this channel mediated by the main browser process... hmmm, may open up the possibility of raciness http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... content/renderer/render_view_impl.cc:2848: OpenURL(frame, url, referrer, default_policy); I see, so in navigations that get initiated in one renderer and satisfied in another... we cancel the webcore navigation by returning WebNavigationPolicyIgnore? Is this the only mechanism for process swapping navigations where the navigation is initiated by webcore? http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... content/renderer/render_view_impl.cc:2897: OpenPostURL(frame, url, referrer, default_policy, post_data); return WebKit::WebNavigationPolicyIgnore here? http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... content/renderer/render_view_impl.cc:2918: OpenURL(frame, url, referrer, default_policy); POST case here too? http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... File content/renderer/render_view_impl.h (right): http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... content/renderer/render_view_impl.h:905: std::vector<content::WebHTTPPOSTBodyParams> post_data); ditto nix pass by value
http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... File content/renderer/render_view_impl.cc (right): http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... content/renderer/render_view_impl.cc:2848: OpenURL(frame, url, referrer, default_policy); I haven't looked at the rest of this CL yet, but I saw this question and wanted to chime in. Yes, this is currently the only way that we punt a navigation to the browser process from the renderer process. (Eventually I'd like the browser process to do the intercepting rather than relying on the somewhat-untrusted renderer, but that's a different project.) On 2012/10/23 23:22:18, michaeln wrote: > I see, so in navigations that get initiated in one renderer and satisfied in > another... we cancel the webcore navigation by returning > WebNavigationPolicyIgnore? Is this the only mechanism for process swapping > navigations where the navigation is initiated by webcore?
http://codereview.chromium.org/11193051/diff/17001/chrome/browser/ui/browser_... File chrome/browser/ui/browser_navigator.h (right): http://codereview.chromium.org/11193051/diff/17001/chrome/browser/ui/browser_... chrome/browser/ui/browser_navigator.h:211: std::vector<content::WebHTTPPOSTBodyParams> post_data; This is the exactly same as the ResourceRequestBody::Element. My purpose of doing this is to pass all the elements in the post request to the new process through IPC. Since you cannot pass an instance of a class through IPC (based on my current understanding of the code.), I create this struct and convert each element to this. On 2012/10/23 23:22:18, michaeln wrote: > How is this construct different than ResourceRequestBody::Element? http://codereview.chromium.org/11193051/diff/17001/content/browser/renderer_h... File content/browser/renderer_host/render_view_host_impl.cc (right): http://codereview.chromium.org/11193051/diff/17001/content/browser/renderer_h... content/browser/renderer_host/render_view_host_impl.cc:1380: true, /* is_post */ Yes. You are right. On 2012/10/23 23:22:18, michaeln wrote: > Is the bool param needed for this method which seems to imply POST already?
Thnx, that's good to know. > http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... > content/renderer/render_view_impl.cc:2848: OpenURL(frame, url, referrer, > default_policy); > I haven't looked at the rest of this CL yet, but I saw this question and wanted > to chime in. Yes, this is currently the only way that we punt a navigation to > the browser process from the renderer process. (Eventually I'd like the browser > process to do the intercepting rather than relying on the somewhat-untrusted > renderer, but that's a different project.) > > On 2012/10/23 23:22:18, michaeln wrote: > > I see, so in navigations that get initiated in one renderer and satisfied in > > another... we cancel the webcore navigation by returning > > WebNavigationPolicyIgnore? Is this the only mechanism for process swapping > > navigations where the navigation is initiated by webcore?
http://codereview.chromium.org/11193051/diff/17001/chrome/browser/ui/browser_... File chrome/browser/ui/browser_navigator.h (right): http://codereview.chromium.org/11193051/diff/17001/chrome/browser/ui/browser_... chrome/browser/ui/browser_navigator.h:211: std::vector<content::WebHTTPPOSTBodyParams> post_data; Looks like ResourceRequestBody and it's Element data type do have ipc message traits so they can be passed around in messages. If possible, it'd be better to reuse that type instead of creating a new type here. http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... File content/renderer/render_view_impl.cc (right): http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... content/renderer/render_view_impl.cc:1260: main_frame->loadRequest(request); I see... this is where the new request is initiated... it does involve sending the data to be posted to the destination renderer. If we had a WebHTTPBody Element that meant "that browser initiated post data you already know about and have sitting around in the browser process" we could avoid the extra round trip of post data. As coded postdata flows like this... - src renderer sends postdata to browser via the OnOpenPostURL. - browser stores it in NavigationEntryImpl - browser sends it to dst renderer via OnNavigate - dst renderer sends it back to browser via WebRequest - browser sends it over the network (hooray) If the WebRequest could refer to the stored copy, we could cut out a round trip. Also, this is where interpreting TypeURL (aka filesystem:// urls) could get difficult. If the source and destination renderers use different storage partitions, a filesystem://<origin>/<path> in one partition does not exist in the other partition. Is it possible to cross-process post across storage partition boundaries? If so, ResourceDispatcherHost will have to take that into account when getting actual bytes to upload in the browser process. And when does the stored copy of the postdata get cleared of the NavigationEntryImpl?
http://codereview.chromium.org/11193051/diff/17001/content/browser/renderer_h... File content/browser/renderer_host/render_view_host_delegate.h (right): http://codereview.chromium.org/11193051/diff/17001/content/browser/renderer_h... content/browser/renderer_host/render_view_host_delegate.h:433: std::vector<content::WebHTTPPOSTBodyParams> post_data) {} On 2012/10/23 23:22:18, michaeln wrote: > post_data probably shouldn't be passed by value Done. http://codereview.chromium.org/11193051/diff/17001/content/browser/renderer_h... content/browser/renderer_host/render_view_host_delegate.h:441: std::vector<content::WebHTTPPOSTBodyParams> post_data) {} On 2012/10/23 23:22:18, michaeln wrote: > ditto Done. http://codereview.chromium.org/11193051/diff/17001/content/browser/renderer_h... File content/browser/renderer_host/render_view_host_impl.h (right): http://codereview.chromium.org/11193051/diff/17001/content/browser/renderer_h... content/browser/renderer_host/render_view_host_impl.h:525: std::vector<content::WebHTTPPOSTBodyParams> post_data); On 2012/10/23 23:22:18, michaeln wrote: > ditto, probably don't want to pass by value Done. http://codereview.chromium.org/11193051/diff/17001/content/browser/web_conten... File content/browser/web_contents/render_view_host_manager.cc (right): http://codereview.chromium.org/11193051/diff/17001/content/browser/web_conten... content/browser/web_contents/render_view_host_manager.cc:852: ChildProcessSecurityPolicyImpl* policy = Not quite sure about it. But i do believe this could be problem. I will further dig into it. On 2012/10/23 23:22:18, michaeln wrote: > Is the 'policy' thread safe? Just checking because i think this smart collection > class gets accessed on the IO thread too. And if it is thread safe, any chance > for raciness around when access is granted to the 'old' compared to when we're > checking to migrate access to the 'new'? http://codereview.chromium.org/11193051/diff/17001/content/browser/web_conten... File content/browser/web_contents/web_contents_impl.h (right): http://codereview.chromium.org/11193051/diff/17001/content/browser/web_conten... content/browser/web_contents/web_contents_impl.h:487: std::vector<content::WebHTTPPOSTBodyParams> post_data) OVERRIDE; On 2012/10/23 23:22:18, michaeln wrote: > ditto pass by value Done. http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... File content/renderer/render_view_impl.h (right): http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... content/renderer/render_view_impl.h:905: std::vector<content::WebHTTPPOSTBodyParams> post_data); On 2012/10/23 23:22:18, michaeln wrote: > ditto nix pass by value Done.
Not a complete review but some comments. I suspect should explicitly not handle the odd form data elements (blobs and filesystemurl) for now. Instead of having logic that looks like it might be correct but really might not be, how about we put CHECK(false) when a form with those element types are presented to the renderer for cross-site post consideration. Either that or neglect to satisfy such submissions via the cross-site mechanism and just handling them inprocess as usual. Would help to divide and conquer. Get the base case working first then deal with the additional cases later. It's clear you've looked closely at the existing cross-site OpenURL stuff and followed suit. It's less clear that you've looked at the existing browser_initiated_post_data_ stuff which seems similar to what your trying to do in other ways. Maybe you have already, but if not, i think its worth understand that existing use case to help inform your work on this new case. Finally, ViewMsg_Request is not a good name for your new struct for a couple of reasons. * usually we name IPC message struct like that where the struct === a particular ipc msg, this struct is not like that * also, i think the intent is a container for the post data + the content type (and arguably + the method), the struct name you have is too generic for this But, I think that struct might melt away after some code massaging? http://codereview.chromium.org/11193051/diff/30004/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/11193051/diff/30004/chrome/browser/ui/browser.... chrome/browser/ui/browser.cc:226: struct ViewMsg_Request; is this needed, given the assignment below, seems like the compiler must know more about the type than just this forward decl? http://codereview.chromium.org/11193051/diff/30004/chrome/browser/ui/browser_... File chrome/browser/ui/browser_navigator.h (right): http://codereview.chromium.org/11193051/diff/30004/chrome/browser/ui/browser_... chrome/browser/ui/browser_navigator.h:23: using content::ViewMsg_Request; please avoid the use of using declarations in .h files to avoid potential namespace collisions http://codereview.chromium.org/11193051/diff/30004/chrome/browser/ui/browser_... chrome/browser/ui/browser_navigator.h:209: ViewMsg_Request request; There's some overlap between member of tha NavigateParams struct and members of the new 'request' struct, in particular the 'url' members. The new info that's missing from this struct seems to be the post data and the content type. I wonder if the content type can go in the extra_headers field? The new transition type implies 'POST' so not really sure we need the method string? http://codereview.chromium.org/11193051/diff/30004/content/browser/renderer_h... File content/browser/renderer_host/render_view_host_delegate.h (right): http://codereview.chromium.org/11193051/diff/30004/content/browser/renderer_h... content/browser/renderer_host/render_view_host_delegate.h:433: ViewMsg_Request request) {} please avoid pass by value (with non primitive typess) where possible, prefer const T& references http://codereview.chromium.org/11193051/diff/30004/content/browser/renderer_h... File content/browser/renderer_host/render_view_host_impl.h (right): http://codereview.chromium.org/11193051/diff/30004/content/browser/renderer_h... content/browser/renderer_host/render_view_host_impl.h:521: const ViewMsg_Request& request); hooray, const ref! http://codereview.chromium.org/11193051/diff/30004/content/browser/web_conten... File content/browser/web_contents/navigation_entry_impl.h (right): http://codereview.chromium.org/11193051/diff/30004/content/browser/web_conten... content/browser/web_contents/navigation_entry_impl.h:18: struct ViewMsg_Request; since this struct is defined in frame_navigate_params.h, is this forward decl needed? http://codereview.chromium.org/11193051/diff/30004/content/browser/web_conten... content/browser/web_contents/navigation_entry_impl.h:172: ViewMsg_Request request; nits: we put trailing underbars on data members_ and for classes prefer data members to be private http://codereview.chromium.org/11193051/diff/30004/content/browser/web_conten... content/browser/web_contents/navigation_entry_impl.h:172: ViewMsg_Request request; Also, ditto comments about overlap with the existing structure. http://codereview.chromium.org/11193051/diff/30004/content/browser/web_conten... content/browser/web_contents/navigation_entry_impl.h:207: scoped_refptr<const base::RefCountedMemory> browser_initiated_post_data_; I think you may want to harmonize your new more expressive POST handling with this older more basic POST data handling. Having two means of handling POSTs in parallel with one another will leave folks wondering which one to use and why? Maybe... scoped_refptr<const base::RefCountedMemory> browser_initiated_post_data_; vs scoped_refptr<const T> content_initiated_cross_site_post_data_; Also, how closely have you reviewed the browser_initiated_post_data handling? The details about when this field is cleared and how it interacts with session restore seem relevant to your new crosssite form posting case. http://codereview.chromium.org/11193051/diff/30004/content/common/view_messag... File content/common/view_messages.h (right): http://codereview.chromium.org/11193051/diff/30004/content/common/view_messag... content/common/view_messages.h:752: IPC_STRUCT_MEMBER(std::vector<unsigned char>, browser_initiated_post_data) I think you're adding a new more expressive way to navigate by way of POST. How do we reconcile the new way with what looks like an existing less expressive way? http://codereview.chromium.org/11193051/diff/30004/content/common/view_messag... content/common/view_messages.h:1919: content::ViewMsg_Request /* Request */) the request param also contains a url data member, is either the param or the data member in the request struct redundant? http://codereview.chromium.org/11193051/diff/30004/content/public/browser/pag... File content/public/browser/page_navigator.h (right): http://codereview.chromium.org/11193051/diff/30004/content/public/browser/pag... content/public/browser/page_navigator.h:70: ViewMsg_Request request; There's some overlap between member of tha OpenURLParams struct and members of the new 'request' struct, in particular the 'url' members. The new info that's missing from this struct seems to be the post data and the content type. I wonder if the content type can go in the extra_headers field? The new transition type implies 'POST' so not really sure we need the method string? http://codereview.chromium.org/11193051/diff/30004/content/public/common/fram... File content/public/common/frame_navigate_params.h (right): http://codereview.chromium.org/11193051/diff/30004/content/public/common/fram... content/public/common/frame_navigate_params.h:31: scoped_refptr<webkit_glue::ResourceRequestBody> request_body; Would these additional params make more sense as additional params of the FrameNavigateParams struct? http://codereview.chromium.org/11193051/diff/30004/content/renderer/render_vi... File content/renderer/render_view_impl.cc (right): http://codereview.chromium.org/11193051/diff/30004/content/renderer/render_vi... content/renderer/render_view_impl.cc:1172: // this part have't been testes. Q: do cross-site posts ever cross storage partition boundaries? http://codereview.chromium.org/11193051/diff/30004/content/renderer/render_vi... content/renderer/render_view_impl.cc:2799: else { style nit : } else { http://codereview.chromium.org/11193051/diff/30004/content/renderer/render_vi... content/renderer/render_view_impl.cc:2816: const FilePath::StringType kFilePath = FILE_PATH_LITERAL( use of FILE_PATH_LITERAL seems odd since the value here is not a literal? http://codereview.chromium.org/11193051/diff/30004/content/renderer/render_vi... content/renderer/render_view_impl.cc:2860: params.headers.insert(std::pair<std::string,std::string>( if there's at most one header value in here, maybe we don't need a whole headers struct for it and instead could just sent contentType as an explicit msg param http://codereview.chromium.org/11193051/diff/30004/content/renderer/render_vi... content/renderer/render_view_impl.cc:2864: return default_policy; return default_policy? Is that right? Seems like this s/b WebKit::WebNavigationPolicyIgnor since by calling OpenPostURL we've just asked the browser process to handle this post in a different renderer. By returning default, does will this POST happens twice, once in this renderer and again in the other renderer per OpenPostURL processing? http://codereview.chromium.org/11193051/diff/30004/content/renderer/render_vi... content/renderer/render_view_impl.cc:2885: OpenURL(frame, url, referrer, default_policy); What is the difference between the cross-site handling above (where you've extended things to also deal with POSTS) and this additional logic? Why does this logic not have to deal with POSTS where the logic above does? I'm not so familiar with this code, so can you explain it to me?
http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... File content/renderer/render_view_impl.cc (right): http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... content/renderer/render_view_impl.cc:2848: OpenURL(frame, url, referrer, default_policy); On 2012/10/24 00:40:37, creis wrote: > I haven't looked at the rest of this CL yet, but I saw this question and wanted > to chime in. Yes, this is currently the only way that we punt a navigation to > the browser process from the renderer process. (Eventually I'd like the browser > process to do the intercepting rather than relying on the somewhat-untrusted > renderer, but that's a different project.) It might be slick if post data delivered from rendererA was sent up more or less as usual, but the response data was piped over to rendererB for parsing and renderering. A resource load where the upload part happens for A and the download part happens for B. Looks like there's some logic in ResourceDispatcherHost for transferring a deferred ResourceLoader from one process to another? Does this logic have anything todo with cross-site'isms or is that about something else? Maybe another potential place to stash the 'postData' could be a deferred ResourceLoader that gets created by A and later picked up by B (purely a creature of the IO thread) instead of the navigation entry (UI thread thingy). void ResourceDispatcherHostImpl::BeginRequest( int request_id, const ResourceHostMsg_Request& request_data, IPC::Message* sync_result, // only valid for sync int route_id) { ..... // If the request that's coming in is being transferred from another process, // we want to reuse and resume the old loader rather than start a new one. linked_ptr<ResourceLoader> deferred_loader; { LoaderMap::iterator it = pending_loaders_.find( GlobalRequestID(request_data.transferred_request_child_id, request_data.transferred_request_request_id)); if (it != pending_loaders_.end()) { if (it->second->is_transferring()) { deferred_loader = it->second; pending_loaders_.erase(it); } else { RecordAction(UserMetricsAction("BadMessageTerminate_RDH")); filter_->BadMessageReceived(); return; } } } But as you say... this would be a differen project?
http://codereview.chromium.org/11193051/diff/30004/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/11193051/diff/30004/chrome/browser/ui/browser.... chrome/browser/ui/browser.cc:226: struct ViewMsg_Request; On 2012/11/02 00:08:23, michaeln wrote: > is this needed, given the assignment below, seems like the compiler must know > more about the type than just this forward decl? Done. http://codereview.chromium.org/11193051/diff/30004/chrome/browser/ui/browser_... File chrome/browser/ui/browser_navigator.h (right): http://codereview.chromium.org/11193051/diff/30004/chrome/browser/ui/browser_... chrome/browser/ui/browser_navigator.h:23: using content::ViewMsg_Request; On 2012/11/02 00:08:23, michaeln wrote: > please avoid the use of using declarations in .h files to avoid potential > namespace collisions Done. http://codereview.chromium.org/11193051/diff/30004/chrome/browser/ui/browser_... chrome/browser/ui/browser_navigator.h:209: ViewMsg_Request request; Yes. You are right. Since all the param structs in this path contain 'url' and 'extra header' field. I believe we can reuse them. On 2012/11/02 00:08:23, michaeln wrote: > There's some overlap between member of tha NavigateParams struct and members of > the new 'request' struct, in particular the 'url' members. The new info that's > missing from this struct seems to be the post data and the content type. I > wonder if the content type can go in the extra_headers field? The new transition > type implies 'POST' so not really sure we need the method string? http://codereview.chromium.org/11193051/diff/30004/content/browser/renderer_h... File content/browser/renderer_host/render_view_host_delegate.h (right): http://codereview.chromium.org/11193051/diff/30004/content/browser/renderer_h... content/browser/renderer_host/render_view_host_delegate.h:433: ViewMsg_Request request) {} On 2012/11/02 00:08:23, michaeln wrote: > please avoid pass by value (with non primitive typess) where possible, prefer > const T& references Done. http://codereview.chromium.org/11193051/diff/30004/content/browser/renderer_h... File content/browser/renderer_host/render_view_host_impl.h (right): http://codereview.chromium.org/11193051/diff/30004/content/browser/renderer_h... content/browser/renderer_host/render_view_host_impl.h:521: const ViewMsg_Request& request); On 2012/11/02 00:08:23, michaeln wrote: > hooray, const ref! Done. http://codereview.chromium.org/11193051/diff/30004/content/browser/web_conten... File content/browser/web_contents/navigation_entry_impl.h (right): http://codereview.chromium.org/11193051/diff/30004/content/browser/web_conten... content/browser/web_contents/navigation_entry_impl.h:18: struct ViewMsg_Request; On 2012/11/02 00:08:23, michaeln wrote: > since this struct is defined in frame_navigate_params.h, is this forward decl > needed? Done. http://codereview.chromium.org/11193051/diff/30004/content/browser/web_conten... content/browser/web_contents/navigation_entry_impl.h:172: ViewMsg_Request request; On 2012/11/02 00:08:23, michaeln wrote: > nits: we put trailing underbars on data members_ and for classes prefer data > members to be private Done. http://codereview.chromium.org/11193051/diff/30004/content/browser/web_conten... content/browser/web_contents/navigation_entry_impl.h:207: scoped_refptr<const base::RefCountedMemory> browser_initiated_post_data_; We also reviewed the browser_initiated_post_data, but this field only exist in the params structs which pass data from browser to render process. For the structs which pass data from render process to browser (e.g OpenURLParams, NavigateParams), there is no such a field. I don't know which one, scoped_refptr<const base::RefCountedMemory> or scoped_refptr<const T>, is faster. If scoped_refptr<const base::RefCountedMemory> is faster, you are right, we need to stick to this field to pass requestbody from render process to browser process. On 2012/11/02 00:08:23, michaeln wrote: > I think you may want to harmonize your new more expressive POST handling with > this older more basic POST data handling. Having two means of handling POSTs in > parallel with one another will leave folks wondering which one to use and why? > Maybe... > > scoped_refptr<const base::RefCountedMemory> browser_initiated_post_data_; > vs > scoped_refptr<const T> content_initiated_cross_site_post_data_; > > Also, how closely have you reviewed the browser_initiated_post_data handling? > The details about when this field is cleared and how it interacts with session > restore seem relevant to your new crosssite form posting case. http://codereview.chromium.org/11193051/diff/30004/content/common/view_messag... File content/common/view_messages.h (right): http://codereview.chromium.org/11193051/diff/30004/content/common/view_messag... content/common/view_messages.h:1919: content::ViewMsg_Request /* Request */) On 2012/11/02 00:08:23, michaeln wrote: > the request param also contains a url data member, is either the param or the > data member in the request struct redundant? Done. http://codereview.chromium.org/11193051/diff/30004/content/public/browser/pag... File content/public/browser/page_navigator.h (right): http://codereview.chromium.org/11193051/diff/30004/content/public/browser/pag... content/public/browser/page_navigator.h:70: ViewMsg_Request request; Besides the url member, the other member in the ViewMsg_Request does not fully support on all the param structs which needs to be used to pass the request. On 2012/11/02 00:08:23, michaeln wrote: > There's some overlap between member of tha OpenURLParams struct and members of > the new 'request' struct, in particular the 'url' members. The new info that's > missing from this struct seems to be the post data and the content type. I > wonder if the content type can go in the extra_headers field? The new transition > type implies 'POST' so not really sure we need the method string? http://codereview.chromium.org/11193051/diff/30004/content/public/common/fram... File content/public/common/frame_navigate_params.h (right): http://codereview.chromium.org/11193051/diff/30004/content/public/common/fram... content/public/common/frame_navigate_params.h:31: scoped_refptr<webkit_glue::ResourceRequestBody> request_body; I think the question is whether do we need to keep ViewMsg_Request struct covering every data or reuse some existing members in other params-structs. The reason why I design like this is because I think this design is much clear. There are several bugs (post-submission, redirect-Chain) which is due to the request body lost during process swap. By keeping all of the request information in one sturct, I think it will benefit others when they need to pass request info from old process to new process. I don't know my concern is correct or not, and I also got your point which is also reasonable. Could you give me some comments on it? Thanks On 2012/11/02 00:08:23, michaeln wrote: > Would these additional params make more sense as additional params of the > FrameNavigateParams struct? http://codereview.chromium.org/11193051/diff/30004/content/renderer/render_vi... File content/renderer/render_view_impl.cc (right): http://codereview.chromium.org/11193051/diff/30004/content/renderer/render_vi... content/renderer/render_view_impl.cc:1172: // this part have't been testes. I need to chat with Albert about it. On 2012/11/02 00:08:23, michaeln wrote: > Q: do cross-site posts ever cross storage partition boundaries? http://codereview.chromium.org/11193051/diff/30004/content/renderer/render_vi... content/renderer/render_view_impl.cc:2799: else { On 2012/11/02 00:08:23, michaeln wrote: > style nit : } else { Done. http://codereview.chromium.org/11193051/diff/30004/content/renderer/render_vi... content/renderer/render_view_impl.cc:2816: const FilePath::StringType kFilePath = FILE_PATH_LITERAL( Also looks odd to me. But I just followed the usage in the file resource_request_body_unittest.cc:24. On 2012/11/02 00:08:23, michaeln wrote: > use of FILE_PATH_LITERAL seems odd since the value here is not a literal? http://codereview.chromium.org/11193051/diff/30004/content/renderer/render_vi... content/renderer/render_view_impl.cc:2860: params.headers.insert(std::pair<std::string,std::string>( For Cross-Process Post Submission, I think Content-Type is the only important header. The reason why I did not use explicit msg param is that I think it can be reused for other problems which need to send all the header through IPC. On 2012/11/02 00:08:23, michaeln wrote: > if there's at most one header value in here, maybe we don't need a whole headers > struct for it and instead could just sent contentType as an explicit msg param http://codereview.chromium.org/11193051/diff/30004/content/renderer/render_vi... content/renderer/render_view_impl.cc:2864: return default_policy; Yes. return WebKit::WebNavigationPolicyIgnor will break the cross-site Post submission. I did not notice the POST happens twice. But I am still investigating what is the right value to return. On 2012/11/02 00:08:23, michaeln wrote: > return default_policy? Is that right? > > Seems like this s/b WebKit::WebNavigationPolicyIgnor since by calling > OpenPostURL we've just asked the browser process to handle this post in a > different renderer. By returning default, does will this POST happens twice, > once in this renderer and again in the other renderer per OpenPostURL > processing? http://codereview.chromium.org/11193051/diff/30004/content/renderer/render_vi... content/renderer/render_view_impl.cc:2885: OpenURL(frame, url, referrer, default_policy); The above code deal with the case when flag --enable-strict-site-isolation is enabled and the top-level navigation. My understanding of the bug is we only need to deal with the strict site isolation case. On 2012/11/02 00:08:23, michaeln wrote: > What is the difference between the cross-site handling above (where you've > extended things to also deal with POSTS) and this additional logic? Why does > this logic not have to deal with POSTS where the logic above does? > > I'm not so familiar with this code, so can you explain it to me?
http://codereview.chromium.org/11193051/diff/30004/content/public/common/fram... File content/public/common/frame_navigate_params.h (right): http://codereview.chromium.org/11193051/diff/30004/content/public/common/fram... content/public/common/frame_navigate_params.h:31: scoped_refptr<webkit_glue::ResourceRequestBody> request_body; Can you point me to a bug about redirect chain being busted due to the upload body data going missing? The presence of duplicate data members actually made things less clear to me when reading thru it. I was left wondering "well, which url are we navigating to really" and "what if these values are different, what might that mean, is that a possibility". Your new struct really doesn't include all of the request information, there's all that other stuff in those surrounding structs as well. I would not attempt to build one-ring-to-binds-them-all and instead make focused small changes that let you accomplish the new behavior very specifically. The header map is a good example of this. Imo it'd be better to just extend structs/params to convey the value that you actually need (the contentType) and not anticipate some other uses involving additional headers that may or may not ever come to be. If/when other header values need to be handled, a map can be added at that time, no need to do it now. On 2012/11/02 17:22:55, irobert wrote: > I think the question is whether do we need to keep ViewMsg_Request struct > covering every data or reuse some existing members in other params-structs. > The reason why I design like this is because I think this design is much clear. > There are several bugs (post-submission, redirect-Chain) which is due to the > request body lost during process swap. By keeping all of the request information > in one sturct, I think it will benefit others when they need to pass request > info from old process to new process. I don't know my concern is correct or not, > and I also got your point which is also reasonable. Could you give me some > comments on it?
A few high level comments below as I've tried to catch up on the discussion. http://codereview.chromium.org/11193051/diff/17001/content/browser/web_conten... File content/browser/web_contents/render_view_host_manager.cc (right): http://codereview.chromium.org/11193051/diff/17001/content/browser/web_conten... content/browser/web_contents/render_view_host_manager.cc:852: ChildProcessSecurityPolicyImpl* policy = On 2012/11/01 19:26:31, irobert wrote: > Not quite sure about it. But i do believe this could be problem. I will further > dig into it. > > On 2012/10/23 23:22:18, michaeln wrote: > > Is the 'policy' thread safe? Just checking because i think this smart > collection > > class gets accessed on the IO thread too. And if it is thread safe, any chance > > for raciness around when access is granted to the 'old' compared to when we're > > checking to migrate access to the 'new'? > From the header file: "ChildProcessSecurityPolicy is a singleton that may be used on any thread." I haven't looked closely enough to tell whether a race between granting and checking is possible, though. http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... File content/renderer/render_view_impl.cc (right): http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... content/renderer/render_view_impl.cc:1106: content::old::GuestToEmbedderChannel* embedder_channel = On 2012/10/23 23:22:18, michaeln wrote: > oh... interesting... this is a direct channel btwn two renderers or is this > channel mediated by the main browser process... hmmm, may open up the > possibility of raciness That code was from the old implementation of BrowserPlugin and has been removed. http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... content/renderer/render_view_impl.cc:1260: main_frame->loadRequest(request); On 2012/10/24 23:45:39, michaeln wrote: > I see... this is where the new request is initiated... it does involve sending > the data to be posted to the destination renderer. If we had a WebHTTPBody > Element that meant "that browser initiated post data you already know about and > have sitting around in the browser process" we could avoid the extra round trip > of post data. As coded postdata flows like this... > > - src renderer sends postdata to browser via the OnOpenPostURL. > - browser stores it in NavigationEntryImpl > - browser sends it to dst renderer via OnNavigate > - dst renderer sends it back to browser via WebRequest > - browser sends it over the network (hooray) > > If the WebRequest could refer to the stored copy, we could cut out a round trip. > > Also, this is where interpreting TypeURL (aka filesystem:// urls) could get > difficult. If the source and destination renderers use different storage > partitions, a filesystem://<origin>/<path> in one partition does not exist in > the other partition. Is it possible to cross-process post across storage > partition boundaries? If so, ResourceDispatcherHost will have to take that into > account when getting actual bytes to upload in the browser process. > > And when does the stored copy of the postdata get cleared of the > NavigationEntryImpl? Yeah, actually this concerns me. Keeping the data in the browser process on NavigationEntryImpl rather than trying to send it to the new renderer might be worth some thought. http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... content/renderer/render_view_impl.cc:2848: OpenURL(frame, url, referrer, default_policy); On 2012/11/02 01:19:24, michaeln wrote: > On 2012/10/24 00:40:37, creis wrote: > > I haven't looked at the rest of this CL yet, but I saw this question and > wanted > > to chime in. Yes, this is currently the only way that we punt a navigation to > > the browser process from the renderer process. (Eventually I'd like the > browser > > process to do the intercepting rather than relying on the somewhat-untrusted > > renderer, but that's a different project.) > > It might be slick if post data delivered from rendererA was sent up more or less > as usual, but the response data was piped over to rendererB for parsing and > renderering. A resource load where the upload part happens for A and the > download part happens for B. > > Looks like there's some logic in ResourceDispatcherHost for transferring a > deferred ResourceLoader from one process to another? Does this logic have > anything todo with cross-site'isms or is that about something else? Maybe > another potential place to stash the 'postData' could be a deferred > ResourceLoader that gets created by A and later picked up by B (purely a > creature of the IO thread) instead of the navigation entry (UI thread thingy). > > void ResourceDispatcherHostImpl::BeginRequest( > int request_id, > const ResourceHostMsg_Request& request_data, > IPC::Message* sync_result, // only valid for sync > int route_id) { > ..... > // If the request that's coming in is being transferred from another process, > // we want to reuse and resume the old loader rather than start a new one. > linked_ptr<ResourceLoader> deferred_loader; > { > LoaderMap::iterator it = pending_loaders_.find( > GlobalRequestID(request_data.transferred_request_child_id, > request_data.transferred_request_request_id)); > if (it != pending_loaders_.end()) { > if (it->second->is_transferring()) { > deferred_loader = it->second; > pending_loaders_.erase(it); > } else { > RecordAction(UserMetricsAction("BadMessageTerminate_RDH")); > filter_->BadMessageReceived(); > return; > } > } > } > > But as you say... this would be a differen project? > Matt Perry added that for doing a process swap during server redirects. Ultimately I'd like to switch all the RenderViewImpl::decidePolicyForNavigation stuff to this approach, but I'm worried about the size of that project. Perhaps it's worth a chat with Matt for a sanity check, though. http://codereview.chromium.org/11193051/diff/30004/chrome/browser/ui/browser_... File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/11193051/diff/30004/chrome/browser/ui/browser_... chrome/browser/ui/browser_navigator.cc:240: content::NavigationController::LOAD_TYPE_BROWSER_INITIATED_HTTP_POST; This seems wrong to me. BROWSER_INITIATED_HTTP_POST is for form submissions from Android Webviews. This is more likely a renderer-initiated POST. http://codereview.chromium.org/11193051/diff/30004/content/browser/web_conten... File content/browser/web_contents/navigation_entry_impl.h (right): http://codereview.chromium.org/11193051/diff/30004/content/browser/web_conten... content/browser/web_contents/navigation_entry_impl.h:207: scoped_refptr<const base::RefCountedMemory> browser_initiated_post_data_; On 2012/11/02 17:22:55, irobert wrote: > We also reviewed the browser_initiated_post_data, but this field only exist in > the params structs which pass data from browser to render process. For the > structs which pass data from render process to browser (e.g OpenURLParams, > NavigateParams), there is no such a field. I don't know which one, > scoped_refptr<const base::RefCountedMemory> or scoped_refptr<const T>, is > faster. If scoped_refptr<const base::RefCountedMemory> is faster, you are right, > we need to stick to this field to pass requestbody from render process to > browser process. > > On 2012/11/02 00:08:23, michaeln wrote: > > I think you may want to harmonize your new more expressive POST handling with > > this older more basic POST data handling. Having two means of handling POSTs > in > > parallel with one another will leave folks wondering which one to use and why? > > Maybe... > > > > scoped_refptr<const base::RefCountedMemory> browser_initiated_post_data_; > > vs > > scoped_refptr<const T> content_initiated_cross_site_post_data_; > > > > Also, how closely have you reviewed the browser_initiated_post_data handling? > > The details about when this field is cleared and how it interacts with session > > restore seem relevant to your new crosssite form posting case. > The browser_initiated_post_data_ stuff is for Android Webviews, allowing the embedder app to generate POST submissions. I agree with the idea of unifying the two approaches, but since Robert's approach must handle more types of submissions, it probably makes sense to switch the existing code to the new approach (whatever it ends up being). http://codereview.chromium.org/11193051/diff/30004/content/renderer/render_vi... File content/renderer/render_view_impl.cc (right): http://codereview.chromium.org/11193051/diff/30004/content/renderer/render_vi... content/renderer/render_view_impl.cc:2864: return default_policy; On 2012/11/02 17:22:55, irobert wrote: > Yes. return WebKit::WebNavigationPolicyIgnor will break the cross-site Post > submission. I did not notice the POST happens twice. But I am still > investigating what is the right value to return. > > On 2012/11/02 00:08:23, michaeln wrote: > > return default_policy? Is that right? > > > > Seems like this s/b WebKit::WebNavigationPolicyIgnor since by calling > > OpenPostURL we've just asked the browser process to handle this post in a > > different renderer. By returning default, does will this POST happens twice, > > once in this renderer and again in the other renderer per OpenPostURL > > processing? > Ignore is the correct thing to return. There must be some other issue if that breaks the submission, because returning the default policy means the old renderer will continue with its POST submission as well. http://codereview.chromium.org/11193051/diff/30004/content/renderer/render_vi... content/renderer/render_view_impl.cc:2885: OpenURL(frame, url, referrer, default_policy); On 2012/11/02 17:22:55, irobert wrote: > The above code deal with the case when flag --enable-strict-site-isolation is > enabled and the top-level navigation. My understanding of the bug is we only > need to deal with the strict site isolation case. Hmm, that's not correct. We'd like this new approach to work for any cross-process navigation, including from a normal page to a hosted app (not just --enable-strict-site-isolation). We should make sure it behaves correctly in other cases as well.
With new data structure to store POST data https://codereview.chromium.org/11193051/diff/17001/content/browser/web_conte... File content/browser/web_contents/render_view_host_manager.cc (right): https://codereview.chromium.org/11193051/diff/17001/content/browser/web_conte... content/browser/web_contents/render_view_host_manager.cc:852: ChildProcessSecurityPolicyImpl* policy = I have verified this with Darin. He mentioned: "It is a thread-safe class. However, you do need to consider sequencing. You need to grant permission before the renderer process issues the POST request." On 2012/11/05 16:21:40, creis wrote: > On 2012/11/01 19:26:31, irobert wrote: > > Not quite sure about it. But i do believe this could be problem. I will > further > > dig into it. > > > > On 2012/10/23 23:22:18, michaeln wrote: > > > Is the 'policy' thread safe? Just checking because i think this smart > > collection > > > class gets accessed on the IO thread too. And if it is thread safe, any > chance > > > for raciness around when access is granted to the 'old' compared to when > we're > > > checking to migrate access to the 'new'? > > > > From the header file: > "ChildProcessSecurityPolicy is a singleton that may be used on any thread." > > I haven't looked closely enough to tell whether a race between granting and > checking is possible, though. https://codereview.chromium.org/11193051/diff/28006/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.h (right): https://codereview.chromium.org/11193051/diff/28006/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.h:13: #include "content/public/common/frame_navigate_params.h" I am planning to include resource_request_body.h. I can build it successfully on my local machine, but when I upload it, I got the following error (See the comments at line 18-21 below). https://codereview.chromium.org/11193051/diff/28006/content/browser/web_conte... File content/browser/web_contents/render_view_host_manager.cc (right): https://codereview.chromium.org/11193051/diff/28006/content/browser/web_conte... content/browser/web_contents/render_view_host_manager.cc:845: ChildProcessSecurityPolicyImpl::GetInstance(); I have verified this with Darin. He mentioned: "It is a thread-safe class. However, you do need to consider sequencing. You need to grant permission before the renderer process issues the POST request." https://codereview.chromium.org/11193051/diff/28006/content/public/common/fra... File content/public/common/frame_navigate_params.h (right): https://codereview.chromium.org/11193051/diff/28006/content/public/common/fra... content/public/common/frame_navigate_params.h:17: #include "webkit/glue/resource_request_body.h" To deal with the problem that cannot include resource_request_body.h in browser_navigator.h https://codereview.chromium.org/11193051/diff/28006/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/11193051/diff/28006/content/renderer/render_v... content/renderer/render_view_impl.cc:2790: // TODO: Fix FILE_PATH_LITERAL; Still looking for the way to convert WebString to FilePath.
https://codereview.chromium.org/11193051/diff/29081/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.h (right): https://codereview.chromium.org/11193051/diff/29081/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.h:13: #include "content/public/common/frame_navigate_params.h" I am planning to include resource_request_body.h. I can build it successfully on my local machine, but when I upload it, I got the following error (See the comments at line 18-21 below). https://codereview.chromium.org/11193051/diff/29081/content/public/common/fra... File content/public/common/frame_navigate_params.h (right): https://codereview.chromium.org/11193051/diff/29081/content/public/common/fra... content/public/common/frame_navigate_params.h:17: #include "webkit/glue/resource_request_body.h" To deal with the problem that cannot include resource_request_body.h in browser_navigator.h
http://codereview.chromium.org/11193051/diff/29081/content/browser/renderer_h... File content/browser/renderer_host/render_view_host_delegate.h (right): http://codereview.chromium.org/11193051/diff/29081/content/browser/renderer_h... content/browser/renderer_host/render_view_host_delegate.h:70: struct ViewMsg_Request; stray code from before http://codereview.chromium.org/11193051/diff/29081/content/browser/renderer_h... content/browser/renderer_host/render_view_host_delegate.h:429: virtual void RequestOpenPostURL( In previous snapshots you had it as an open question whether the exising RequestOpen interfaces should be widened to accept optional additional POST data params or new parallel interfaces for the POST case would be preferable. In looking at the parallel method bodies, there's some of duplicated code in there that probably can be eliminated. I think I'd lean towards merging OpenURL and OpenPostURL since most of the logic is the same (modulo passing the POST data around). http://codereview.chromium.org/11193051/diff/29081/content/common/view_messag... File content/common/view_messages.h (right): http://codereview.chromium.org/11193051/diff/29081/content/common/view_messag... content/common/view_messages.h:1908: IPC_MESSAGE_ROUTED5(ViewHostMsg_OpenPostURL, Instead of defining a new message that lives in parallel with the OpenURL message, i think we should consider consolidating the two. Our ipc macros only support up to 5 params. When we hit that limit, what we generally do is define a 'struct' to contain all of the params to be sent, and then spec the ipc to take the struct only, and name the struct as such... IPC_MESSAGE_ROUTED1(ViewHostMsg_OpenURL, ViewHostMsg_OpenURL_Params) I think we could do that here and add the additional params for POST case to that struct... bool is_post; std::string post_content_type; scoped_refptr<ResourceRequestBody> post_body; std::string post_method; // maybe this one isn't needed? The way you've named the inner struct in this CL makes my eyes burn because it doesn't really follow that pattern despite being named in that <msg_name>_Params fashion. http://codereview.chromium.org/11193051/diff/29081/content/renderer/render_vi... File content/renderer/render_view_impl.cc (right): http://codereview.chromium.org/11193051/diff/29081/content/renderer/render_vi... content/renderer/render_view_impl.cc:1137: // Deal With Cross-Process Post Submission Since this isn't always for cross-process post handling the comment could be misleading. Would be better to remove it i think. http://codereview.chromium.org/11193051/diff/29081/content/renderer/render_vi... content/renderer/render_view_impl.cc:1172: request.setHTTPMethod(WebString::fromUTF8("POST")); If it's OK to assume POST as the method here, great! . What happens with content like <form action="x" method="PUT">?
> http://codereview.chromium.org/11193051/diff/17001/content/renderer/render_vi... > content/renderer/render_view_impl.cc:1260: main_frame->loadRequest(request); > On 2012/10/24 23:45:39, michaeln wrote: > > I see... this is where the new request is initiated... it does involve sending > > the data to be posted to the destination renderer. If we had a WebHTTPBody > > Element that meant "that browser initiated post data you already know about > and > > have sitting around in the browser process" we could avoid the extra round > trip > > of post data. As coded postdata flows like this... > > > > - src renderer sends postdata to browser via the OnOpenPostURL. > > - browser stores it in NavigationEntryImpl > > - browser sends it to dst renderer via OnNavigate > > - dst renderer sends it back to browser via WebRequest > > - browser sends it over the network (hooray) > > > > If the WebRequest could refer to the stored copy, we could cut out a round > trip. > > > > Also, this is where interpreting TypeURL (aka filesystem:// urls) could get > > difficult. If the source and destination renderers use different storage > > partitions, a filesystem://<origin>/<path> in one partition does not exist in > > the other partition. Is it possible to cross-process post across storage > > partition boundaries? If so, ResourceDispatcherHost will have to take that > into > > account when getting actual bytes to upload in the browser process. > > > > And when does the stored copy of the postdata get cleared of the > > NavigationEntryImpl? > > Yeah, actually this concerns me. Keeping the data in the browser process on > NavigationEntryImpl rather than trying to send it to the new renderer might be > worth some thought. When you stand back and look at the data flow, the ping/pongs do look unnecessary, but remedying that would mean altering the 'loader'. But I doubt the optimization is worth the trouble at this point?
https://codereview.chromium.org/11193051/diff/29081/content/browser/renderer_... File content/browser/renderer_host/render_view_host_delegate.h (right): https://codereview.chromium.org/11193051/diff/29081/content/browser/renderer_... content/browser/renderer_host/render_view_host_delegate.h:70: struct ViewMsg_Request; On 2012/11/05 23:38:29, michaeln wrote: > stray code from before Done. https://codereview.chromium.org/11193051/diff/29081/content/browser/renderer_... content/browser/renderer_host/render_view_host_delegate.h:429: virtual void RequestOpenPostURL( Yes. I am also have the same feeling, since the only purpose of OpenPostURL is to pass the extra POST data struct to browser process. On 2012/11/05 23:38:29, michaeln wrote: > In previous snapshots you had it as an open question whether the exising > RequestOpen interfaces should be widened to accept optional additional POST data > params or new parallel interfaces for the POST case would be preferable. > > In looking at the parallel method bodies, there's some of duplicated code in > there that probably can be eliminated. I think I'd lean towards merging OpenURL > and OpenPostURL since most of the logic is the same (modulo passing the POST > data around). https://codereview.chromium.org/11193051/diff/29081/content/common/view_messa... File content/common/view_messages.h (right): https://codereview.chromium.org/11193051/diff/29081/content/common/view_messa... content/common/view_messages.h:1908: IPC_MESSAGE_ROUTED5(ViewHostMsg_OpenPostURL, On 2012/11/05 23:38:29, michaeln wrote: > Instead of defining a new message that lives in parallel with the OpenURL > message, i think we should consider consolidating the two. Our ipc macros only > support up to 5 params. When we hit that limit, what we generally do is define a > 'struct' to contain all of the params to be sent, and then spec the ipc to take > the struct only, and name the struct as such... > > IPC_MESSAGE_ROUTED1(ViewHostMsg_OpenURL, > ViewHostMsg_OpenURL_Params) > > I think we could do that here and add the additional params for POST case to > that struct... > bool is_post; > std::string post_content_type; > scoped_refptr<ResourceRequestBody> post_body; > std::string post_method; // maybe this one isn't needed? > > The way you've named the inner struct in this CL makes my eyes burn because it > doesn't really follow that pattern despite being named in that <msg_name>_Params > fashion. Done. https://codereview.chromium.org/11193051/diff/29081/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/11193051/diff/29081/content/renderer/render_v... content/renderer/render_view_impl.cc:1137: // Deal With Cross-Process Post Submission On 2012/11/05 23:38:29, michaeln wrote: > Since this isn't always for cross-process post handling the comment could be > misleading. Would be better to remove it i think. Done. https://codereview.chromium.org/11193051/diff/29081/content/renderer/render_v... content/renderer/render_view_impl.cc:1172: request.setHTTPMethod(WebString::fromUTF8("POST")); We can assume this from the original code, this piece of code only deals with POST submission. It is also important to support PUT or DELETE method even they are not supported in HTML5. I will keep investigate whether the httpbody is different for these methods. On 2012/11/05 23:38:29, michaeln wrote: > If it's OK to assume POST as the method here, great! . What happens with content > like <form action="x" method="PUT">?
I'll take a looks at the new snapshot. https://codereview.chromium.org/11193051/diff/29081/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/11193051/diff/29081/content/renderer/render_v... content/renderer/render_view_impl.cc:1172: request.setHTTPMethod(WebString::fromUTF8("POST")); On 2012/11/06 05:39:20, irobert wrote: > We can assume this from the original code. Not sure i follow. If content shows up that looks like <form action='x' method='PUT'> what happens, do we not end up in this block of logic (maybe we dont)? And if not, what happens when a <form> element like like that is submitted? I don't know if content like that is valid or how its currently handled by chrome, but if handling of forms with methods other than "POST" involve navigating frames and uploading data to servers in the body of an HTTP msg, we'd want to do the cross-site magic for there too and retain the same HTTP method names and bodies seen by servers in the current impl. Please let us know what you find out about forms with other method names.
https://codereview.chromium.org/11193051/diff/29081/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/11193051/diff/29081/content/renderer/render_v... content/renderer/render_view_impl.cc:1172: request.setHTTPMethod(WebString::fromUTF8("POST")); PUT method submission will not end up in this block. The result for a PUT method submission will lost all the form data due to the process swap. I do agree with you that we also need to support there. I will investigate on how PUT method submission works. On 2012/11/06 22:18:41, michaeln wrote: > On 2012/11/06 05:39:20, irobert wrote: > > We can assume this from the original code. > > Not sure i follow. If content shows up that looks like <form action='x' > method='PUT'> what happens, do we not end up in this block of logic (maybe we > dont)? And if not, what happens when a <form> element like like that is > submitted? I don't know if content like that is valid or how its currently > handled by chrome, but if handling of forms with methods other than "POST" > involve navigating frames and uploading data to servers in the body of an HTTP > msg, we'd want to do the cross-site magic for there too and retain the same HTTP > method names and bodies seen by servers in the current impl. > > Please let us know what you find out about forms with other method names. https://codereview.chromium.org/11193051/diff/26043/chrome/test/data/extensio... File chrome/test/data/extensions/isolated_apps/app1/main.html (right): https://codereview.chromium.org/11193051/diff/26043/chrome/test/data/extensio... chrome/test/data/extensions/isolated_apps/app1/main.html:13: function submitform(url, data) { alert(url);alert(data); Already removed alert.
https://codereview.chromium.org/11193051/diff/34081/chrome/browser/extensions... File chrome/browser/extensions/isolated_app_browsertest.cc (right): https://codereview.chromium.org/11193051/diff/34081/chrome/browser/extensions... chrome/browser/extensions/isolated_app_browsertest.cc:78: IN_PROC_BROWSER_TEST_F(IsolatedAppTest, CrossSiteDataPost) { This Test is for the DataType Post Submission. I am still trying to figure out the test for FileType Post submission. I think we have to simulate the file upload request using native C++ code instead of javascript since it cannot construct a FileType post data. https://codereview.chromium.org/11193051/diff/34081/content/browser/web_conte... File content/browser/web_contents/navigation_entry_impl.h (right): https://codereview.chromium.org/11193051/diff/34081/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.h:201: scoped_refptr<webkit_glue::ResourceRequestBody> browser_initiated_post_data_; We decide not to use base::RefCountedMemory* anymore since it only deals with DataType. We use ResourceRequestBody instead with can support all kinds of data type for POST submission. https://codereview.chromium.org/11193051/diff/34081/content/public/browser/na... File content/public/browser/navigation_controller.h (right): https://codereview.chromium.org/11193051/diff/34081/content/public/browser/na... content/public/browser/navigation_controller.h:143: scoped_refptr<webkit_glue::ResourceRequestBody> browser_initiated_post_data; We decide not to use base::RefCountedMemory* anymore since it only deals with DataType. We use ResourceRequestBody instead with can support all kinds of data type for POST submission. We found out that this browser_initiated_post_data member is introduced for the API loadURL in WebView, which allow Android application load a POST submission navigation to WebView. And the assumption is this POST submission initiated by the Android can only be DataType. https://codereview.chromium.org/11193051/diff/34081/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/11193051/diff/34081/content/renderer/render_v... content/renderer/render_view_impl.cc:2787: FilePath(element.filePath.utf8()), This statement cannot be built on windows but is ok on linux. Do we need to have something like: #if defined(OS_POSIX) #elif defined(OS_WIN) #endif
https://codereview.chromium.org/11193051/diff/36040/chrome/browser/extensions... File chrome/browser/extensions/isolated_app_browsertest.cc (right): https://codereview.chromium.org/11193051/diff/36040/chrome/browser/extensions... chrome/browser/extensions/isolated_app_browsertest.cc:78: IN_PROC_BROWSER_TEST_F(IsolatedAppTest, CrossSiteFilePost) { I am currently blocking on this Test https://codereview.chromium.org/11193051/diff/36040/chrome/test/data/extensio... File chrome/test/data/extensions/isolated_apps/app1/main.html (right): https://codereview.chromium.org/11193051/diff/36040/chrome/test/data/extensio... chrome/test/data/extensions/isolated_apps/app1/main.html:30: <iframe src="../non_app/subframe.html"></iframe>--> Should uncomment this https://codereview.chromium.org/11193051/diff/36040/content/browser/renderer_... File content/browser/renderer_host/transfer_navigation_resource_throttle.cc (right): https://codereview.chromium.org/11193051/diff/36040/content/browser/renderer_... content/browser/renderer_host/transfer_navigation_resource_throttle.cc:39: std::string(""), NULL); The last parameter should passed as NULL if this navigation is not POST. https://codereview.chromium.org/11193051/diff/36040/content/browser/web_conte... File content/browser/web_contents/navigation_controller_impl.cc (right): https://codereview.chromium.org/11193051/diff/36040/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl.cc:270: LOG(WARNING) << " ReloadInternal " << reload_type; Should Remove. https://codereview.chromium.org/11193051/diff/36040/content/browser/web_conte... File content/browser/web_contents/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/11193051/diff/36040/content/browser/web_conte... content/browser/web_contents/navigation_controller_impl_unittest.cc:473: new webkit_glue::ResourceRequestBody(); nits https://codereview.chromium.org/11193051/diff/55003/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/11193051/diff/55003/content/renderer/render_v... content/renderer/render_view_impl.h:857: std::string extra_header, If we save the POST data in the navigation_state or document_state, we can avoid changing this API.
Some review comments below. I think there are still some unresolved questions about using that webkit_glue struct, though. https://codereview.chromium.org/11193051/diff/55003/chrome/browser/extensions... File chrome/browser/extensions/isolated_app_browsertest.cc (right): https://codereview.chromium.org/11193051/diff/55003/chrome/browser/extensions... chrome/browser/extensions/isolated_app_browsertest.cc:151: string16 actual_title = title_watcher.WaitAndGetTitle(); Can we avoid relying on title changes? Might be better to wait for LOAD_STOP. (Also, it's best to declare any observers before the action itself. In this case, you'd declare it above ExecuteJavaScript and wait afterwards.) https://codereview.chromium.org/11193051/diff/55003/chrome/browser/extensions... chrome/browser/extensions/isolated_app_browsertest.cc:154: // Make Sure we are in the new process nit: sure nit: end with period. https://codereview.chromium.org/11193051/diff/55003/chrome/browser/extensions... chrome/browser/extensions/isolated_app_browsertest.cc:159: std::string getResult = "window.domAutomationController.send("; Maybe add a comment to this block about how it's reading the post data from the body of the page. https://codereview.chromium.org/11193051/diff/55003/chrome/browser/extensions... chrome/browser/extensions/isolated_app_browsertest.cc:167: EXPECT_EQ("data="+data+"\n", value); nit: Spaces around the +'s https://codereview.chromium.org/11193051/diff/55003/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/11193051/diff/55003/chrome/browser/ui/browser... chrome/browser/ui/browser.cc:1303: nav_params.browser_initiated_post_data = params.browser_initiated_post_data; We used the "browser_initated_post_data" name before because it could only be initiated from the browser process. Maybe we should just change it to post_data. (I'd also like to look over the CL that introduced it again, since I seem to remember it being dangerous if renderers could ever set it. I'll have to see if we've resolved those concerns.) https://codereview.chromium.org/11193051/diff/55003/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.h (right): https://codereview.chromium.org/11193051/diff/55003/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.h:21: //#include "webkit/glue/resource_request_body.h" What's the status of this? https://codereview.chromium.org/11193051/diff/55003/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.h:215: // request body Please elaborate in this comment. https://codereview.chromium.org/11193051/diff/55003/content/browser/android/c... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/11193051/diff/55003/content/browser/android/c... content/browser/android/content_view_core_impl.cc:676: base::RefCountedBytes::TakeVector(&post_data_vector); nit: Indent 2 more spaces. https://codereview.chromium.org/11193051/diff/55003/content/browser/renderer_... File content/browser/renderer_host/transfer_navigation_resource_throttle.cc (right): https://codereview.chromium.org/11193051/diff/55003/content/browser/renderer_... content/browser/renderer_host/transfer_navigation_resource_throttle.cc:39: false, std::string(""), NULL); nit: No quotes on empty string. https://codereview.chromium.org/11193051/diff/55003/content/browser/web_conte... File content/browser/web_contents/navigation_entry_impl.h (right): https://codereview.chromium.org/11193051/diff/55003/content/browser/web_conte... content/browser/web_contents/navigation_entry_impl.h:202: scoped_refptr<webkit_glue::ResourceRequestBody> browser_initiated_post_data_; I have some concerns about using a ResourceRequestBody struct for all of this, but I haven't had a chance to look carefully yet. I think this is something we'll need to think about and get approval on if we go with it. https://codereview.chromium.org/11193051/diff/55003/content/browser/web_conte... File content/browser/web_contents/render_view_host_manager.cc (right): https://codereview.chromium.org/11193051/diff/55003/content/browser/web_conte... content/browser/web_contents/render_view_host_manager.cc:842: // this permission after the POST. I don't think we want to do this here. I need more time to figure out where it should be, though. https://codereview.chromium.org/11193051/diff/55003/content/browser/web_conte... content/browser/web_contents/render_view_host_manager.cc:849: // is pointed to an invalid address. Valuable comment, but it should go before the if (not in the middle of it). https://codereview.chromium.org/11193051/diff/55003/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/11193051/diff/55003/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:2909: if(request_body != NULL) { nit: Space after if. nit: Don't need "!= NULL" https://codereview.chromium.org/11193051/diff/55003/content/public/browser/na... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/11193051/diff/55003/content/public/browser/na... content/public/browser/navigation_entry.h:131: // 1) browser_initiated_post_data when a new post data request is started. We may want to update this comment as well. https://codereview.chromium.org/11193051/diff/55003/content/public/common/fra... File content/public/common/frame_navigate_params.h (right): https://codereview.chromium.org/11193051/diff/55003/content/public/common/fra... content/public/common/frame_navigate_params.h:17: #include "webkit/glue/resource_request_body.h" Do we need this? (If it's to get around the include problem in browser_navigator.h, then we need to reconsider this. The DEPS check was complaining for a valid reason.) https://codereview.chromium.org/11193051/diff/55003/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/11193051/diff/55003/content/renderer/render_v... content/renderer/render_view_impl.cc:1141: if(params.is_post) { nit: Space after if. https://codereview.chromium.org/11193051/diff/55003/content/renderer/render_v... content/renderer/render_view_impl.cc:1144: const std::vector<ResourceRequestBody::Element>* uploads = Note: this file still needs more review. I haven't looked at this part closely yet. https://codereview.chromium.org/11193051/diff/55003/content/renderer/render_v... content/renderer/render_view_impl.cc:2717: OpenURL(frame, request.url(), referrer, policy, std::string(""), NULL); nit: No quotes. https://codereview.chromium.org/11193051/diff/55003/content/renderer/render_v... content/renderer/render_view_impl.cc:2735: std::string header; Can this be moved to a helper function? https://codereview.chromium.org/11193051/diff/55003/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/11193051/diff/55003/content/renderer/render_v... content/renderer/render_view_impl.h:857: std::string extra_header, On 2012/11/19 19:56:31, irobert wrote: > If we save the POST data in the navigation_state or document_state, we can avoid > changing this API. But those aren't sent to the browser process, are they? And content_state doesn't get sent at the same time. https://codereview.chromium.org/11193051/diff/55003/net/tools/testserver/test... File net/tools/testserver/testserver.py (right): https://codereview.chromium.org/11193051/diff/55003/net/tools/testserver/test... net/tools/testserver/testserver.py:813: self.wfile.write('<html><head><title>echoall</title><style>' Let's avoid changing this file if we can. It could affect many tests.
https://codereview.chromium.org/11193051/diff/55003/chrome/browser/extensions... File chrome/browser/extensions/isolated_app_browsertest.cc (right): https://codereview.chromium.org/11193051/diff/55003/chrome/browser/extensions... chrome/browser/extensions/isolated_app_browsertest.cc:151: string16 actual_title = title_watcher.WaitAndGetTitle(); On 2012/11/20 05:46:03, creis wrote: > Can we avoid relying on title changes? Might be better to wait for LOAD_STOP. > (Also, it's best to declare any observers before the action itself. In this > case, you'd declare it above ExecuteJavaScript and wait afterwards.) Done. https://codereview.chromium.org/11193051/diff/55003/chrome/browser/extensions... chrome/browser/extensions/isolated_app_browsertest.cc:154: // Make Sure we are in the new process On 2012/11/20 05:46:03, creis wrote: > nit: sure > nit: end with period. Done. https://codereview.chromium.org/11193051/diff/55003/chrome/browser/extensions... chrome/browser/extensions/isolated_app_browsertest.cc:159: std::string getResult = "window.domAutomationController.send("; On 2012/11/20 05:46:03, creis wrote: > Maybe add a comment to this block about how it's reading the post data from the > body of the page. Done. https://codereview.chromium.org/11193051/diff/55003/chrome/browser/extensions... chrome/browser/extensions/isolated_app_browsertest.cc:167: EXPECT_EQ("data="+data+"\n", value); On 2012/11/20 05:46:03, creis wrote: > nit: Spaces around the +'s Done. https://codereview.chromium.org/11193051/diff/55003/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.h (right): https://codereview.chromium.org/11193051/diff/55003/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.h:21: //#include "webkit/glue/resource_request_body.h" I did a investigation on this, but do haven't got any clue yet On 2012/11/20 05:46:03, creis wrote: > What's the status of this? https://codereview.chromium.org/11193051/diff/55003/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.h:215: // request body On 2012/11/20 05:46:03, creis wrote: > Please elaborate in this comment. Done. https://codereview.chromium.org/11193051/diff/55003/content/browser/android/c... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/11193051/diff/55003/content/browser/android/c... content/browser/android/content_view_core_impl.cc:676: base::RefCountedBytes::TakeVector(&post_data_vector); On 2012/11/20 05:46:03, creis wrote: > nit: Indent 2 more spaces. Done. https://codereview.chromium.org/11193051/diff/55003/content/browser/renderer_... File content/browser/renderer_host/transfer_navigation_resource_throttle.cc (right): https://codereview.chromium.org/11193051/diff/55003/content/browser/renderer_... content/browser/renderer_host/transfer_navigation_resource_throttle.cc:39: false, std::string(""), NULL); On 2012/11/20 05:46:03, creis wrote: > nit: No quotes on empty string. Done. https://codereview.chromium.org/11193051/diff/55003/content/browser/web_conte... File content/browser/web_contents/render_view_host_manager.cc (right): https://codereview.chromium.org/11193051/diff/55003/content/browser/web_conte... content/browser/web_contents/render_view_host_manager.cc:849: // is pointed to an invalid address. On 2012/11/20 05:46:03, creis wrote: > Valuable comment, but it should go before the if (not in the middle of it). Done. https://codereview.chromium.org/11193051/diff/55003/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/11193051/diff/55003/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:2909: if(request_body != NULL) { On 2012/11/20 05:46:03, creis wrote: > nit: Space after if. > nit: Don't need "!= NULL" Done. https://codereview.chromium.org/11193051/diff/55003/content/public/browser/na... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/11193051/diff/55003/content/public/browser/na... content/public/browser/navigation_entry.h:131: // 1) browser_initiated_post_data when a new post data request is started. On 2012/11/20 05:46:03, creis wrote: > We may want to update this comment as well. Done. https://codereview.chromium.org/11193051/diff/55003/content/public/common/fra... File content/public/common/frame_navigate_params.h (right): https://codereview.chromium.org/11193051/diff/55003/content/public/common/fra... content/public/common/frame_navigate_params.h:17: #include "webkit/glue/resource_request_body.h" Yes. This one is to get around the include problem. I just put it here to upload my cl. On 2012/11/20 05:46:03, creis wrote: > Do we need this? (If it's to get around the include problem in > browser_navigator.h, then we need to reconsider this. The DEPS check was > complaining for a valid reason.) https://codereview.chromium.org/11193051/diff/55003/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/11193051/diff/55003/content/renderer/render_v... content/renderer/render_view_impl.cc:1141: if(params.is_post) { On 2012/11/20 05:46:03, creis wrote: > nit: Space after if. Done. https://codereview.chromium.org/11193051/diff/55003/content/renderer/render_v... content/renderer/render_view_impl.cc:2717: OpenURL(frame, request.url(), referrer, policy, std::string(""), NULL); On 2012/11/20 05:46:03, creis wrote: > nit: No quotes. Done. https://codereview.chromium.org/11193051/diff/55003/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/11193051/diff/55003/content/renderer/render_v... content/renderer/render_view_impl.h:857: std::string extra_header, Like what we have done for Cross-Process redirect navigation. We can retrieve the POST data from navigation_state and pass it to ViewHostMsg_OpenURL_Params. On 2012/11/20 05:46:03, creis wrote: > On 2012/11/19 19:56:31, irobert wrote: > > If we save the POST data in the navigation_state or document_state, we can > avoid > > changing this API. > > But those aren't sent to the browser process, are they? And content_state > doesn't get sent at the same time. https://codereview.chromium.org/11193051/diff/55003/net/tools/testserver/test... File net/tools/testserver/testserver.py (right): https://codereview.chromium.org/11193051/diff/55003/net/tools/testserver/test... net/tools/testserver/testserver.py:813: self.wfile.write('<html><head><title>echoall</title><style>' I also had a discussion with Albert about this issue. The purpose of doing this is to make sure the page is finished loading. I ran the tests, it seems won't effect other tests. But I have already changed our test so that we do not reply on the title. Therefore, I will remove it in the new CL. On 2012/11/20 05:46:03, creis wrote: > Let's avoid changing this file if we can. It could affect many tests. |
