Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1308113008:
OOPIFs: Transitioning Get/Send...SavableResourceLinks away from RenderViewHost. (Closed)
|
Created:
5 years, 3 months ago by Łukasz Anforowicz Modified:
5 years, 3 months ago CC:
chromium-reviews, asanka, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, benjhayden+dwatch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@page-serialization-test Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOOPIFs: Transitioning Get/Send...SavableResourceLinks away from RenderViewHost.
RenderView => RenderFrame
=========================
This CL replaces ViewMsg_GetAllSavableResourceLinksForCurrentPage and
ViewHostMsg_SendCurrentPageAllSavableResourceLinks IPC messages by their
frame-oriented equivalents: FrameMsg_GetSavableResourceLinks,
FrameHostMsg_SavableResourceLinksResponse and
FrameHostMsg_NonSavableResponse (last one is needed to distinguish
between 1) a non-savable frame and 2) a savable frame with no savable
resources underneath it).
URL uniqueness and referrers
============================
SavePackage creates only one SaveItem per unique URL to be saved.
This CL moves uniqueness checks that used to be done in
content/renderer/savable_resources.cc into
content/browser/download/save_package.cc
Having only one SaveItem per given URL means that the code has to pick a
single referrer out of multiple referrals made from the page to the
given URL. This is true both for the old and new code. There might be
a need to follow-up to make sure that this is a valid approach (i.e.
some servers might return different content for different referrers, but
the same URL). I've opened crbug.com/528453 to track this.
Just as before, after this CL we also take care to first process savable
resource links (preferring their referrers) and only later process frame
urls. OTOH, after this CL we do not prefer referrers appearing earlier
(when sychronously walking over all subframes), but instead prefer
referrers from renderer processes which reply first (which can be
somewhat random).
Racyness of URL / content changes
=================================
Page content can change (i.e. with changes trigerred by javascript) between the
time the user requests save-page-as and the time we ask renderers for savable
links and serialized content. The old code tries to protect against this, by
1. Calling Stop() in WebContentsImpl::OnSavePage
2. Verifying in content/renderer/savable_resources.cc that |page_url|
(top-level url of the page user wants to save) is the same as
|main_page_gurl| (current top-level url of the page).
In case of (2), save-page-as is cancelled.
Note that the old behavior of cancelling save-page-as in case of content
changes means that saving frequently changing content (i.e. page that
self-refreshes every X seconds) will fail every now and then. It seems
preferrable to continue save-page-as in this case (as it will give the user
files with some (maybe more recent than intended) content rather than fail
altogether (i.e. this is preferrable because this behavior can satisfy users
who are ok with more recent content, whereas the other behavior would fail to
satisfy any users).
Also note that (2) does not protect against changes in subframes - in this case
the new subframe url will not appear in "urls that have local copy" field of
ViewMsg_GetSerializedHtmlDataForCurrentPageWithLocalLinks message (and
therefore the saved html will not be truly "complete").
Because of the above, the current CL doesn't make any attempt to try to
enumerate and lock down URLs of frames in the "before" state and ensure that
the same URLs are processed during later stages of save-page-as (gathering
savable resource links [this CL] and serialization [later CLs]).
BUG=526786
Committed: https://crrev.com/6af746b740287560dbc9e85d22738000bc72b521
Cr-Commit-Position: refs/heads/master@{#349809}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fixed 2 minor issues caught during a late self-review. #
Total comments: 39
Patch Set 3 : Addressed most of CR feedback from Nasko. #
Total comments: 15
Patch Set 4 : Addressed more CR feedback from Nasko. #
Total comments: 2
Patch Set 5 : Addressed remaining CR feedback from Nasko. #Patch Set 6 : Preserving old behavior wrt referrers. #Patch Set 7 : Fixing member initialization in the other constructor. #Patch Set 8 : Make Send target the right renderer process. #
Total comments: 2
Patch Set 9 : Calling Send directly on RenderFrameHost. #
Total comments: 12
Patch Set 10 : Addressed CR feedback from Randy. #
Total comments: 2
Patch Set 11 : Removed braces from some short if statements. #Patch Set 12 : Rebasing... #Patch Set 13 : Fixed savable_resources_browsertest.cc to use RenderFrame rather than RenderView. #
Messages
Total messages: 37 (8 generated)
lukasza@chromium.org changed reviewers: + nasko@chromium.org
Nasko, can you please take a look? https://codereview.chromium.org/1308113008/diff/1/content/browser/download/sa... File content/browser/download/save_package.cc (right): https://codereview.chromium.org/1308113008/diff/1/content/browser/download/sa... content/browser/download/save_package.cc:1201: continue; In some cases referrers_list[i] might be preferred over what was already saved before (and recorded in waiting_item_queue_ and unique_urls_to_save_). This is because we can get a savable url multiple times from either resources_list of from frame_url - each of those savable urls can come with a different referrer. We probably should pick the "best" referrer. I don't know what "best" means, but the default referrer created for frame_url case below is probably not "best". Note that the old code preferred referrers from resources_list over ones from frame_list, but still more or less randomly (via SavableResourcesUniqueCheck) picked referrers from within the resources_list (without comparing which referrer is "best").
Thanks for the quick CL and help! I think we also need someone that knows the downloads code to give it a look over. Some initial comments. https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... File content/browser/download/save_package.cc (right): https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:345: DCHECK_EQ(SAVE_PAGE_TYPE_AS_ONLY_HTML, save_type_) << save_type_; A side note. Do we behave correctly when saving only HTML? I'm suspecting no, but wanted to ask if you've checked. https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:1161: WebContents* web_contents = WebContentsObserver::web_contents(); This object is already WebContentsObserver, so no need to prefix the method call. Also, being an observer should guarantee this object will not outlive the WebContents. https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:1172: base::Unretained(this))); // Safe, because ForEachFrame is synchronous. Thanks for the comment! https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:1174: if (0 == number_of_frames_with_pending_get_savable_resource_links_) { Is this condition possible? If not, let's use a DCHECK instead of handling it in code. https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:1214: GURL frame_url = render_frame_host->GetLastCommittedURL(); Hmm, I wonder if this can be racy. Might be useful to include the URL of the document in the renderer response and bail out of this method if the last committed URL doesn't match the one coming from the renderer. https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:1233: if (0 < number_of_frames_with_pending_get_savable_resource_links_) { We should never go negative, should we? https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:1236: DCHECK_EQ(0, number_of_frames_with_pending_get_savable_resource_links_); It seems to me that this DCHECK can easily hit. This method is called on each frame response, so if you have more than one frame and the first one responds, the number will not be zero, right? https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... File content/browser/download/save_package.h (right): https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.h:183: void GetSavableResourceLinksForFrame(RenderFrameHost*); Parameters need names. https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.h:184: void OnReceivedNonSavableFrameIndication(RenderFrameHost* render_frame_host); The usual pattern for naming IPC message handlers is On(IPC message name, without the prefix). In this case it will be OnNonSavableResponse. Also, ordering should be the same between header and .cc file. https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.h:190: std::set<GURL> unique_urls_to_save_; // Used to de-dupe urls. Member variables go down in the class, after all methods. https://codereview.chromium.org/1308113008/diff/20001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/1308113008/diff/20001/content/common/frame_me... content/common/frame_messages.h:1025: // Messages for getting savable resource links from frames into the browser. nit: s/into/to/ https://codereview.chromium.org/1308113008/diff/20001/content/common/frame_me... content/common/frame_messages.h:1026: IPC_MESSAGE_ROUTED0(FrameMsg_GetSavableResourceLinks) Please keep FrameMsg messages (which means browser->renderer) with the rest of the FrameMsg ones. The file is split in two parts and are grouped by direction of the message. FrameHostMsg are renderer->browser. https://codereview.chromium.org/1308113008/diff/20001/content/common/frame_me... content/common/frame_messages.h:1027: IPC_MESSAGE_ROUTED0(FrameHostMsg_NonSavableResponse) Put a comment explaining what this message is about. Same for the one below. https://codereview.chromium.org/1308113008/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1308113008/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:4435: WebFrame* frame = GetWebFrame(); nit: Why not just use frame_? https://codereview.chromium.org/1308113008/diff/20001/content/renderer/savabl... File content/renderer/savable_resources.cc (right): https://codereview.chromium.org/1308113008/diff/20001/content/renderer/savabl... content/renderer/savable_resources.cc:48: element.hasHTMLTagName("frame")) { nit: The above fits on one line. Also might be good to update the comment that we aren't handling the iframe, but rather skipping it.
Nasko - can you please take another look? I tried to address most of your feedback, but note that two major issues need more discussion: racyness of changes within the frames being saved + handling of referrers. I hope that asanka@ will be able to help us with this discussion (once crrev.com/1323113002 lands, since this also requires his input + the current CL is built on top of that other CL). https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... File content/browser/download/save_package.cc (right): https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:345: DCHECK_EQ(SAVE_PAGE_TYPE_AS_ONLY_HTML, save_type_) << save_type_; On 2015/09/02 23:45:55, nasko (slow to review) wrote: > A side note. Do we behave correctly when saving only HTML? I'm suspecting no, > but wanted to ask if you've checked. I haven't tested html-only. I need to do this (eventually - outside of the current CL under review). I hope that this will simply work. Also - I haven't yet tested mhtml. I think this will fail with OOPIFs (i.e. I see mhtml-related messages for RV/RVH). https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:1161: WebContents* web_contents = WebContentsObserver::web_contents(); On 2015/09/02 23:45:55, nasko (slow to review) wrote: > This object is already WebContentsObserver, so no need to prefix the method > call. This is tricky. I need the prefix, because otherwise the compiler will think that I want to invoke the local |web_contents| variable, rather than calling the inherited web_contents method. > Also, being an observer should guarantee this object will not outlive the > WebContents. Hmm... I thought that there is no such guarantee, because I saw that destructor of WebContentsImpl has this code: FOR_EACH_OBSERVER(WebContentsObserver, observers_, ResetWebContents()); which ends up causing WebContentsObserver::web_contents() to return nullptr. Does this make sense? What am I missing? https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:1172: base::Unretained(this))); // Safe, because ForEachFrame is synchronous. On 2015/09/02 23:45:55, nasko (slow to review) wrote: > Thanks for the comment! Acknowledged. https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:1174: if (0 == number_of_frames_with_pending_get_savable_resource_links_) { On 2015/09/02 23:45:55, nasko (slow to review) wrote: > Is this condition possible? If not, let's use a DCHECK instead of handling it in > code. Done. I wasn't sure if WebContents is guaranteed to have at least 1 frame, but looking at FrameTree::root_, I think this can indeed be a DCHECK. Thanks. https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:1214: GURL frame_url = render_frame_host->GetLastCommittedURL(); On 2015/09/02 23:45:55, nasko (slow to review) wrote: > Hmm, I wonder if this can be racy. Might be useful to include the URL of the > document in the renderer response and bail out of this method if the last > committed URL doesn't match the one coming from the renderer. Right. I tried to allude to this in the CL description. We do need to discuss this. OTOH, maybe let's postpone discussing this until the CL is ready for review with with asanka@ (i.e. until we flush out generic, or OOPIF-specific CR issues). Some discussion points for url lock (let's keep Referrer discussion in a separate comments thread): - Locking top-level url, vs urls of all frames (old code only did the former) - Expected save-page-as behavior for a page that has some auto-refreshing iframes - Does WebContentsImpl::Stop (called from WebContentsImpl::OnSave / propagated into StopLoading for each frame in FTN) prevent *future* navigations / url changes? https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:1233: if (0 < number_of_frames_with_pending_get_savable_resource_links_) { On 2015/09/02 23:45:55, nasko (slow to review) wrote: > We should never go negative, should we? Correct - we should never go negative. This is enforced by the DCHECK below. Slightly related question - the code assumes that I will eventually get a response from *each* RF/FH pair (and SavePackage will "hung" otherwise). Not sure if this is okay (and what to do about it if this is not ok). https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:1236: DCHECK_EQ(0, number_of_frames_with_pending_get_savable_resource_links_); On 2015/09/02 23:45:55, nasko (slow to review) wrote: > It seems to me that this DCHECK can easily hit. This method is called on each > frame response, so if you have more than one frame and the first one responds, > the number will not be zero, right? If the number is not zero, then we will return on line 1234, right? (wow, nice line number :-). https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... File content/browser/download/save_package.h (right): https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.h:183: void GetSavableResourceLinksForFrame(RenderFrameHost*); On 2015/09/02 23:45:55, nasko (slow to review) wrote: > Parameters need names. Initially I wanted to push back on this feedback (language allows no parameter names in declarations + I felt that the parameter name [thinking about "render_frame_host"] would be redundant + I don't see anything in the style guide that would talk about this). OTOH, I guess there is a better name than "render_frame_host" [i.e. "target" conveys some info beyond the type] and I can go forward with this. So - done here (but then "broken" in declaration of OnMessageReceived). https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.h:184: void OnReceivedNonSavableFrameIndication(RenderFrameHost* render_frame_host); On 2015/09/02 23:45:55, nasko (slow to review) wrote: > The usual pattern for naming IPC message handlers is On(IPC message name, > without the prefix). In this case it will be OnNonSavableResponse. > > Also, ordering should be the same between header and .cc file. Done. https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.h:190: std::set<GURL> unique_urls_to_save_; // Used to de-dupe urls. On 2015/09/02 23:45:55, nasko (slow to review) wrote: > Member variables go down in the class, after all methods. Done. https://codereview.chromium.org/1308113008/diff/20001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/1308113008/diff/20001/content/common/frame_me... content/common/frame_messages.h:1025: // Messages for getting savable resource links from frames into the browser. On 2015/09/02 23:45:55, nasko (slow to review) wrote: > nit: s/into/to/ N/A after moving the messages (and having a separate comment for each). https://codereview.chromium.org/1308113008/diff/20001/content/common/frame_me... content/common/frame_messages.h:1026: IPC_MESSAGE_ROUTED0(FrameMsg_GetSavableResourceLinks) On 2015/09/02 23:45:55, nasko (slow to review) wrote: > Please keep FrameMsg messages (which means browser->renderer) with the rest of > the FrameMsg ones. The file is split in two parts and are grouped by direction > of the message. FrameHostMsg are renderer->browser. Done. https://codereview.chromium.org/1308113008/diff/20001/content/common/frame_me... content/common/frame_messages.h:1027: IPC_MESSAGE_ROUTED0(FrameHostMsg_NonSavableResponse) On 2015/09/02 23:45:55, nasko (slow to review) wrote: > Put a comment explaining what this message is about. Same for the one below. Done. https://codereview.chromium.org/1308113008/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1308113008/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:4435: WebFrame* frame = GetWebFrame(); On 2015/09/02 23:45:55, nasko (slow to review) wrote: > nit: Why not just use frame_? Done. https://codereview.chromium.org/1308113008/diff/20001/content/renderer/savabl... File content/renderer/savable_resources.cc (right): https://codereview.chromium.org/1308113008/diff/20001/content/renderer/savabl... content/renderer/savable_resources.cc:48: element.hasHTMLTagName("frame")) { On 2015/09/02 23:45:55, nasko (slow to review) wrote: > nit: The above fits on one line. Also might be good to update the comment that > we aren't handling the iframe, but rather skipping it. Good point. Done.
https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... File content/browser/download/save_package.cc (right): https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:345: DCHECK_EQ(SAVE_PAGE_TYPE_AS_ONLY_HTML, save_type_) << save_type_; On 2015/09/03 16:59:57, Łukasz Anforowicz wrote: > On 2015/09/02 23:45:55, nasko (slow to review) wrote: > > A side note. Do we behave correctly when saving only HTML? I'm suspecting no, > > but wanted to ask if you've checked. > > I haven't tested html-only. I need to do this (eventually - outside of the > current CL under review). I hope that this will simply work. > > Also - I haven't yet tested mhtml. I think this will fail with OOPIFs (i.e. I > see mhtml-related messages for RV/RVH). MHTML is almost definitely going to be broken :). HTML-only save might actually just work, based on some test saves I just did, which is nice! https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:1161: WebContents* web_contents = WebContentsObserver::web_contents(); On 2015/09/03 16:59:57, Łukasz Anforowicz wrote: > On 2015/09/02 23:45:55, nasko (slow to review) wrote: > > This object is already WebContentsObserver, so no need to prefix the method > > call. > > This is tricky. I need the prefix, because otherwise the compiler will think > that I want to invoke the local |web_contents| variable, rather than calling the > inherited web_contents method. Why do you need a local web_contents variable is the main question? s/web_contents/web_contents()/ seems equivalent to me. > > Also, being an observer should guarantee this object will not outlive the > > WebContents. > > Hmm... I thought that there is no such guarantee, because I saw that destructor > of WebContentsImpl has this code: > > FOR_EACH_OBSERVER(WebContentsObserver, > observers_, > ResetWebContents()); > > which ends up causing WebContentsObserver::web_contents() to return nullptr. > > Does this make sense? What am I missing? > WebContentsImpl also destroys all the observers as part of its cleaning up the observers_ list. This ends up deleting all objects and the destructor for WebContentsObserver is virtual, so the inherited objects should be deleted by default, unless they override the destructor to do something else. https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:1233: if (0 < number_of_frames_with_pending_get_savable_resource_links_) { On 2015/09/03 16:59:57, Łukasz Anforowicz wrote: > On 2015/09/02 23:45:55, nasko (slow to review) wrote: > > We should never go negative, should we? > > Correct - we should never go negative. This is enforced by the DCHECK below. > > Slightly related question - the code assumes that I will eventually get a > response from *each* RF/FH pair (and SavePackage will "hung" otherwise). Not > sure if this is okay (and what to do about it if this is not ok). If we never should go negative, then we shouldn't be adding logic to handle this. A DCHECK indicating expectation is all we need. If we return early, we are peppering over a bug, which is not the approach Chromium takes. https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:1236: DCHECK_EQ(0, number_of_frames_with_pending_get_savable_resource_links_); On 2015/09/03 16:59:56, Łukasz Anforowicz wrote: > On 2015/09/02 23:45:55, nasko (slow to review) wrote: > > It seems to me that this DCHECK can easily hit. This method is called on each > > frame response, so if you have more than one frame and the first one responds, > > the number will not be zero, right? > > If the number is not zero, then we will return on line 1234, right? (wow, nice > line number :-). Since that if statement should disappear, the DCHECK will have to change. Also, the if statement guarantees that the value is not less than zero, but why can't it be positive? This method is called unconditionally on each response, no? https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... File content/browser/download/save_package.h (right): https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.h:183: void GetSavableResourceLinksForFrame(RenderFrameHost*); On 2015/09/03 16:59:57, Łukasz Anforowicz wrote: > On 2015/09/02 23:45:55, nasko (slow to review) wrote: > > Parameters need names. > > Initially I wanted to push back on this feedback (language allows no parameter > names in declarations + I felt that the parameter name [thinking about > "render_frame_host"] would be redundant + I don't see anything in the style > guide that would talk about this). OTOH, I guess there is a better name than > "render_frame_host" [i.e. "target" conveys some info beyond the type] and I can > go forward with this. So - done here (but then "broken" in declaration of > OnMessageReceived). From the Google C++ style guide: "All parameters should be named, with identical names in the declaration and implementation." https://codereview.chromium.org/1308113008/diff/40001/content/browser/downloa... File content/browser/download/save_package.h (right): https://codereview.chromium.org/1308113008/diff/40001/content/browser/downloa... content/browser/download/save_package.h:149: bool OnMessageReceived(const IPC::Message&) override; Why did these methods lose the parameter name? Chromium style guide prohibits nameless parameters (even though Blink allows them). https://codereview.chromium.org/1308113008/diff/40001/content/browser/downloa... content/browser/download/save_package.h:150: bool OnMessageReceived(const IPC::Message&, RenderFrameHost* source) override; nit: Let's keep the parameter name consistent, s/source/render_frame_host/. https://codereview.chromium.org/1308113008/diff/40001/content/browser/downloa... content/browser/download/save_package.h:273: int number_of_frames_with_pending_get_savable_resource_links_; I wonder if we can name this a bit shorter. Do we send any other message that we wait on for responses? If not, number_of_frames_pending_response_ is a lot shorter and avoids the actual IPC message name. https://codereview.chromium.org/1308113008/diff/40001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/1308113008/diff/40001/content/common/frame_me... content/common/frame_messages.h:647: // (this covers only the immediate frame / doesn't cover subframes). nit: Drop the parenthesis and prefix with Note: https://codereview.chromium.org/1308113008/diff/40001/content/common/frame_me... content/common/frame_messages.h:1035: // non-savable content (i.e. from a non-savable schema) or if there were nit: s/schema/scheme/
Nasko, can you take one more look please? https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... File content/browser/download/save_package.cc (right): https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:1161: WebContents* web_contents = WebContentsObserver::web_contents(); On 2015/09/03 18:17:01, nasko (slow to review) wrote: > On 2015/09/03 16:59:57, Łukasz Anforowicz wrote: > > On 2015/09/02 23:45:55, nasko (slow to review) wrote: > > > This object is already WebContentsObserver, so no need to prefix the method > > > call. > > > > This is tricky. I need the prefix, because otherwise the compiler will think > > that I want to invoke the local |web_contents| variable, rather than calling > the > > inherited web_contents method. > > Why do you need a local web_contents variable is the main question? > s/web_contents/web_contents()/ seems equivalent to me. Oh, right... Done. :-) > > > > Also, being an observer should guarantee this object will not outlive the > > > WebContents. > > > > Hmm... I thought that there is no such guarantee, because I saw that > destructor > > of WebContentsImpl has this code: > > > > FOR_EACH_OBSERVER(WebContentsObserver, > > observers_, > > ResetWebContents()); > > > > which ends up causing WebContentsObserver::web_contents() to return nullptr. > > > > Does this make sense? What am I missing? > > > > WebContentsImpl also destroys all the observers as part of its cleaning up the > observers_ list. This ends up deleting all objects and the destructor for > WebContentsObserver is virtual, so the inherited objects should be deleted by > default, unless they override the destructor to do something else. Thanks for explaining. https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:1233: if (0 < number_of_frames_with_pending_get_savable_resource_links_) { On 2015/09/03 18:17:01, nasko (slow to review) wrote: > On 2015/09/03 16:59:57, Łukasz Anforowicz wrote: > > On 2015/09/02 23:45:55, nasko (slow to review) wrote: > > > We should never go negative, should we? > > > > Correct - we should never go negative. This is enforced by the DCHECK below. > > > > Slightly related question - the code assumes that I will eventually get a > > response from *each* RF/FH pair (and SavePackage will "hung" otherwise). Not > > sure if this is okay (and what to do about it if this is not ok). > > If we never should go negative, then we shouldn't be adding logic to handle > this. A DCHECK indicating expectation is all we need. If we return early, we are > peppering over a bug, which is not the approach Chromium takes. I rewrote the DCHECK and "if" to make it more obvious that they check that number_of_frames_... is *greater* than 0 (not less than 0). https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.cc:1236: DCHECK_EQ(0, number_of_frames_with_pending_get_savable_resource_links_); On 2015/09/03 18:17:01, nasko (slow to review) wrote: > On 2015/09/03 16:59:56, Łukasz Anforowicz wrote: > > On 2015/09/02 23:45:55, nasko (slow to review) wrote: > > > It seems to me that this DCHECK can easily hit. This method is called on > each > > > frame response, so if you have more than one frame and the first one > responds, > > > the number will not be zero, right? > > > > If the number is not zero, then we will return on line 1234, right? (wow, > nice > > line number :-). > > Since that if statement should disappear, the DCHECK will have to change. Also, > the if statement guarantees that the value is not less than zero, but why can't > it be positive? This method is called unconditionally on each response, no? See the previous comment. https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... File content/browser/download/save_package.h (right): https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... content/browser/download/save_package.h:183: void GetSavableResourceLinksForFrame(RenderFrameHost*); On 2015/09/03 18:17:02, nasko (slow to review) wrote: > On 2015/09/03 16:59:57, Łukasz Anforowicz wrote: > > On 2015/09/02 23:45:55, nasko (slow to review) wrote: > > > Parameters need names. > > > > Initially I wanted to push back on this feedback (language allows no parameter > > names in declarations + I felt that the parameter name [thinking about > > "render_frame_host"] would be redundant + I don't see anything in the style > > guide that would talk about this). OTOH, I guess there is a better name than > > "render_frame_host" [i.e. "target" conveys some info beyond the type] and I > can > > go forward with this. So - done here (but then "broken" in declaration of > > OnMessageReceived). > > From the Google C++ style guide: > > "All parameters should be named, with identical names in the declaration and > implementation." Ok... https://codereview.chromium.org/1308113008/diff/40001/content/browser/downloa... File content/browser/download/save_package.h (right): https://codereview.chromium.org/1308113008/diff/40001/content/browser/downloa... content/browser/download/save_package.h:149: bool OnMessageReceived(const IPC::Message&) override; On 2015/09/03 18:17:02, nasko (slow to review) wrote: > Why did these methods lose the parameter name? Chromium style guide prohibits > nameless parameters (even though Blink allows them). Because I saw this practice in Blink and thought that it is more readable (in cases where the parameter name is the same as the type name). I guess I need to revert if the style guide says so... :-/ https://codereview.chromium.org/1308113008/diff/40001/content/browser/downloa... content/browser/download/save_package.h:150: bool OnMessageReceived(const IPC::Message&, RenderFrameHost* source) override; On 2015/09/03 18:17:02, nasko (slow to review) wrote: > nit: Let's keep the parameter name consistent, s/source/render_frame_host/. I think that "source" is a better parameter name, because it doesn't just repeat the type of the parameter, but explains what is the meaning of this object in the method. Oh, I think that by "consistent" you mean "consistent with the name used in the base virtual method that gets overridden here". Hmmm... I still prefer "source"... https://codereview.chromium.org/1308113008/diff/40001/content/browser/downloa... content/browser/download/save_package.h:273: int number_of_frames_with_pending_get_savable_resource_links_; On 2015/09/03 18:17:02, nasko (slow to review) wrote: > I wonder if we can name this a bit shorter. Do we send any other message that we > wait on for responses? If not, number_of_frames_pending_response_ is a lot > shorter and avoids the actual IPC message name. We will probably need something equivalent for per-frame-equivalents of ViewMsg_GetSerializedHtmlDataForCurrentPageWithLocalLinks and ViewMsg_SavePageAsMHTML. So, I think I'll stick with the current name for now. OTOH, For a moment I wondered whether I can have something generic here - something equivalent to WebContents::SendToAllFrames, but that takes a callback that is invoked with all the gathered results. I didn't see an immediately obvious way to do it, so I didn't go there. WDYT? https://codereview.chromium.org/1308113008/diff/40001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/1308113008/diff/40001/content/common/frame_me... content/common/frame_messages.h:647: // (this covers only the immediate frame / doesn't cover subframes). On 2015/09/03 18:17:02, nasko (slow to review) wrote: > nit: Drop the parenthesis and prefix with Note: Done. https://codereview.chromium.org/1308113008/diff/40001/content/common/frame_me... content/common/frame_messages.h:1035: // non-savable content (i.e. from a non-savable schema) or if there were On 2015/09/03 18:17:02, nasko (slow to review) wrote: > nit: s/schema/scheme/ Oh man, really? I've been spelling it wrong my whole life... :-( TIL. Done.
https://codereview.chromium.org/1308113008/diff/40001/content/browser/downloa... File content/browser/download/save_package.h (right): https://codereview.chromium.org/1308113008/diff/40001/content/browser/downloa... content/browser/download/save_package.h:150: bool OnMessageReceived(const IPC::Message&, RenderFrameHost* source) override; On 2015/09/03 19:41:24, Łukasz Anforowicz wrote: > On 2015/09/03 18:17:02, nasko (slow to review) wrote: > > nit: Let's keep the parameter name consistent, s/source/render_frame_host/. > > I think that "source" is a better parameter name, because it doesn't just repeat > the type of the parameter, but explains what is the meaning of this object in > the method. > > Oh, I think that by "consistent" you mean "consistent with the name used in the > base virtual method that gets overridden here". Hmmm... I still prefer > "source"... In general, the Chrome codebase tries to be consistent and keep the parameter names. https://codereview.chromium.org/1308113008/diff/40001/content/browser/downloa... content/browser/download/save_package.h:273: int number_of_frames_with_pending_get_savable_resource_links_; On 2015/09/03 19:41:24, Łukasz Anforowicz wrote: > On 2015/09/03 18:17:02, nasko (slow to review) wrote: > > I wonder if we can name this a bit shorter. Do we send any other message that > we > > wait on for responses? If not, number_of_frames_pending_response_ is a lot > > shorter and avoids the actual IPC message name. > > We will probably need something equivalent for per-frame-equivalents of > ViewMsg_GetSerializedHtmlDataForCurrentPageWithLocalLinks and > ViewMsg_SavePageAsMHTML. So, I think I'll stick with the current name for now. The latter message will likely need it. The question is does it really matter which type of responses we are waiting to hear from the renderer? In general, it is really a counter of how many messages we've sent that haven't been replied to. Do we support concurrent save operations? > OTOH, For a moment I wondered whether I can have something generic here - > something equivalent to WebContents::SendToAllFrames, but that takes a callback > that is invoked with all the gathered results. I didn't see an immediately > obvious way to do it, so I didn't go there. WDYT? I think the main problem is that each IPC message is independent from the rest and we don't have formal code way to express that a message is reply to another. This makes wiring it such that a callback is called on a reply not very obvious. I'd suggest writing it in the style the owners of this file prefer. https://codereview.chromium.org/1308113008/diff/40001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/1308113008/diff/40001/content/common/frame_me... content/common/frame_messages.h:1035: // non-savable content (i.e. from a non-savable schema) or if there were On 2015/09/03 19:41:24, Łukasz Anforowicz wrote: > On 2015/09/03 18:17:02, nasko (slow to review) wrote: > > nit: s/schema/scheme/ > > Oh man, really? I've been spelling it wrong my whole life... :-( > TIL. Done. Well, there is schema too, but it is in other areas of Computer Science :). https://codereview.chromium.org/1308113008/diff/60001/content/browser/downloa... File content/browser/download/save_package.cc (right): https://codereview.chromium.org/1308113008/diff/60001/content/browser/downloa... content/browser/download/save_package.cc:1220: if (number_of_frames_with_pending_get_savable_resource_links_ != 0) { nit: Single line if statements don't need {}
Nasko, could you please take another look? There are 2 sets of unreviewed changes here: - patchset #5 - this addresses the remaining stylistic issues from your code review feedback. I think I might have accidentally included a rebase from origin/master here - my apologies for screwing up patchset#4 <-> patchset#5 diff :-/ - patchset #6 and #7 - this aligns old and new behavior wrt referrers + follows your earlier suggestion to pass frame_url in the IPC message (to reduce some aspects of racyness). I've also changed the CL description to talk about those 2 things in more detail. Can you please take a look? If this looks ok, then I'll loop in asanka@. https://codereview.chromium.org/1308113008/diff/40001/content/browser/downloa... File content/browser/download/save_package.h (right): https://codereview.chromium.org/1308113008/diff/40001/content/browser/downloa... content/browser/download/save_package.h:150: bool OnMessageReceived(const IPC::Message&, RenderFrameHost* source) override; On 2015/09/04 16:03:21, nasko (slow to review) wrote: > On 2015/09/03 19:41:24, Łukasz Anforowicz wrote: > > On 2015/09/03 18:17:02, nasko (slow to review) wrote: > > > nit: Let's keep the parameter name consistent, s/source/render_frame_host/. > > > > I think that "source" is a better parameter name, because it doesn't just > repeat > > the type of the parameter, but explains what is the meaning of this object in > > the method. > > > > Oh, I think that by "consistent" you mean "consistent with the name used in > the > > base virtual method that gets overridden here". Hmmm... I still prefer > > "source"... > > In general, the Chrome codebase tries to be consistent and keep the parameter > names. Ok, I've renamed to reuse the name picked by the base class... https://codereview.chromium.org/1308113008/diff/40001/content/browser/downloa... content/browser/download/save_package.h:273: int number_of_frames_with_pending_get_savable_resource_links_; On 2015/09/04 16:03:21, nasko (slow to review) wrote: > On 2015/09/03 19:41:24, Łukasz Anforowicz wrote: > > On 2015/09/03 18:17:02, nasko (slow to review) wrote: > > > I wonder if we can name this a bit shorter. Do we send any other message > that > > we > > > wait on for responses? If not, number_of_frames_pending_response_ is a lot > > > shorter and avoids the actual IPC message name. > > > > We will probably need something equivalent for per-frame-equivalents of > > ViewMsg_GetSerializedHtmlDataForCurrentPageWithLocalLinks and > > ViewMsg_SavePageAsMHTML. So, I think I'll stick with the current name for > now. > > The latter message will likely need it. The question is does it really matter > which type of responses we are waiting to hear from the renderer? In general, it > is really a counter of how many messages we've sent that haven't been replied > to. Do we support concurrent save operations? Ah, makes sense. Each instance of SavePackage only handles a single save-page-as + it only does one kind of messages at a time. So - done. Thanks. > > > OTOH, For a moment I wondered whether I can have something generic here - > > something equivalent to WebContents::SendToAllFrames, but that takes a > callback > > that is invoked with all the gathered results. I didn't see an immediately > > obvious way to do it, so I didn't go there. WDYT? > > I think the main problem is that each IPC message is independent from the rest > and we don't have formal code way to express that a message is reply to another. > This makes wiring it such that a callback is called on a reply not very obvious. > I'd suggest writing it in the style the owners of this file prefer. I was thinking about temporarily instantiating a separate WebContentsObserver that intercepts messages (so no need to wire-in an external message callback). The main problem was with figuring out how to infer-or-specify the type of values carried in the responses. https://codereview.chromium.org/1308113008/diff/60001/content/browser/downloa... File content/browser/download/save_package.cc (right): https://codereview.chromium.org/1308113008/diff/60001/content/browser/downloa... content/browser/download/save_package.cc:1220: if (number_of_frames_with_pending_get_savable_resource_links_ != 0) { On 2015/09/04 16:03:21, nasko (slow to review) wrote: > nit: Single line if statements don't need {} Acknowledged.
I think this is good! Let's loop in asanka@ and I'll stamp the review once we have the green light. https://codereview.chromium.org/1308113008/diff/140001/content/browser/downlo... File content/browser/download/save_package.cc (right): https://codereview.chromium.org/1308113008/diff/140001/content/browser/downlo... content/browser/download/save_package.cc:1173: target->GetProcess()->Send( Why call GetProcess? RenderFrameHost is an IPC::Sender and should have a Send method on it directly.
lukasza@chromium.org changed reviewers: + asanka@chromium.org
Asanka, could you please take a look? In addition to looking at the code, could you please take a look at the CL description, where I describe how the CL mostly preserves the current behavior (which means preserving some potential issues with the current behavior). Also - I've started a document that looks ahead 2-3 CLs in case it helps understand the context of the current CL. See here: https://docs.google.com/document/d/1OMRoBCBuLV-HOcL8ZGcstYdocq0kHmFbZhWwJC7GeGE (shared with @chromium.org). https://codereview.chromium.org/1308113008/diff/140001/content/browser/downlo... File content/browser/download/save_package.cc (right): https://codereview.chromium.org/1308113008/diff/140001/content/browser/downlo... content/browser/download/save_package.cc:1173: target->GetProcess()->Send( On 2015/09/09 00:51:21, nasko (out until Sept 11) wrote: > Why call GetProcess? RenderFrameHost is an IPC::Sender and should have a Send > method on it directly. Done.
lukasza@chromium.org changed reviewers: + rdsmith@chromium.org - asanka@chromium.org
Adding Randy as a reviewer and moving Asanka to CC (as discussed over email earlier today).
The SavePackage code looks basically good. I haven't reviewed the render side code as I don't know that code at all, and I assume you can find a better reviewer. Let me know if I'm the best you can get :-}. https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... File content/browser/download/save_package.cc (right): https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... content/browser/download/save_package.cc:1164: DCHECK_EQ(0, number_of_frames_pending_response_); // No parallel operations? Could you add a comment about why this can't happen? A "?" at the end of "No parallel operations" doesn't give the most comforting feeling :-}. My understanding is that this routine shouldn't be called multiple times because of the check on wait_state_ at the beginning of SP::Init(), but that means the check on wait_state_ above should be a DCHECK, so I may be missing something. https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... content/browser/download/save_package.cc:1168: DCHECK_LT(0, number_of_frames_pending_response_); We're certain there's no way a web page could have no frames, and that there's no type of page for which an IPC might execute synchronously? (If it has to go to the renderer process it won't be synchronous, but I'm not sure how, e.g., chrome:// URLs are processed.) https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... content/browser/download/save_package.cc:1190: DCHECK(u.is_valid()); Hmmm. I'm a little uncomfortable DCHECKing the validity of renderer-sourced data, even if it's only a DCHECK. Shouldn't we explicitly handle bad data from the renderer? (I'm happy for the save package to fail in that case.) (Applies below as well.) https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... content/browser/download/save_package.cc:1223: // covered by savable resource links. Do you have a particular use/model case for when a savable resource and a frame URL will be the same? (Just curious; I'm happy to prefer the savable resource referrer.)
On 2015/09/16 20:48:49, rdsmith wrote: > The SavePackage code looks basically good. I haven't reviewed the render side > code as I don't know that code at all, and I assume you can find a better > reviewer. Let me know if I'm the best you can get :-}. > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > File content/browser/download/save_package.cc (right): > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > content/browser/download/save_package.cc:1164: DCHECK_EQ(0, > number_of_frames_pending_response_); // No parallel operations? > Could you add a comment about why this can't happen? A "?" at the end of "No > parallel operations" doesn't give the most comforting feeling :-}. My > understanding is that this routine shouldn't be called multiple times because of > the check on wait_state_ at the beginning of SP::Init(), but that means the > check on wait_state_ above should be a DCHECK, so I may be missing something. > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > content/browser/download/save_package.cc:1168: DCHECK_LT(0, > number_of_frames_pending_response_); > We're certain there's no way a web page could have no frames, and that there's > no type of page for which an IPC might execute synchronously? (If it has to go > to the renderer process it won't be synchronous, but I'm not sure how, e.g., > chrome:// URLs are processed.) > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > content/browser/download/save_package.cc:1190: DCHECK(u.is_valid()); > Hmmm. I'm a little uncomfortable DCHECKing the validity of renderer-sourced > data, even if it's only a DCHECK. Shouldn't we explicitly handle bad data from > the renderer? (I'm happy for the save package to fail in that case.) (Applies > below as well.) > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > content/browser/download/save_package.cc:1223: // covered by savable resource > links. > Do you have a particular use/model case for when a savable resource and a frame > URL will be the same? (Just curious; I'm happy to prefer the savable resource > referrer.) Whoops, sorry, top level response: The areas in which you've called out change of behavior because of this CL are ok with me--the code is (AFAIC) already broken in those areas, and I don't mind it being broken in a slightly different way.
On 2015/09/16 20:49:30, rdsmith wrote: > On 2015/09/16 20:48:49, rdsmith wrote: > > The SavePackage code looks basically good. I haven't reviewed the render side > > code as I don't know that code at all, and I assume you can find a better > > reviewer. Let me know if I'm the best you can get :-}. I assume that nasko@ can review and sign-off on the other code. > [...] > > Whoops, sorry, top level response: The areas in which you've called out change > of behavior because of this CL are ok with me--the code is (AFAIC) already > broken in those areas, and I don't mind it being broken in a slightly different > way. Ack - thanks for checking / I appreciate a second set of eyes keeping me honest :-)
Randy, can you take another look please? https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... File content/browser/download/save_package.cc (right): https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... content/browser/download/save_package.cc:1164: DCHECK_EQ(0, number_of_frames_pending_response_); // No parallel operations? On 2015/09/16 20:48:48, rdsmith wrote: > Could you add a comment about why this can't happen? A "?" at the end of "No > parallel operations" doesn't give the most comforting feeling :-}. I think I'll just get rid of the comment altogether. The pair of DCHECKs is probably easier to understand and agree with when looking just at the assertions/conditions they cover. > My > understanding is that this routine shouldn't be called multiple times because of > the check on wait_state_ at the beginning of SP::Init() Correct. > but that means the > check on wait_state_ above should be a DCHECK, so I may be missing something. Why? If this routine cannot be called multiple times, then the DCHECK will always hold, right? Also - note that |number_of_frames_pending_response_| will be reused [in the next CL] for the other pair of messages (and so the DCHECK also enforces that the other pair of messages is not unfinished / happening in parallel). https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... content/browser/download/save_package.cc:1168: DCHECK_LT(0, number_of_frames_pending_response_); On 2015/09/16 20:48:48, rdsmith wrote: > We're certain there's no way a web page could have no frames, This was touched upon in here: https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa...: I double-checked and looking at FrameTree::ForEach I see that it will crash (dereference nullptr) if FrameTree::root_ is null (therefore "no crashing" implies "there is at least one frame"). I assume that author of FrameTree::ForEach (nasko@) assumes that there is always 1 frame. Maybe I should add a DCHECK to that point in FrameTree. I wasn't sure if WebContents is guaranteed to have at least 1 frame, but looking at FrameTree::root_, I think this can indeed be a DCHECK. > and that there's > no type of page for which an IPC might execute synchronously? (If it has to go > to the renderer process it won't be synchronous, but I'm not sure how, e.g., > chrome:// URLs are processed.) https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... content/browser/download/save_package.cc:1190: DCHECK(u.is_valid()); On 2015/09/16 20:48:48, rdsmith wrote: > Hmmm. I'm a little uncomfortable DCHECKing the validity of renderer-sourced > data, even if it's only a DCHECK. Shouldn't we explicitly handle bad data from > the renderer? (I'm happy for the save package to fail in that case.) (Applies > below as well.) Good point. I was copy&pasting this code from below, but yes, we should preserve this check in release builds (because as you point out the renderer is not to be trusted). https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... content/browser/download/save_package.cc:1223: // covered by savable resource links. On 2015/09/16 20:48:48, rdsmith wrote: > Do you have a particular use/model case for when a savable resource and a frame > URL will be the same? (Just curious; I'm happy to prefer the savable resource > referrer.) This will happen if something like ins.cite or blockquote.cite point to the same url as iframe.src I am not sure what is the rationale for treating "cite" attribute as a "savable resource" (i.e. why does it get different treatment from a.href which doesn't get saved locally / is not treated as a savable resource). Theoretically this will also happen when something like img.src points to the same url as iframe.src (I guess this is unlikely in practice as img will point to images and iframe will point to html documents). I cooked up the examples above when looking at https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/s...
LGTM with a nit. https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... File content/browser/download/save_package.cc (right): https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... content/browser/download/save_package.cc:1168: DCHECK_LT(0, number_of_frames_pending_response_); On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: > On 2015/09/16 20:48:48, rdsmith wrote: > > We're certain there's no way a web page could have no frames, > > This was touched upon in here: > https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa...: > > I double-checked and looking at FrameTree::ForEach I see that it will crash > (dereference nullptr) if FrameTree::root_ is null (therefore "no crashing" > implies "there is at least one frame"). I assume that author of > FrameTree::ForEach (nasko@) assumes that there is always 1 frame. Maybe I > should add a DCHECK to that point in FrameTree. > I wasn't sure if WebContents is guaranteed to have at least 1 frame, but looking > at FrameTree::root_, I think this can indeed be a DCHECK. > > > and that there's > > no type of page for which an IPC might execute synchronously? (If it has to > go > > to the renderer process it won't be synchronous, but I'm not sure how, e.g., > > chrome:// URLs are processed.) > We are guaranteed to have a root frame, otherwise we can't render anything. A single web page or even about:blank still has a RenderFrameHost and we call it the "main frame". https://codereview.chromium.org/1308113008/diff/180001/content/browser/downlo... File content/browser/download/save_package.cc (right): https://codereview.chromium.org/1308113008/diff/180001/content/browser/downlo... content/browser/download/save_package.cc:1191: if (!u.is_valid()) { nit: no {} for one line if statements (this one and below).
Just looking for someone to tell me that I don't need to worry about synchronous IPCs, then I'm good :-}. https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... File content/browser/download/save_package.cc (right): https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... content/browser/download/save_package.cc:1164: DCHECK_EQ(0, number_of_frames_pending_response_); // No parallel operations? On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: > On 2015/09/16 20:48:48, rdsmith wrote: > > Could you add a comment about why this can't happen? A "?" at the end of "No > > parallel operations" doesn't give the most comforting feeling :-}. > > I think I'll just get rid of the comment altogether. The pair of DCHECKs is > probably easier to understand and agree with when looking just at the > assertions/conditions they cover. > > > My > > understanding is that this routine shouldn't be called multiple times because > of > > the check on wait_state_ at the beginning of SP::Init() > > Correct. > > > but that means the > > check on wait_state_ above should be a DCHECK, so I may be missing something. > > Why? If this routine cannot be called multiple times, then the DCHECK will > always hold, right? I'm a bit worried we're talking past each other :-}. My comment wasn't about this line, it was about the fact that a bit above it we have if (wait_state_ != START_PROCESS) return; which I would think would be a DCHECK if my analysis was correct, so the fact that it isn't suggests that my analysis may be wrong (or that it should be a DCHECK :-}). Not relevant for your CL other than in the confidence to place on my analysis of the inter-class flow. > Also - note that |number_of_frames_pending_response_| will be reused [in the > next CL] for the other pair of messages (and so the DCHECK also enforces that > the other pair of messages is not unfinished / happening in parallel). SG. https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... content/browser/download/save_package.cc:1168: DCHECK_LT(0, number_of_frames_pending_response_); On 2015/09/17 21:18:24, nasko (slow to review) wrote: > On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: > > On 2015/09/16 20:48:48, rdsmith wrote: > > > We're certain there's no way a web page could have no frames, > > > > This was touched upon in here: > > > https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa...: > > > > I double-checked and looking at FrameTree::ForEach I see that it will crash > > (dereference nullptr) if FrameTree::root_ is null (therefore "no crashing" > > implies "there is at least one frame"). I assume that author of > > FrameTree::ForEach (nasko@) assumes that there is always 1 frame. Maybe I > > should add a DCHECK to that point in FrameTree. > > I wasn't sure if WebContents is guaranteed to have at least 1 frame, but > looking > > at FrameTree::root_, I think this can indeed be a DCHECK. > > > > > and that there's > > > no type of page for which an IPC might execute synchronously? (If it has to > > go > > > to the renderer process it won't be synchronous, but I'm not sure how, e.g., > > > chrome:// URLs are processed.) > > > > We are guaranteed to have a root frame, otherwise we can't render anything. A > single web page or even about:blank still has a RenderFrameHost and we call it > the "main frame". Are we also guaranteed that the IPC will never execute synchronously (i.e. be handled in the browser process)? https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... content/browser/download/save_package.cc:1168: DCHECK_LT(0, number_of_frames_pending_response_); On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: > On 2015/09/16 20:48:48, rdsmith wrote: > > We're certain there's no way a web page could have no frames, > > This was touched upon in here: > https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa...: > > I double-checked and looking at FrameTree::ForEach I see that it will crash > (dereference nullptr) if FrameTree::root_ is null (therefore "no crashing" > implies "there is at least one frame"). I assume that author of > FrameTree::ForEach (nasko@) assumes that there is always 1 frame. Maybe I > should add a DCHECK to that point in FrameTree. > I wasn't sure if WebContents is guaranteed to have at least 1 frame, but looking > at FrameTree::root_, I think this can indeed be a DCHECK. > > > and that there's > > no type of page for which an IPC might execute synchronously? (If it has to > go > > to the renderer process it won't be synchronous, but I'm not sure how, e.g., > > chrome:// URLs are processed.) > Ok, I'll accept that there'll always be at least one frame. Is there any circumstance under which the IPC might execute synchronously? (Possibly that's obviously a stupid question, but having asked it, I'd like to hear the answer :-}.)
On 2015/09/17 22:04:37, rdsmith wrote: > Just looking for someone to tell me that I don't need to worry about synchronous > IPCs, then I'm good :-}. Which synchronous IPCs? In general, browser cannot issue synchronous IPCs to the renderer. Are you worried about the other way around? > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > File content/browser/download/save_package.cc (right): > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > content/browser/download/save_package.cc:1164: DCHECK_EQ(0, > number_of_frames_pending_response_); // No parallel operations? > On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: > > On 2015/09/16 20:48:48, rdsmith wrote: > > > Could you add a comment about why this can't happen? A "?" at the end of > "No > > > parallel operations" doesn't give the most comforting feeling :-}. > > > > I think I'll just get rid of the comment altogether. The pair of DCHECKs is > > probably easier to understand and agree with when looking just at the > > assertions/conditions they cover. > > > > > My > > > understanding is that this routine shouldn't be called multiple times > because > > of > > > the check on wait_state_ at the beginning of SP::Init() > > > > Correct. > > > > > but that means the > > > check on wait_state_ above should be a DCHECK, so I may be missing > something. > > > > Why? If this routine cannot be called multiple times, then the DCHECK will > > always hold, right? > > I'm a bit worried we're talking past each other :-}. My comment wasn't about > this line, it was about the fact that a bit above it we have > > if (wait_state_ != START_PROCESS) > return; > > which I would think would be a DCHECK if my analysis was correct, so the fact > that it isn't suggests that my analysis may be wrong (or that it should be a > DCHECK :-}). Not relevant for your CL other than in the confidence to place on > my analysis of the inter-class flow. > > > Also - note that |number_of_frames_pending_response_| will be reused [in the > > next CL] for the other pair of messages (and so the DCHECK also enforces that > > the other pair of messages is not unfinished / happening in parallel). > > SG. > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > content/browser/download/save_package.cc:1168: DCHECK_LT(0, > number_of_frames_pending_response_); > On 2015/09/17 21:18:24, nasko (slow to review) wrote: > > On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: > > > On 2015/09/16 20:48:48, rdsmith wrote: > > > > We're certain there's no way a web page could have no frames, > > > > > > This was touched upon in here: > > > > > > https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa...: > > > > > > I double-checked and looking at FrameTree::ForEach I see that it will crash > > > (dereference nullptr) if FrameTree::root_ is null (therefore "no crashing" > > > implies "there is at least one frame"). I assume that author of > > > FrameTree::ForEach (nasko@) assumes that there is always 1 frame. Maybe I > > > should add a DCHECK to that point in FrameTree. > > > I wasn't sure if WebContents is guaranteed to have at least 1 frame, but > > looking > > > at FrameTree::root_, I think this can indeed be a DCHECK. > > > > > > > and that there's > > > > no type of page for which an IPC might execute synchronously? (If it has > to > > > go > > > > to the renderer process it won't be synchronous, but I'm not sure how, > e.g., > > > > chrome:// URLs are processed.) > > > > > > > We are guaranteed to have a root frame, otherwise we can't render anything. A > > single web page or even about:blank still has a RenderFrameHost and we call it > > the "main frame". > > Are we also guaranteed that the IPC will never execute synchronously (i.e. be > handled in the browser process)? > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > content/browser/download/save_package.cc:1168: DCHECK_LT(0, > number_of_frames_pending_response_); > On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: > > On 2015/09/16 20:48:48, rdsmith wrote: > > > We're certain there's no way a web page could have no frames, > > > > This was touched upon in here: > > > https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa...: > > > > I double-checked and looking at FrameTree::ForEach I see that it will crash > > (dereference nullptr) if FrameTree::root_ is null (therefore "no crashing" > > implies "there is at least one frame"). I assume that author of > > FrameTree::ForEach (nasko@) assumes that there is always 1 frame. Maybe I > > should add a DCHECK to that point in FrameTree. > > I wasn't sure if WebContents is guaranteed to have at least 1 frame, but > looking > > at FrameTree::root_, I think this can indeed be a DCHECK. > > > > > and that there's > > > no type of page for which an IPC might execute synchronously? (If it has to > > go > > > to the renderer process it won't be synchronous, but I'm not sure how, e.g., > > > chrome:// URLs are processed.) > > > > Ok, I'll accept that there'll always be at least one frame. Is there any > circumstance under which the IPC might execute synchronously? (Possibly that's > obviously a stupid question, but having asked it, I'd like to hear the answer > :-}.)
On 2015/09/17 23:31:01, nasko (slow to review) wrote: > On 2015/09/17 22:04:37, rdsmith wrote: > > Just looking for someone to tell me that I don't need to worry about > synchronous > > IPCs, then I'm good :-}. > > Which synchronous IPCs? In general, browser cannot issue synchronous IPCs to the > renderer. Are you worried about the other way around? Is there ever any context in which something that appears to be a renderer to this code will be backed by code in the browser process (chrome://?), meaning that contrary to naive expectations, the response IPC handler will be executed synchronously with the dispatch of the request RPC, resulting in the DCHECK failing? > > > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > > File content/browser/download/save_package.cc (right): > > > > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > > content/browser/download/save_package.cc:1164: DCHECK_EQ(0, > > number_of_frames_pending_response_); // No parallel operations? > > On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: > > > On 2015/09/16 20:48:48, rdsmith wrote: > > > > Could you add a comment about why this can't happen? A "?" at the end of > > "No > > > > parallel operations" doesn't give the most comforting feeling :-}. > > > > > > I think I'll just get rid of the comment altogether. The pair of DCHECKs is > > > probably easier to understand and agree with when looking just at the > > > assertions/conditions they cover. > > > > > > > My > > > > understanding is that this routine shouldn't be called multiple times > > because > > > of > > > > the check on wait_state_ at the beginning of SP::Init() > > > > > > Correct. > > > > > > > but that means the > > > > check on wait_state_ above should be a DCHECK, so I may be missing > > something. > > > > > > Why? If this routine cannot be called multiple times, then the DCHECK will > > > always hold, right? > > > > I'm a bit worried we're talking past each other :-}. My comment wasn't about > > this line, it was about the fact that a bit above it we have > > > > if (wait_state_ != START_PROCESS) > > return; > > > > which I would think would be a DCHECK if my analysis was correct, so the fact > > that it isn't suggests that my analysis may be wrong (or that it should be a > > DCHECK :-}). Not relevant for your CL other than in the confidence to place > on > > my analysis of the inter-class flow. > > > > > Also - note that |number_of_frames_pending_response_| will be reused [in the > > > next CL] for the other pair of messages (and so the DCHECK also enforces > that > > > the other pair of messages is not unfinished / happening in parallel). > > > > SG. > > > > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > > content/browser/download/save_package.cc:1168: DCHECK_LT(0, > > number_of_frames_pending_response_); > > On 2015/09/17 21:18:24, nasko (slow to review) wrote: > > > On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: > > > > On 2015/09/16 20:48:48, rdsmith wrote: > > > > > We're certain there's no way a web page could have no frames, > > > > > > > > This was touched upon in here: > > > > > > > > > > https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa...: > > > > > > > > I double-checked and looking at FrameTree::ForEach I see that it will > crash > > > > (dereference nullptr) if FrameTree::root_ is null (therefore "no crashing" > > > > implies "there is at least one frame"). I assume that author of > > > > FrameTree::ForEach (nasko@) assumes that there is always 1 frame. Maybe I > > > > should add a DCHECK to that point in FrameTree. > > > > I wasn't sure if WebContents is guaranteed to have at least 1 frame, but > > > looking > > > > at FrameTree::root_, I think this can indeed be a DCHECK. > > > > > > > > > and that there's > > > > > no type of page for which an IPC might execute synchronously? (If it > has > > to > > > > go > > > > > to the renderer process it won't be synchronous, but I'm not sure how, > > e.g., > > > > > chrome:// URLs are processed.) > > > > > > > > > > We are guaranteed to have a root frame, otherwise we can't render anything. > A > > > single web page or even about:blank still has a RenderFrameHost and we call > it > > > the "main frame". > > > > Are we also guaranteed that the IPC will never execute synchronously (i.e. be > > handled in the browser process)? > > > > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > > content/browser/download/save_package.cc:1168: DCHECK_LT(0, > > number_of_frames_pending_response_); > > On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: > > > On 2015/09/16 20:48:48, rdsmith wrote: > > > > We're certain there's no way a web page could have no frames, > > > > > > This was touched upon in here: > > > > > > https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa...: > > > > > > I double-checked and looking at FrameTree::ForEach I see that it will crash > > > (dereference nullptr) if FrameTree::root_ is null (therefore "no crashing" > > > implies "there is at least one frame"). I assume that author of > > > FrameTree::ForEach (nasko@) assumes that there is always 1 frame. Maybe I > > > should add a DCHECK to that point in FrameTree. > > > I wasn't sure if WebContents is guaranteed to have at least 1 frame, but > > looking > > > at FrameTree::root_, I think this can indeed be a DCHECK. > > > > > > > and that there's > > > > no type of page for which an IPC might execute synchronously? (If it has > to > > > go > > > > to the renderer process it won't be synchronous, but I'm not sure how, > e.g., > > > > chrome:// URLs are processed.) > > > > > > > Ok, I'll accept that there'll always be at least one frame. Is there any > > circumstance under which the IPC might execute synchronously? (Possibly > that's > > obviously a stupid question, but having asked it, I'd like to hear the answer > > :-}.)
I think the question will be clearer when you look at: https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... AFAIK Randy is worried about reentrancy - when browser calls Send, then can the message get handled synchronously, before Send method returns. And AFAIK the answer is "no - there is no reentracy", because OnMessageReceived called almost directly from a message loop (and there is only one message loop / the method that calls Send is called from the message loop, so OnMessageReceived cannot be called before we return from Send and from the method that called Send). On Thu, Sep 17, 2015 at 4:31 PM, <nasko@chromium.org> wrote: > On 2015/09/17 22:04:37, rdsmith wrote: > >> Just looking for someone to tell me that I don't need to worry about >> > synchronous > >> IPCs, then I'm good :-}. >> > > Which synchronous IPCs? In general, browser cannot issue synchronous IPCs > to the > renderer. Are you worried about the other way around? > > > > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > >> File content/browser/download/save_package.cc (right): >> > > > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > >> content/browser/download/save_package.cc:1164: DCHECK_EQ(0, >> number_of_frames_pending_response_); // No parallel operations? >> On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: >> > On 2015/09/16 20:48:48, rdsmith wrote: >> > > Could you add a comment about why this can't happen? A "?" at the >> end of >> "No >> > > parallel operations" doesn't give the most comforting feeling :-}. >> > >> > I think I'll just get rid of the comment altogether. The pair of >> DCHECKs is >> > probably easier to understand and agree with when looking just at the >> > assertions/conditions they cover. >> > >> > > My >> > > understanding is that this routine shouldn't be called multiple times >> because >> > of >> > > the check on wait_state_ at the beginning of SP::Init() >> > >> > Correct. >> > >> > > but that means the >> > > check on wait_state_ above should be a DCHECK, so I may be missing >> something. >> > >> > Why? If this routine cannot be called multiple times, then the DCHECK >> will >> > always hold, right? >> > > I'm a bit worried we're talking past each other :-}. My comment wasn't >> about >> this line, it was about the fact that a bit above it we have >> > > if (wait_state_ != START_PROCESS) >> return; >> > > which I would think would be a DCHECK if my analysis was correct, so the >> fact >> that it isn't suggests that my analysis may be wrong (or that it should >> be a >> DCHECK :-}). Not relevant for your CL other than in the confidence to >> place >> > on > >> my analysis of the inter-class flow. >> > > > Also - note that |number_of_frames_pending_response_| will be reused [in >> the >> > next CL] for the other pair of messages (and so the DCHECK also enforces >> > that > >> > the other pair of messages is not unfinished / happening in parallel). >> > > SG. >> > > > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > >> content/browser/download/save_package.cc:1168: DCHECK_LT(0, >> number_of_frames_pending_response_); >> On 2015/09/17 21:18:24, nasko (slow to review) wrote: >> > On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: >> > > On 2015/09/16 20:48:48, rdsmith wrote: >> > > > We're certain there's no way a web page could have no frames, >> > > >> > > This was touched upon in here: >> > > >> > >> > > > https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... > : > >> > > >> > > I double-checked and looking at FrameTree::ForEach I see that it will >> > crash > >> > > (dereference nullptr) if FrameTree::root_ is null (therefore "no >> crashing" >> > > implies "there is at least one frame"). I assume that author of >> > > FrameTree::ForEach (nasko@) assumes that there is always 1 frame. >> Maybe I >> > > should add a DCHECK to that point in FrameTree. >> > > I wasn't sure if WebContents is guaranteed to have at least 1 frame, >> but >> > looking >> > > at FrameTree::root_, I think this can indeed be a DCHECK. >> > > >> > > > and that there's >> > > > no type of page for which an IPC might execute synchronously? (If >> it >> > has > >> to >> > > go >> > > > to the renderer process it won't be synchronous, but I'm not sure >> how, >> e.g., >> > > > chrome:// URLs are processed.) >> > > >> > >> > We are guaranteed to have a root frame, otherwise we can't render >> anything. >> > A > >> > single web page or even about:blank still has a RenderFrameHost and we >> call >> > it > >> > the "main frame". >> > > Are we also guaranteed that the IPC will never execute synchronously (i.e. >> be >> handled in the browser process)? >> > > > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > >> content/browser/download/save_package.cc:1168: DCHECK_LT(0, >> number_of_frames_pending_response_); >> On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: >> > On 2015/09/16 20:48:48, rdsmith wrote: >> > > We're certain there's no way a web page could have no frames, >> > >> > This was touched upon in here: >> > >> > > > https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... > : > >> > >> > I double-checked and looking at FrameTree::ForEach I see that it will >> crash >> > (dereference nullptr) if FrameTree::root_ is null (therefore "no >> crashing" >> > implies "there is at least one frame"). I assume that author of >> > FrameTree::ForEach (nasko@) assumes that there is always 1 frame. >> Maybe I >> > should add a DCHECK to that point in FrameTree. >> > I wasn't sure if WebContents is guaranteed to have at least 1 frame, but >> looking >> > at FrameTree::root_, I think this can indeed be a DCHECK. >> > >> > > and that there's >> > > no type of page for which an IPC might execute synchronously? (If it >> has >> > to > >> > go >> > > to the renderer process it won't be synchronous, but I'm not sure how, >> > e.g., > >> > > chrome:// URLs are processed.) >> > >> > > Ok, I'll accept that there'll always be at least one frame. Is there any >> circumstance under which the IPC might execute synchronously? (Possibly >> > that's > >> obviously a stupid question, but having asked it, I'd like to hear the >> answer >> :-}.) >> > > > > https://codereview.chromium.org/1308113008/ > -- Thanks, Lukasz To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/09/17 23:43:06, chromium-reviews wrote: > I think the question will be clearer when you look at: > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > > AFAIK Randy is worried about reentrancy - when browser calls Send, then can > the message get handled synchronously, before Send method returns. And > AFAIK the answer is "no - there is no reentracy", because OnMessageReceived > called almost directly from a message loop (and there is only one message > loop / the method that calls Send is called from the message loop, so > OnMessageReceived cannot be called before we return from Send and from the > method that called Send). Yep, that's my concern. Specifically I'm wondering if there are any corner case implementations of RenderFrameHost for render frames that are served from the browser process (which may never happen--I certainly believe it'd be unusual) in which Send() would actually just indirectly invoke the code to receive the message, and the same on the way back. Tell me that that doesn't happen and you have my stamp. Actually, given that I think it's really unlikely it happens: LGTM, conditional on assurance from a knowledgeable source that my concern in this paragraph is invalid. > > On Thu, Sep 17, 2015 at 4:31 PM, <mailto:nasko@chromium.org> wrote: > > > On 2015/09/17 22:04:37, rdsmith wrote: > > > >> Just looking for someone to tell me that I don't need to worry about > >> > > synchronous > > > >> IPCs, then I'm good :-}. > >> > > > > Which synchronous IPCs? In general, browser cannot issue synchronous IPCs > > to the > > renderer. Are you worried about the other way around? > > > > > > > > > > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > > > >> File content/browser/download/save_package.cc (right): > >> > > > > > > > > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > > > >> content/browser/download/save_package.cc:1164: DCHECK_EQ(0, > >> number_of_frames_pending_response_); // No parallel operations? > >> On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: > >> > On 2015/09/16 20:48:48, rdsmith wrote: > >> > > Could you add a comment about why this can't happen? A "?" at the > >> end of > >> "No > >> > > parallel operations" doesn't give the most comforting feeling :-}. > >> > > >> > I think I'll just get rid of the comment altogether. The pair of > >> DCHECKs is > >> > probably easier to understand and agree with when looking just at the > >> > assertions/conditions they cover. > >> > > >> > > My > >> > > understanding is that this routine shouldn't be called multiple times > >> because > >> > of > >> > > the check on wait_state_ at the beginning of SP::Init() > >> > > >> > Correct. > >> > > >> > > but that means the > >> > > check on wait_state_ above should be a DCHECK, so I may be missing > >> something. > >> > > >> > Why? If this routine cannot be called multiple times, then the DCHECK > >> will > >> > always hold, right? > >> > > > > I'm a bit worried we're talking past each other :-}. My comment wasn't > >> about > >> this line, it was about the fact that a bit above it we have > >> > > > > if (wait_state_ != START_PROCESS) > >> return; > >> > > > > which I would think would be a DCHECK if my analysis was correct, so the > >> fact > >> that it isn't suggests that my analysis may be wrong (or that it should > >> be a > >> DCHECK :-}). Not relevant for your CL other than in the confidence to > >> place > >> > > on > > > >> my analysis of the inter-class flow. > >> > > > > > Also - note that |number_of_frames_pending_response_| will be reused [in > >> the > >> > next CL] for the other pair of messages (and so the DCHECK also enforces > >> > > that > > > >> > the other pair of messages is not unfinished / happening in parallel). > >> > > > > SG. > >> > > > > > > > > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > > > >> content/browser/download/save_package.cc:1168: DCHECK_LT(0, > >> number_of_frames_pending_response_); > >> On 2015/09/17 21:18:24, nasko (slow to review) wrote: > >> > On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: > >> > > On 2015/09/16 20:48:48, rdsmith wrote: > >> > > > We're certain there's no way a web page could have no frames, > >> > > > >> > > This was touched upon in here: > >> > > > >> > > >> > > > > > > > https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... > > : > > > >> > > > >> > > I double-checked and looking at FrameTree::ForEach I see that it will > >> > > crash > > > >> > > (dereference nullptr) if FrameTree::root_ is null (therefore "no > >> crashing" > >> > > implies "there is at least one frame"). I assume that author of > >> > > FrameTree::ForEach (nasko@) assumes that there is always 1 frame. > >> Maybe I > >> > > should add a DCHECK to that point in FrameTree. > >> > > I wasn't sure if WebContents is guaranteed to have at least 1 frame, > >> but > >> > looking > >> > > at FrameTree::root_, I think this can indeed be a DCHECK. > >> > > > >> > > > and that there's > >> > > > no type of page for which an IPC might execute synchronously? (If > >> it > >> > > has > > > >> to > >> > > go > >> > > > to the renderer process it won't be synchronous, but I'm not sure > >> how, > >> e.g., > >> > > > chrome:// URLs are processed.) > >> > > > >> > > >> > We are guaranteed to have a root frame, otherwise we can't render > >> anything. > >> > > A > > > >> > single web page or even about:blank still has a RenderFrameHost and we > >> call > >> > > it > > > >> > the "main frame". > >> > > > > Are we also guaranteed that the IPC will never execute synchronously (i.e. > >> be > >> handled in the browser process)? > >> > > > > > > > > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > > > >> content/browser/download/save_package.cc:1168: DCHECK_LT(0, > >> number_of_frames_pending_response_); > >> On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: > >> > On 2015/09/16 20:48:48, rdsmith wrote: > >> > > We're certain there's no way a web page could have no frames, > >> > > >> > This was touched upon in here: > >> > > >> > > > > > > > https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... > > : > > > >> > > >> > I double-checked and looking at FrameTree::ForEach I see that it will > >> crash > >> > (dereference nullptr) if FrameTree::root_ is null (therefore "no > >> crashing" > >> > implies "there is at least one frame"). I assume that author of > >> > FrameTree::ForEach (nasko@) assumes that there is always 1 frame. > >> Maybe I > >> > should add a DCHECK to that point in FrameTree. > >> > I wasn't sure if WebContents is guaranteed to have at least 1 frame, but > >> looking > >> > at FrameTree::root_, I think this can indeed be a DCHECK. > >> > > >> > > and that there's > >> > > no type of page for which an IPC might execute synchronously? (If it > >> has > >> > > to > > > >> > go > >> > > to the renderer process it won't be synchronous, but I'm not sure how, > >> > > e.g., > > > >> > > chrome:// URLs are processed.) > >> > > >> > > > > Ok, I'll accept that there'll always be at least one frame. Is there any > >> circumstance under which the IPC might execute synchronously? (Possibly > >> > > that's > > > >> obviously a stupid question, but having asked it, I'd like to hear the > >> answer > >> :-}.) > >> > > > > > > > > https://codereview.chromium.org/1308113008/ > > > > > > -- > Thanks, > > Lukasz > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2015/09/18 16:07:32, rdsmith wrote: > On 2015/09/17 23:43:06, chromium-reviews wrote: > > I think the question will be clearer when you look at: > > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > > > > AFAIK Randy is worried about reentrancy - when browser calls Send, then can > > the message get handled synchronously, before Send method returns. And > > AFAIK the answer is "no - there is no reentracy", because OnMessageReceived > > called almost directly from a message loop (and there is only one message > > loop / the method that calls Send is called from the message loop, so > > OnMessageReceived cannot be called before we return from Send and from the > > method that called Send). > > Yep, that's my concern. Specifically I'm wondering if there are any corner case > implementations of RenderFrameHost for render frames that are served from the > browser process (which may never happen--I certainly believe it'd be unusual) in > which Send() would actually just indirectly invoke the code to receive the > message, and the same on the way back. Tell me that that doesn't happen and you > have my stamp. Actually, given that I think it's really unlikely it happens: > LGTM, conditional on assurance from a knowledgeable source that my concern in > this paragraph is invalid. FWIW, it doesn't happen. : ) We will have much bigger problems if it did. > > > > On Thu, Sep 17, 2015 at 4:31 PM, <mailto:nasko@chromium.org> wrote: > > > > > On 2015/09/17 22:04:37, rdsmith wrote: > > > > > >> Just looking for someone to tell me that I don't need to worry about > > >> > > > synchronous > > > > > >> IPCs, then I'm good :-}. > > >> > > > > > > Which synchronous IPCs? In general, browser cannot issue synchronous IPCs > > > to the > > > renderer. Are you worried about the other way around? > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > > > > > >> File content/browser/download/save_package.cc (right): > > >> > > > > > > > > > > > > > > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > > > > > >> content/browser/download/save_package.cc:1164: DCHECK_EQ(0, > > >> number_of_frames_pending_response_); // No parallel operations? > > >> On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: > > >> > On 2015/09/16 20:48:48, rdsmith wrote: > > >> > > Could you add a comment about why this can't happen? A "?" at the > > >> end of > > >> "No > > >> > > parallel operations" doesn't give the most comforting feeling :-}. > > >> > > > >> > I think I'll just get rid of the comment altogether. The pair of > > >> DCHECKs is > > >> > probably easier to understand and agree with when looking just at the > > >> > assertions/conditions they cover. > > >> > > > >> > > My > > >> > > understanding is that this routine shouldn't be called multiple times > > >> because > > >> > of > > >> > > the check on wait_state_ at the beginning of SP::Init() > > >> > > > >> > Correct. > > >> > > > >> > > but that means the > > >> > > check on wait_state_ above should be a DCHECK, so I may be missing > > >> something. > > >> > > > >> > Why? If this routine cannot be called multiple times, then the DCHECK > > >> will > > >> > always hold, right? > > >> > > > > > > I'm a bit worried we're talking past each other :-}. My comment wasn't > > >> about > > >> this line, it was about the fact that a bit above it we have > > >> > > > > > > if (wait_state_ != START_PROCESS) > > >> return; > > >> > > > > > > which I would think would be a DCHECK if my analysis was correct, so the > > >> fact > > >> that it isn't suggests that my analysis may be wrong (or that it should > > >> be a > > >> DCHECK :-}). Not relevant for your CL other than in the confidence to > > >> place > > >> > > > on > > > > > >> my analysis of the inter-class flow. > > >> > > > > > > > Also - note that |number_of_frames_pending_response_| will be reused [in > > >> the > > >> > next CL] for the other pair of messages (and so the DCHECK also enforces > > >> > > > that > > > > > >> > the other pair of messages is not unfinished / happening in parallel). > > >> > > > > > > SG. > > >> > > > > > > > > > > > > > > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > > > > > >> content/browser/download/save_package.cc:1168: DCHECK_LT(0, > > >> number_of_frames_pending_response_); > > >> On 2015/09/17 21:18:24, nasko (slow to review) wrote: > > >> > On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: > > >> > > On 2015/09/16 20:48:48, rdsmith wrote: > > >> > > > We're certain there's no way a web page could have no frames, > > >> > > > > >> > > This was touched upon in here: > > >> > > > > >> > > > >> > > > > > > > > > > > > https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... > > > : > > > > > >> > > > > >> > > I double-checked and looking at FrameTree::ForEach I see that it will > > >> > > > crash > > > > > >> > > (dereference nullptr) if FrameTree::root_ is null (therefore "no > > >> crashing" > > >> > > implies "there is at least one frame"). I assume that author of > > >> > > FrameTree::ForEach (nasko@) assumes that there is always 1 frame. > > >> Maybe I > > >> > > should add a DCHECK to that point in FrameTree. > > >> > > I wasn't sure if WebContents is guaranteed to have at least 1 frame, > > >> but > > >> > looking > > >> > > at FrameTree::root_, I think this can indeed be a DCHECK. > > >> > > > > >> > > > and that there's > > >> > > > no type of page for which an IPC might execute synchronously? (If > > >> it > > >> > > > has > > > > > >> to > > >> > > go > > >> > > > to the renderer process it won't be synchronous, but I'm not sure > > >> how, > > >> e.g., > > >> > > > chrome:// URLs are processed.) > > >> > > > > >> > > > >> > We are guaranteed to have a root frame, otherwise we can't render > > >> anything. > > >> > > > A > > > > > >> > single web page or even about:blank still has a RenderFrameHost and we > > >> call > > >> > > > it > > > > > >> > the "main frame". > > >> > > > > > > Are we also guaranteed that the IPC will never execute synchronously (i.e. > > >> be > > >> handled in the browser process)? > > >> > > > > > > > > > > > > > > > https://codereview.chromium.org/1308113008/diff/160001/content/browser/downlo... > > > > > >> content/browser/download/save_package.cc:1168: DCHECK_LT(0, > > >> number_of_frames_pending_response_); > > >> On 2015/09/16 22:47:30, Łukasz Anforowicz wrote: > > >> > On 2015/09/16 20:48:48, rdsmith wrote: > > >> > > We're certain there's no way a web page could have no frames, > > >> > > > >> > This was touched upon in here: > > >> > > > >> > > > > > > > > > > > > https://codereview.chromium.org/1308113008/diff/20001/content/browser/downloa... > > > : > > > > > >> > > > >> > I double-checked and looking at FrameTree::ForEach I see that it will > > >> crash > > >> > (dereference nullptr) if FrameTree::root_ is null (therefore "no > > >> crashing" > > >> > implies "there is at least one frame"). I assume that author of > > >> > FrameTree::ForEach (nasko@) assumes that there is always 1 frame. > > >> Maybe I > > >> > should add a DCHECK to that point in FrameTree. > > >> > I wasn't sure if WebContents is guaranteed to have at least 1 frame, but > > >> looking > > >> > at FrameTree::root_, I think this can indeed be a DCHECK. > > >> > > > >> > > and that there's > > >> > > no type of page for which an IPC might execute synchronously? (If it > > >> has > > >> > > > to > > > > > >> > go > > >> > > to the renderer process it won't be synchronous, but I'm not sure how, > > >> > > > e.g., > > > > > >> > > chrome:// URLs are processed.) > > >> > > > >> > > > > > > Ok, I'll accept that there'll always be at least one frame. Is there any > > >> circumstance under which the IPC might execute synchronously? (Possibly > > >> > > > that's > > > > > >> obviously a stupid question, but having asked it, I'd like to hear the > > >> answer > > >> :-}.) > > >> > > > > > > > > > > > > https://codereview.chromium.org/1308113008/ > > > > > > > > > > > -- > > Thanks, > > > > Lukasz > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1308113008/diff/180001/content/browser/downlo... File content/browser/download/save_package.cc (right): https://codereview.chromium.org/1308113008/diff/180001/content/browser/downlo... content/browser/download/save_package.cc:1191: if (!u.is_valid()) { On 2015/09/17 21:18:24, nasko (slow to review) wrote: > nit: no {} for one line if statements (this one and below). Done - I see that the rest of the file indeed consistently uses no braces for things like that. OTOH, I did take a look at the style guide and it doesn't encourage anything one way or the other. Quotes (ephasis mine): https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals "Short conditional statements *may* be written on one line if this enhances readability." "In general, curly braces are *not* *required* for single-line statements, but they are *allowed* if you like them; conditional or loop statements with complex conditions or statements may be more readable with curly braces. Some projects require that an if must always always have an accompanying brace."
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1308113008/#ps220001 (title: "Rebasing...")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308113008/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308113008/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Nasko/Randy - I realized that I also need to make changes in savable_resources_browsertest.cc (patchset #13 - fixed savable_resources_browsertest.cc to use RenderFrame rather than RenderView). These changes are more-or-less mechanical, but in case you want to take a look, I'll postpone pushing to CQ until Monday.
On 2015/09/18 22:27:58, Łukasz Anforowicz wrote: > Nasko/Randy - I realized that I also need to make changes in > savable_resources_browsertest.cc (patchset #13 - fixed > savable_resources_browsertest.cc to use RenderFrame rather than RenderView). > These changes are more-or-less mechanical, but in case you want to take a look, > I'll postpone pushing to CQ until Monday. Oh, this test file ... cannot be unseen : ). Still LGTM.
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/1308113008/#ps240001 (title: "Fixed savable_resources_browsertest.cc to use RenderFrame rather than RenderView.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308113008/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308113008/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/6af746b740287560dbc9e85d22738000bc72b521 Cr-Commit-Position: refs/heads/master@{#349809} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
