Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1736393002:
Remove incorrect sanity checks in RenderFrameHostManager::OnCrossSiteResponse. (Closed)
|
Created:
4 years, 9 months ago by alexmos Modified:
4 years, 9 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove incorrect sanity checks in RenderFrameHostManager::OnCrossSiteResponse.
The existing DCHECKs implied that having a pending RenderFrameHost
means that it was the one that made the request, but this is not
necessarily the case. For example, suppose that during a pending
cross-site navigation, the frame performs a different same-site
navigation which redirects cross-site. In this case, there will be a
pending RenderFrameHost, but this request is made by the current
RenderFrameHost.
It should be ok to remove these DCHECKS as there is another DCHECK for
verifying that the transfer came from either the pending or current
RFH, which gets most of the desired coverage.
I hit this while working on blocking frames due to CSP/X-Frame-Options
with --site-per-process (trying to navigate a blocked frame
cross-site), and split this off from
https://codereview.chromium.org/1710283003/.
BUG=584845
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/8b16a4f748cd849186289bba79e215cacc310561
Cr-Commit-Position: refs/heads/master@{#378111}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Remove DCHECKs #Patch Set 3 : Rebase #Messages
Total messages: 14 (7 generated)
Description was changed from ========== Relax sanity check in RenderFrameHostManager::OnCrossSiteResponse. Having a pending RenderFrameHost does not imply that it was the one that made the request, so relax the DCHECKs to allow for this. For example, suppose that during a pending cross-site navigation, the frame performs a different same-site navigation which redirects cross-site. In this case, there will be a pending RenderFrameHost, but this request is made by the current RenderFrameHost. I hit this while working on blocking frames due to CSP/X-Frame-Options with --site-per-process (trying to navigate a blocked frame cross-site), and split this off from https://codereview.chromium.org/1710283003/. BUG=584845 ========== to ========== Relax sanity check in RenderFrameHostManager::OnCrossSiteResponse. Having a pending RenderFrameHost does not imply that it was the one that made the request, so relax the DCHECKs to allow for this. For example, suppose that during a pending cross-site navigation, the frame performs a different same-site navigation which redirects cross-site. In this case, there will be a pending RenderFrameHost, but this request is made by the current RenderFrameHost. I hit this while working on blocking frames due to CSP/X-Frame-Options with --site-per-process (trying to navigate a blocked frame cross-site), and split this off from https://codereview.chromium.org/1710283003/. BUG=584845 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
alexmos@chromium.org changed reviewers: + creis@chromium.org
Charlie, please take a look. I agree this is good to land on its own, so I split this off from https://codereview.chromium.org/1710283003/.
Looking closer, I'm wondering if we want to drop these DCHECKs in favor of the one on line 438. We could move a version of your comment there, to make it clear that it could be either one. Reasoning below. (Correct me if I'm wrong!) https://codereview.chromium.org/1736393002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1736393002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:422: RenderFrameHostImpl* pending_render_frame_host, Maybe we should rename this while we're at it? Something like transferring_render_frame_host? (I'm open to better suggestions, but the current name is admittedly confusing.) https://codereview.chromium.org/1736393002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:478: request_routing_id == render_frame_host_->GetRoutingID()); This seems redundant with line 438. (It kind of was before, too, except for the case where it was wrong.) :) We're basically just saying that the RFH parameter should match either the current or pending RFH, and that's handled above. https://codereview.chromium.org/1736393002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:479: int process_id = request_routing_id == pending_routing_id ? Looking closer, I don't think this first part is right. The pending and current RFHs could have the same routing ID but different process IDs, and this would make us pick the pending RFH's process ID even if the request came from the current RFH. But maybe we don't need this. The process_id DCHECK below is basically just comparing the RFH parameter's GetProcess()->GetID() with global_request_id.child_id now. We could do that if we wanted, but it doesn't seem likely to fail in practice.
Description was changed from ========== Relax sanity check in RenderFrameHostManager::OnCrossSiteResponse. Having a pending RenderFrameHost does not imply that it was the one that made the request, so relax the DCHECKs to allow for this. For example, suppose that during a pending cross-site navigation, the frame performs a different same-site navigation which redirects cross-site. In this case, there will be a pending RenderFrameHost, but this request is made by the current RenderFrameHost. I hit this while working on blocking frames due to CSP/X-Frame-Options with --site-per-process (trying to navigate a blocked frame cross-site), and split this off from https://codereview.chromium.org/1710283003/. BUG=584845 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Remove incorrect sanity checks in RenderFrameHostManager::OnCrossSiteResponse. The existing DCHECKs implied that having a pending RenderFrameHost means that it was the one that made the request, but this is not necessarily the case. For example, suppose that during a pending cross-site navigation, the frame performs a different same-site navigation which redirects cross-site. In this case, there will be a pending RenderFrameHost, but this request is made by the current RenderFrameHost. It should be ok to remove these DCHECKS as there is another DCHECK for verifying that the transfer came from either the pending or current RFH, which gets most of the desired coverage. I hit this while working on blocking frames due to CSP/X-Frame-Options with --site-per-process (trying to navigate a blocked frame cross-site), and split this off from https://codereview.chromium.org/1710283003/. BUG=584845 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
https://codereview.chromium.org/1736393002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1736393002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:422: RenderFrameHostImpl* pending_render_frame_host, On 2016/02/26 23:51:19, Charlie Reis (slow til Mar 7) wrote: > Maybe we should rename this while we're at it? Something like > transferring_render_frame_host? (I'm open to better suggestions, but the > current name is admittedly confusing.) Good idea, done. https://codereview.chromium.org/1736393002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:478: request_routing_id == render_frame_host_->GetRoutingID()); On 2016/02/26 23:51:19, Charlie Reis (slow til Mar 7) wrote: > This seems redundant with line 438. (It kind of was before, too, except for the > case where it was wrong.) :) > > We're basically just saying that the RFH parameter should match either the > current or pending RFH, and that's handled above. Acknowledged. https://codereview.chromium.org/1736393002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:479: int process_id = request_routing_id == pending_routing_id ? On 2016/02/26 23:51:19, Charlie Reis (slow til Mar 7) wrote: > Looking closer, I don't think this first part is right. The pending and current > RFHs could have the same routing ID but different process IDs, and this would > make us pick the pending RFH's process ID even if the request came from the > current RFH. > > But maybe we don't need this. The process_id DCHECK below is basically just > comparing the RFH parameter's GetProcess()->GetID() with > global_request_id.child_id now. We could do that if we wanted, but it doesn't > seem likely to fail in practice. Indeed, you're right. Thanks for catching it! I agree the check on line 438 is enough, so I've removed both of these DCHECKs and merged the comment here to line 438.
Description was changed from ========== Remove incorrect sanity checks in RenderFrameHostManager::OnCrossSiteResponse. The existing DCHECKs implied that having a pending RenderFrameHost means that it was the one that made the request, but this is not necessarily the case. For example, suppose that during a pending cross-site navigation, the frame performs a different same-site navigation which redirects cross-site. In this case, there will be a pending RenderFrameHost, but this request is made by the current RenderFrameHost. It should be ok to remove these DCHECKS as there is another DCHECK for verifying that the transfer came from either the pending or current RFH, which gets most of the desired coverage. I hit this while working on blocking frames due to CSP/X-Frame-Options with --site-per-process (trying to navigate a blocked frame cross-site), and split this off from https://codereview.chromium.org/1710283003/. BUG=584845 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Remove incorrect sanity checks in RenderFrameHostManager::OnCrossSiteResponse. The existing DCHECKs implied that having a pending RenderFrameHost means that it was the one that made the request, but this is not necessarily the case. For example, suppose that during a pending cross-site navigation, the frame performs a different same-site navigation which redirects cross-site. In this case, there will be a pending RenderFrameHost, but this request is made by the current RenderFrameHost. It should be ok to remove these DCHECKS as there is another DCHECK for verifying that the transfer came from either the pending or current RFH, which gets most of the desired coverage. I hit this while working on blocking frames due to CSP/X-Frame-Options with --site-per-process (trying to navigate a blocked frame cross-site), and split this off from https://codereview.chromium.org/1710283003/. BUG=584845 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Thanks! LGTM.
The CQ bit was checked by alexmos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1736393002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1736393002/40001
Message was sent while issue was closed.
Description was changed from ========== Remove incorrect sanity checks in RenderFrameHostManager::OnCrossSiteResponse. The existing DCHECKs implied that having a pending RenderFrameHost means that it was the one that made the request, but this is not necessarily the case. For example, suppose that during a pending cross-site navigation, the frame performs a different same-site navigation which redirects cross-site. In this case, there will be a pending RenderFrameHost, but this request is made by the current RenderFrameHost. It should be ok to remove these DCHECKS as there is another DCHECK for verifying that the transfer came from either the pending or current RFH, which gets most of the desired coverage. I hit this while working on blocking frames due to CSP/X-Frame-Options with --site-per-process (trying to navigate a blocked frame cross-site), and split this off from https://codereview.chromium.org/1710283003/. BUG=584845 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Remove incorrect sanity checks in RenderFrameHostManager::OnCrossSiteResponse. The existing DCHECKs implied that having a pending RenderFrameHost means that it was the one that made the request, but this is not necessarily the case. For example, suppose that during a pending cross-site navigation, the frame performs a different same-site navigation which redirects cross-site. In this case, there will be a pending RenderFrameHost, but this request is made by the current RenderFrameHost. It should be ok to remove these DCHECKS as there is another DCHECK for verifying that the transfer came from either the pending or current RFH, which gets most of the desired coverage. I hit this while working on blocking frames due to CSP/X-Frame-Options with --site-per-process (trying to navigate a blocked frame cross-site), and split this off from https://codereview.chromium.org/1710283003/. BUG=584845 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Remove incorrect sanity checks in RenderFrameHostManager::OnCrossSiteResponse. The existing DCHECKs implied that having a pending RenderFrameHost means that it was the one that made the request, but this is not necessarily the case. For example, suppose that during a pending cross-site navigation, the frame performs a different same-site navigation which redirects cross-site. In this case, there will be a pending RenderFrameHost, but this request is made by the current RenderFrameHost. It should be ok to remove these DCHECKS as there is another DCHECK for verifying that the transfer came from either the pending or current RFH, which gets most of the desired coverage. I hit this while working on blocking frames due to CSP/X-Frame-Options with --site-per-process (trying to navigate a blocked frame cross-site), and split this off from https://codereview.chromium.org/1710283003/. BUG=584845 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Remove incorrect sanity checks in RenderFrameHostManager::OnCrossSiteResponse. The existing DCHECKs implied that having a pending RenderFrameHost means that it was the one that made the request, but this is not necessarily the case. For example, suppose that during a pending cross-site navigation, the frame performs a different same-site navigation which redirects cross-site. In this case, there will be a pending RenderFrameHost, but this request is made by the current RenderFrameHost. It should be ok to remove these DCHECKS as there is another DCHECK for verifying that the transfer came from either the pending or current RFH, which gets most of the desired coverage. I hit this while working on blocking frames due to CSP/X-Frame-Options with --site-per-process (trying to navigate a blocked frame cross-site), and split this off from https://codereview.chromium.org/1710283003/. BUG=584845 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/8b16a4f748cd849186289bba79e215cacc310561 Cr-Commit-Position: refs/heads/master@{#378111} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8b16a4f748cd849186289bba79e215cacc310561 Cr-Commit-Position: refs/heads/master@{#378111} |
