Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2568893002:
Prevent drag-and-drop events from firing over cross-site, same-page frames. (Closed)
DescriptionPrevent drag-and-drop events from firing over cross-site, same-page frames.
This is done by storing the source RenderViewHost and RenderProcessHost for each dragstart. Then, for other drag events (like dragover), if the target frame for that event is in the same RenderViewHost, but different RenderProcessHost, than the source, then the event is not fired.
Note that this patch will not affect behavior on Mac, and a subsequent CL will enforce the same thing for Mac.
BUG=666858
Committed: https://crrev.com/7dfa247951db2ccb10b8b5276eb58a33cda4728f
Cr-Commit-Position: refs/heads/master@{#438609}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed comments. #
Total comments: 23
Patch Set 3 : Addressed additional comments from everyone. #
Total comments: 2
Patch Set 4 : Fixed typo in comment. #
Messages
Total messages: 38 (20 generated)
Description was changed from ========== Prevent drag-and-drop events from firing over cross-site, same-page frames. BUG=666858 ========== to ========== Prevent drag-and-drop events from firing over cross-site, same-page frames. Note that this patch will not affect behavior on Mac, and a subsequent CL will enforce the same thing for Mac. BUG=666858 ==========
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Description was changed from ========== Prevent drag-and-drop events from firing over cross-site, same-page frames. Note that this patch will not affect behavior on Mac, and a subsequent CL will enforce the same thing for Mac. BUG=666858 ========== to ========== Prevent drag-and-drop events from firing over cross-site, same-page frames. This is done by storing the source RenderViewHost and RenderProcessHost for each dragstart. Then, for other drag events like dragover, if the target frame for that event is in the same RenderViewHost, but different RenderProcessHost, than the source, then the event is not fired. Note that this patch will not affect behavior on Mac, and a subsequent CL will enforce the same thing for Mac. BUG=666858 ==========
Description was changed from ========== Prevent drag-and-drop events from firing over cross-site, same-page frames. This is done by storing the source RenderViewHost and RenderProcessHost for each dragstart. Then, for other drag events like dragover, if the target frame for that event is in the same RenderViewHost, but different RenderProcessHost, than the source, then the event is not fired. Note that this patch will not affect behavior on Mac, and a subsequent CL will enforce the same thing for Mac. BUG=666858 ========== to ========== Prevent drag-and-drop events from firing over cross-site, same-page frames. This is done by storing the source RenderViewHost and RenderProcessHost for each dragstart. Then, for other drag events (like dragover), if the target frame for that event is in the same RenderViewHost, but different RenderProcessHost, than the source, then the event is not fired. Note that this patch will not affect behavior on Mac, and a subsequent CL will enforce the same thing for Mac. BUG=666858 ==========
paulmeyer@chromium.org changed reviewers: + creis@chromium.org
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
creis@chromium.org changed reviewers: + lfg@chromium.org
Thanks! I can take a look after my meeting. Maybe lfg@ can take a first pass, since I think he's more familiar with this code?
Could you please also remove the test filter from testing/buildbot/filters/site-per-process.interactive_ui_tests.filter (i.e. revert to the revision in https://chromium.googlesource.com/chromium/src/+/07dcd0c5edf1fa134708755ab406...).
https://codereview.chromium.org/2568893002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/2568893002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:912: drag_source_rph_ = source_rwh->GetProcess(); Would it be possible that a RenderProcessHost is destroyed and a new RenderProcessHost is recreated at the same memory address as the previous one? Shouldn't we be storing the RenderProcessHost IDs instead? https://codereview.chromium.org/2568893002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:1143: if (drag_source_rvh_ == web_contents_->GetRenderViewHost() && I don't understand this check. This would only be false if the WebContentsViewAura that we called OnDragEntered is different than the WebContentsViewAura that we called StartDragging. In that case, won't drag_source_rvh_ be null? Is it possible to fail this check while drag_source_rvh_ != null?
https://codereview.chromium.org/2568893002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/2568893002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:912: drag_source_rph_ = source_rwh->GetProcess(); On 2016/12/12 20:34:21, lfg wrote: > Would it be possible that a RenderProcessHost is destroyed and a new > RenderProcessHost is recreated at the same memory address as the previous one? > Shouldn't we be storing the RenderProcessHost IDs instead? Yes, I'm concerned about the void* pointers as well, here and the one added long ago in https://chromiumcodereview.appspot.com/10388105/ (just above this in the .h file). Storing the process ID and RVH routing ID would seem safer to me.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2568893002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/2568893002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:912: drag_source_rph_ = source_rwh->GetProcess(); On 2016/12/12 20:34:21, lfg wrote: > Would it be possible that a RenderProcessHost is destroyed and a new > RenderProcessHost is recreated at the same memory address as the previous one? > Shouldn't we be storing the RenderProcessHost IDs instead? Done. https://codereview.chromium.org/2568893002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:912: drag_source_rph_ = source_rwh->GetProcess(); On 2016/12/12 20:57:02, Charlie Reis wrote: > On 2016/12/12 20:34:21, lfg wrote: > > Would it be possible that a RenderProcessHost is destroyed and a new > > RenderProcessHost is recreated at the same memory address as the previous one? > > Shouldn't we be storing the RenderProcessHost IDs instead? > > Yes, I'm concerned about the void* pointers as well, here and the one added long > ago in https://chromiumcodereview.appspot.com/10388105/ (just above this in the > .h file). Storing the process ID and RVH routing ID would seem safer to me. Done. https://codereview.chromium.org/2568893002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:1143: if (drag_source_rvh_ == web_contents_->GetRenderViewHost() && On 2016/12/12 20:34:21, lfg wrote: > I don't understand this check. This would only be false if the > WebContentsViewAura that we called OnDragEntered is different than the > WebContentsViewAura that we called StartDragging. In that case, won't > drag_source_rvh_ be null? > > Is it possible to fail this check while drag_source_rvh_ != null? Per offline discussion, I will continue checking the RVH, but will store it as (process ID, routing ID) pair instead of a pointer.
Thanks. A few quick thoughts on the .h file before I have to run. https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.h (right): https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:206: // We also keep track of the ID of the RenderViewHost we're dragging over to Please elaborate in both comments what each int in the pair is, since it's very easy to guess wrong. https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:210: // We track the IDs of the source RenderProcessHost and RenderViewHost from Nasko brought up the question about why this isn't tracking RenderWidgetHost. That's because we want to support A-B-A within the same page, right? It's worth clarifying in the comment that we're intentionally tracking this at the page level. (This will matter as we get rid of RenderViewHost and turn it more into a page structure.) https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:214: int drag_source_rph_; It's hard to tell from the name what this is. Maybe drag_source_process_id_? https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:215: std::pair<int, int> drag_source_rvh_; drag_source_view_ids_?
lukasza@chromium.org changed reviewers: + lukasza@chromium.org
https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.h (right): https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:206: // We also keep track of the ID of the RenderViewHost we're dragging over to On 2016/12/13 23:01:16, Charlie Reis wrote: > Please elaborate in both comments what each int in the pair is, since it's very > easy to guess wrong. Not sure if it is applicable here + don't want to block the CL, but please note that there is GlobalRoutingID in content/browser/loader/global_routing_id.h (a more strongly-typed alternative to std::pair<int, int>).
Mostly looks good, I think we should have a test for this, can you add a TODO/create a bug for adding a test? https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:523: current_rvh_for_drag_(-1, -1), You should use kInvalidUniqueID for invalid process ids, and MSG_ROUTING_NONE for invalid routing IDs. https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:562: drag_source_rph_ = -1; kInvalidUniqueID https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:563: drag_source_rvh_ = std::make_pair(-1, -1); kInvalidUniqueID, MSG_ROUTING_NONE.
On 2016/12/14 16:10:10, lfg wrote: > Mostly looks good, I think we should have a test for this, can you add a > TODO/create a bug for adding a test? We do have a test - DragAndDropBrowserTest.CrossSiteDrag After this CL lands we remove the test filter from testing/buildbot/filters/site-per-process.interactive_ui_tests.filter - i.e. revert to the revision in https://chromium.googlesource.com/chromium/src/+/07dcd0c5edf1fa134708755ab406... I don't have a strong opinion whether removal of the test filter should be done in this CL or not (it will cause merge conflicts when merging back to M56 branch, but these conflicts should be simple to remove).
A few more suggestions to finish it up. I do think we should enable Lukasz's test in this CL by removing it from the filter, to verify that this fix gets the test to pass on all the bots. Thanks! https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:9: #include <utility> Might not need this if we use GlobalRoutingID. https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:644: return target_rwh->GetProcess()->GetID() == drag_source_rph_ || Please put a comment explaining what this is trying to check, since it's a bit subtle. Something like: Allow the drag if it's within the same process (in which case Blink will validate it), or if it's between different pages. Do not allow cross-process drags within the same page. https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.h (right): https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:93: bool ValidDragTarget(RenderWidgetHostImpl* target_rwh) const; nit: IsValidDragTarget https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:206: // We also keep track of the ID of the RenderViewHost we're dragging over to On 2016/12/13 23:07:24, Łukasz Anforowicz wrote: > On 2016/12/13 23:01:16, Charlie Reis wrote: > > Please elaborate in both comments what each int in the pair is, since it's > very > > easy to guess wrong. > > Not sure if it is applicable here + don't want to block the CL, but please note > that there is GlobalRoutingID in content/browser/loader/global_routing_id.h (a > more strongly-typed alternative to std::pair<int, int>). That's a good idea-- let's use that. (Looks like it won't be a problem from this file to depend on content/browser/loader, per DEPS.) https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:213: // to the source frame). See crbug.com/666858. We should also clarify that the RPH and RVH here may not be in the same process, which isn't immediately obvious. The RPH is from the frame that started the drag (possibly an OOPIF), and the RVH corresponds to the main frame. https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:215: std::pair<int, int> drag_source_rvh_; On 2016/12/13 23:01:16, Charlie Reis wrote: > drag_source_view_ids_? Or drag_source_view_global_routing_id_.
PTAL https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.h (right): https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:93: bool ValidDragTarget(RenderWidgetHostImpl* target_rwh) const; On 2016/12/14 18:06:50, Charlie Reis wrote: > nit: IsValidDragTarget Done. https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:206: // We also keep track of the ID of the RenderViewHost we're dragging over to On 2016/12/13 23:07:24, Łukasz Anforowicz wrote: > On 2016/12/13 23:01:16, Charlie Reis wrote: > > Please elaborate in both comments what each int in the pair is, since it's > very > > easy to guess wrong. > > Not sure if it is applicable here + don't want to block the CL, but please note > that there is GlobalRoutingID in content/browser/loader/global_routing_id.h (a > more strongly-typed alternative to std::pair<int, int>). Okay, GlobalRoutingID seems like a better thing to use here. https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:210: // We track the IDs of the source RenderProcessHost and RenderViewHost from On 2016/12/13 23:01:16, Charlie Reis wrote: > Nasko brought up the question about why this isn't tracking RenderWidgetHost. > That's because we want to support A-B-A within the same page, right? It's worth > clarifying in the comment that we're intentionally tracking this at the page > level. > > (This will matter as we get rid of RenderViewHost and turn it more into a page > structure.) RenderViewHost is tracked to check the "same-page" part, while the RenderProcessHost is tracked to check the "cross-site" part. The RenderViewHost isn't being used in place of RenderWidgetHost; rather it's the RenderProcessHost that is being used in place of the RenderWidgetHost to allow A-B-A in same page. I'll add to the comment to clarify this. https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:213: // to the source frame). See crbug.com/666858. On 2016/12/14 18:06:50, Charlie Reis wrote: > We should also clarify that the RPH and RVH here may not be in the same process, > which isn't immediately obvious. The RPH is from the frame that started the > drag (possibly an OOPIF), and the RVH corresponds to the main frame. Done. https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:214: int drag_source_rph_; On 2016/12/13 23:01:16, Charlie Reis wrote: > It's hard to tell from the name what this is. Maybe drag_source_process_id_? Actually, I noticed there are some other uses of the term "source" and even "drag_source" but aren't really the same as these, which is confusing. Because of this, I'm going to change "source" to "start" in these, since they are tracked specifically from dragStart. I'll rename this to drag_start_process_id_. https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:215: std::pair<int, int> drag_source_rvh_; On 2016/12/13 23:01:16, Charlie Reis wrote: > drag_source_view_ids_? Renamed to drag_start_view_id_.
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! That's much clearer. LGTM with the changes below. https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:644: return target_rwh->GetProcess()->GetID() == drag_source_rph_ || On 2016/12/14 18:06:50, Charlie Reis wrote: > Please put a comment explaining what this is trying to check, since it's a bit > subtle. Something like: > > Allow the drag if it's within the same process (in which case Blink will > validate it), or if it's between different pages. Do not allow cross-process > drags within the same page. I think you missed this comment, which I think is worthwhile because of the Blink validation part. https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.h (right): https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:214: int drag_source_rph_; On 2016/12/14 18:27:48, paulmeyer wrote: > On 2016/12/13 23:01:16, Charlie Reis wrote: > > It's hard to tell from the name what this is. Maybe drag_source_process_id_? > > Actually, I noticed there are some other uses of the term "source" and even > "drag_source" but aren't really the same as these, which is confusing. Because > of this, I'm going to change "source" to "start" in these, since they are > tracked specifically from dragStart. > > I'll rename this to drag_start_process_id_. Acknowledged. https://codereview.chromium.org/2568893002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.h (right): https://codereview.chromium.org/2568893002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:220: // The RenderViewHost may not be in the same process as the RenderProcessHost, nit: s/The/the/
https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.cc (right): https://codereview.chromium.org/2568893002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.cc:644: return target_rwh->GetProcess()->GetID() == drag_source_rph_ || On 2016/12/14 18:36:36, Charlie Reis wrote: > On 2016/12/14 18:06:50, Charlie Reis wrote: > > Please put a comment explaining what this is trying to check, since it's a bit > > subtle. Something like: > > > > Allow the drag if it's within the same process (in which case Blink will > > validate it), or if it's between different pages. Do not allow cross-process > > drags within the same page. > > I think you missed this comment, which I think is worthwhile because of the > Blink validation part. Oh sorry, I meant to reply that the comment is already on this function in the header. https://codereview.chromium.org/2568893002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_view_aura.h (right): https://codereview.chromium.org/2568893002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_view_aura.h:220: // The RenderViewHost may not be in the same process as the RenderProcessHost, On 2016/12/14 18:36:36, Charlie Reis wrote: > nit: s/The/the/ Done.
The CQ bit was checked by paulmeyer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2568893002/#ps80001 (title: "Fixed typo in comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481741648678400,
"parent_rev": "9214b5035d190d66b6b2e9f92d4995e3ba55b7e5", "commit_rev":
"cd8e29080c018bb21e73797eacbf4ede25175c30"}
Message was sent while issue was closed.
Description was changed from ========== Prevent drag-and-drop events from firing over cross-site, same-page frames. This is done by storing the source RenderViewHost and RenderProcessHost for each dragstart. Then, for other drag events (like dragover), if the target frame for that event is in the same RenderViewHost, but different RenderProcessHost, than the source, then the event is not fired. Note that this patch will not affect behavior on Mac, and a subsequent CL will enforce the same thing for Mac. BUG=666858 ========== to ========== Prevent drag-and-drop events from firing over cross-site, same-page frames. This is done by storing the source RenderViewHost and RenderProcessHost for each dragstart. Then, for other drag events (like dragover), if the target frame for that event is in the same RenderViewHost, but different RenderProcessHost, than the source, then the event is not fired. Note that this patch will not affect behavior on Mac, and a subsequent CL will enforce the same thing for Mac. BUG=666858 Review-Url: https://codereview.chromium.org/2568893002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Prevent drag-and-drop events from firing over cross-site, same-page frames. This is done by storing the source RenderViewHost and RenderProcessHost for each dragstart. Then, for other drag events (like dragover), if the target frame for that event is in the same RenderViewHost, but different RenderProcessHost, than the source, then the event is not fired. Note that this patch will not affect behavior on Mac, and a subsequent CL will enforce the same thing for Mac. BUG=666858 Review-Url: https://codereview.chromium.org/2568893002 ========== to ========== Prevent drag-and-drop events from firing over cross-site, same-page frames. This is done by storing the source RenderViewHost and RenderProcessHost for each dragstart. Then, for other drag events (like dragover), if the target frame for that event is in the same RenderViewHost, but different RenderProcessHost, than the source, then the event is not fired. Note that this patch will not affect behavior on Mac, and a subsequent CL will enforce the same thing for Mac. BUG=666858 Committed: https://crrev.com/7dfa247951db2ccb10b8b5276eb58a33cda4728f Cr-Commit-Position: refs/heads/master@{#438609} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7dfa247951db2ccb10b8b5276eb58a33cda4728f Cr-Commit-Position: refs/heads/master@{#438609} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
