Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2636193003:
Fix cross-site subframe navigations that transfer back to original RFH. (Closed)
|
Created:
3 years, 11 months ago by alexmos Modified:
3 years, 9 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix cross-site subframe navigations that transfer back to original RFH.
This fixes a bug where if an OOPIF at site A navigates to site B which
redirects back to A, the navigation timed out because Transfer() was
never called on transfer_navigation_handle_, and the pending RFH for B
was never canceled. This is because UpdateStateForNavigate returned
the current RFH early, due to checking CanSubframeSwapProcess before
reaching those steps. Not calling Transfer() later led to the
transferred request being dropped in
ResourceDispatcherHostImpl::CompleteTransfer, due to not finding a
matching entry in pending_loaders_. This CL fixes the
CanSubframeSwapProcess check to be performed after calling Transfer()
and canceling the pending RFH (if necessary).
This uncovered another complication, which is that while redirects
to data: and about: URLs are explicitly disallowed as unsafe (e.g.,
see DataProtocolHandler::IsSafeRedirectTarget), they are allowed when
an extension uses the webRequest API to redirect certain URLs.
One of the tests, ExtensionWebRequestApiTest.WebRequestDeclarative1,
was exercising this: it had a subframe which made a same-site request
that was redirected to data:,. Previously, in --site-per-process, it
worked due to the ordering problem that this CL fixes: we first
decided to transfer the request (IsRendererTransferNeededForNavigation
returned true), then we returned the current RFH early due to
CanSubframeSwapProcess returning |false| before calling Transfer().
With the fixed ordering, we were now also calling Transfer(), as we
were expecting the data: request to be put into a new SiteInstance
which didn't match the current one, yet the request was never
transferred (we weren't spinning up a new pending RFH, and we weren't
sending a Navigate IPC to the current RFH due to the
is_transfer_to_same case in NavigateToEntry, so no one actually made a
request to the data: URL).
To address this, CanSubframeSwapProcess is changed to allow a process
swap for such transfers to data: and about:. This changes behavior
where these redirects were previously kept in the original
SiteInstance, but this should be safer, and it should not break any
scripting relationships (the redirect URLs are provided by an
extension; the site that made the request that's being redirected
can't expect to script the redirect target).
BUG=681077, 660407, 659613
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2636193003
Cr-Commit-Position: refs/heads/master@{#453800}
Committed: https://chromium.googlesource.com/chromium/src/+/0431b6bc6c9bf5624cccf1121285392b07b74ec6
Patch Set 1 #Patch Set 2 : Fix webRequest test and remove third return path from UpdateStateForNavigate #Patch Set 3 : Second attempt at fix #Patch Set 4 : Experiment #Patch Set 5 : Try #3 #
Total comments: 14
Patch Set 6 : Rebase #Patch Set 7 : Fix PlzNavigate #
Total comments: 2
Patch Set 8 : Rebase #Patch Set 9 : Rebase #
Messages
Total messages: 53 (44 generated)
Description was changed from ========== Fix cross-site subframe navigations that transfer back to original RFH. BUG=681077,660407,659613 ========== to ========== Fix cross-site subframe navigations that transfer back to original RFH. BUG=681077,660407,659613 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by alexmos@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexmos@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by alexmos@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by alexmos@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by alexmos@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix cross-site subframe navigations that transfer back to original RFH. BUG=681077,660407,659613 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix cross-site subframe navigations that transfer back to original RFH. This CL fixes a bug where if an OOPIF at site A navigates to site B which redirects back to A, the navigation timed out because Transfer() was never called on transfer_navigation_handle_, and the pending RFH for B was never canceled. This is because UpdateStateForNavigate never reached those steps and returned the current RFH early, after checking CanSubframeSwapProcess. Now, the current RFH is performed after calling Transfer() and canceling any pending RFH. This fix uncovered another complication, which is that while redirects to data: and about: URLs are explicitly disallowed as unsafe (e.g., see DataProtocolHandler::IsSafeRedirectTarget), they are explicitly allowed when an extension uses declarativeWebRequest API to redirect certain URLs. One of the tests, ExtensionWebRequestApiTest.WebRequestDeclarative1, was exercising this: it had a subframe which made a same-site request that was redirected to data:,. Previously, in --site-per-process, it worked due to the ordering problem that this CL fixes: we first decided to transfer the request (IsRendererTransferNeededForNavigation returned true), then we returned the current RFH early due to CanSubframeSwapProcess returning |false|. With the fixed ordering, we were calling Transfer(), as we were expecting the data: request to be put into a new SiteInstance which didn't match the current one, yet the request was never transferred later on. To avoid this, CanSubframeSwapProcess is changed to swap processes for such redirects to data: and about:. This changes behavior where these redirects were previously kept in the original SiteInstance, but this should be safer. and it should not break any scripting relationships (the redirect URLs are provided by an extension; the site that made the request being redirected can't expect to script the redirect target). BUG=681077,660407,659613 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Fix cross-site subframe navigations that transfer back to original RFH. This CL fixes a bug where if an OOPIF at site A navigates to site B which redirects back to A, the navigation timed out because Transfer() was never called on transfer_navigation_handle_, and the pending RFH for B was never canceled. This is because UpdateStateForNavigate never reached those steps and returned the current RFH early, after checking CanSubframeSwapProcess. Now, the current RFH is performed after calling Transfer() and canceling any pending RFH. This fix uncovered another complication, which is that while redirects to data: and about: URLs are explicitly disallowed as unsafe (e.g., see DataProtocolHandler::IsSafeRedirectTarget), they are explicitly allowed when an extension uses declarativeWebRequest API to redirect certain URLs. One of the tests, ExtensionWebRequestApiTest.WebRequestDeclarative1, was exercising this: it had a subframe which made a same-site request that was redirected to data:,. Previously, in --site-per-process, it worked due to the ordering problem that this CL fixes: we first decided to transfer the request (IsRendererTransferNeededForNavigation returned true), then we returned the current RFH early due to CanSubframeSwapProcess returning |false|. With the fixed ordering, we were calling Transfer(), as we were expecting the data: request to be put into a new SiteInstance which didn't match the current one, yet the request was never transferred later on. To avoid this, CanSubframeSwapProcess is changed to swap processes for such redirects to data: and about:. This changes behavior where these redirects were previously kept in the original SiteInstance, but this should be safer. and it should not break any scripting relationships (the redirect URLs are provided by an extension; the site that made the request being redirected can't expect to script the redirect target). BUG=681077,660407,659613 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix cross-site subframe navigations that transfer back to original RFH. This CL fixes a bug where if an OOPIF at site A navigates to site B which redirects back to A, the navigation timed out because Transfer() was never called on transfer_navigation_handle_, and the pending RFH for B was never canceled. This is because UpdateStateForNavigate returned the current RFH early, due to checking CanSubframeSwapProcess before reaching those steps. Now, the CanSubframeSwapProcess check is performed after calling Transfer() and canceling the pending RFH (if necessary). This fix uncovered another complication, which is that while redirects to data: and about: URLs are explicitly disallowed as unsafe (e.g., see DataProtocolHandler::IsSafeRedirectTarget), they are explicitly allowed when an extension uses declarativeWebRequest API to redirect certain URLs. One of the tests, ExtensionWebRequestApiTest.WebRequestDeclarative1, was exercising this: it had a subframe which made a same-site request that was redirected to data:,. Previously, in --site-per-process, it worked due to the ordering problem that this CL fixes: we first decided to transfer the request (IsRendererTransferNeededForNavigation returned true), then we returned the current RFH early due to CanSubframeSwapProcess returning |false| before calling Transfer(). With the fixed ordering, we were now also calling Transfer(), as we were expecting the data: request to be put into a new SiteInstance which didn't match the current one, yet the request was never transferred (we weren't spinning up a new pending RFH, and we weren't sending a Navigate IPC to the current RFH due to the is_transfer_to_same case in NavigateToEntry, so no one actually made a request to the data: URL). To address this, CanSubframeSwapProcess is changed to allow a process swap for such redirects to data: and about:. This changes behavior where these redirects were previously kept in the original SiteInstance, but this should be safer, and it should not break any scripting relationships (the redirect URLs are provided by an extension; the site that made the request that's being redirected can't expect to script the redirect target). BUG=681077,660407,659613 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Fix cross-site subframe navigations that transfer back to original RFH. This CL fixes a bug where if an OOPIF at site A navigates to site B which redirects back to A, the navigation timed out because Transfer() was never called on transfer_navigation_handle_, and the pending RFH for B was never canceled. This is because UpdateStateForNavigate returned the current RFH early, due to checking CanSubframeSwapProcess before reaching those steps. Now, the CanSubframeSwapProcess check is performed after calling Transfer() and canceling the pending RFH (if necessary). This fix uncovered another complication, which is that while redirects to data: and about: URLs are explicitly disallowed as unsafe (e.g., see DataProtocolHandler::IsSafeRedirectTarget), they are explicitly allowed when an extension uses declarativeWebRequest API to redirect certain URLs. One of the tests, ExtensionWebRequestApiTest.WebRequestDeclarative1, was exercising this: it had a subframe which made a same-site request that was redirected to data:,. Previously, in --site-per-process, it worked due to the ordering problem that this CL fixes: we first decided to transfer the request (IsRendererTransferNeededForNavigation returned true), then we returned the current RFH early due to CanSubframeSwapProcess returning |false| before calling Transfer(). With the fixed ordering, we were now also calling Transfer(), as we were expecting the data: request to be put into a new SiteInstance which didn't match the current one, yet the request was never transferred (we weren't spinning up a new pending RFH, and we weren't sending a Navigate IPC to the current RFH due to the is_transfer_to_same case in NavigateToEntry, so no one actually made a request to the data: URL). To address this, CanSubframeSwapProcess is changed to allow a process swap for such redirects to data: and about:. This changes behavior where these redirects were previously kept in the original SiteInstance, but this should be safer, and it should not break any scripting relationships (the redirect URLs are provided by an extension; the site that made the request that's being redirected can't expect to script the redirect target). BUG=681077,660407,659613 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix cross-site subframe navigations that transfer back to original RFH. This CL fixes a bug where if an OOPIF at site A navigates to site B which redirects back to A, the navigation timed out because Transfer() was never called on transfer_navigation_handle_, and the pending RFH for B was never canceled. This is because UpdateStateForNavigate returned the current RFH early, due to checking CanSubframeSwapProcess before reaching those steps. Now, the CanSubframeSwapProcess check is performed after calling Transfer() and canceling the pending RFH (if necessary). This fix uncovered another complication, which is that while redirects to data: and about: URLs are explicitly disallowed as unsafe (e.g., see DataProtocolHandler::IsSafeRedirectTarget), they are allowed when an extension uses declarativeWebRequest API to redirect certain URLs. One of the tests, ExtensionWebRequestApiTest.WebRequestDeclarative1, was exercising this: it had a subframe which made a same-site request that was redirected to data:,. Previously, in --site-per-process, it worked due to the ordering problem that this CL fixes: we first decided to transfer the request (IsRendererTransferNeededForNavigation returned true), then we returned the current RFH early due to CanSubframeSwapProcess returning |false| before calling Transfer(). With the fixed ordering, we were now also calling Transfer(), as we were expecting the data: request to be put into a new SiteInstance which didn't match the current one, yet the request was never transferred (we weren't spinning up a new pending RFH, and we weren't sending a Navigate IPC to the current RFH due to the is_transfer_to_same case in NavigateToEntry, so no one actually made a request to the data: URL). To address this, CanSubframeSwapProcess is changed to allow a process swap for such redirects to data: and about:. This changes behavior where these redirects were previously kept in the original SiteInstance, but this should be safer, and it should not break any scripting relationships (the redirect URLs are provided by an extension; the site that made the request that's being redirected can't expect to script the redirect target). BUG=681077,660407,659613 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Fix cross-site subframe navigations that transfer back to original RFH. This CL fixes a bug where if an OOPIF at site A navigates to site B which redirects back to A, the navigation timed out because Transfer() was never called on transfer_navigation_handle_, and the pending RFH for B was never canceled. This is because UpdateStateForNavigate returned the current RFH early, due to checking CanSubframeSwapProcess before reaching those steps. Now, the CanSubframeSwapProcess check is performed after calling Transfer() and canceling the pending RFH (if necessary). This fix uncovered another complication, which is that while redirects to data: and about: URLs are explicitly disallowed as unsafe (e.g., see DataProtocolHandler::IsSafeRedirectTarget), they are allowed when an extension uses declarativeWebRequest API to redirect certain URLs. One of the tests, ExtensionWebRequestApiTest.WebRequestDeclarative1, was exercising this: it had a subframe which made a same-site request that was redirected to data:,. Previously, in --site-per-process, it worked due to the ordering problem that this CL fixes: we first decided to transfer the request (IsRendererTransferNeededForNavigation returned true), then we returned the current RFH early due to CanSubframeSwapProcess returning |false| before calling Transfer(). With the fixed ordering, we were now also calling Transfer(), as we were expecting the data: request to be put into a new SiteInstance which didn't match the current one, yet the request was never transferred (we weren't spinning up a new pending RFH, and we weren't sending a Navigate IPC to the current RFH due to the is_transfer_to_same case in NavigateToEntry, so no one actually made a request to the data: URL). To address this, CanSubframeSwapProcess is changed to allow a process swap for such redirects to data: and about:. This changes behavior where these redirects were previously kept in the original SiteInstance, but this should be safer, and it should not break any scripting relationships (the redirect URLs are provided by an extension; the site that made the request that's being redirected can't expect to script the redirect target). BUG=681077,660407,659613 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix cross-site subframe navigations that transfer back to original RFH. This fixes a bug where if an OOPIF at site A navigates to site B which redirects back to A, the navigation timed out because Transfer() was never called on transfer_navigation_handle_, and the pending RFH for B was never canceled. This is because UpdateStateForNavigate returned the current RFH early, due to checking CanSubframeSwapProcess before reaching those steps. Not calling Transfer() later led to the transferred request being dropped in ResourceDispatcherHostImpl::CompleteTransfer, due to not finding a matching entry in pending_loaders_. This CL fixes the CanSubframeSwapProcess check to be performed after calling Transfer() and canceling the pending RFH (if necessary). This uncovered another complication, which is that while redirects to data: and about: URLs are explicitly disallowed as unsafe (e.g., see DataProtocolHandler::IsSafeRedirectTarget), they are allowed when an extension uses declarativeWebRequest API to redirect certain URLs. One of the tests, ExtensionWebRequestApiTest.WebRequestDeclarative1, was exercising this: it had a subframe which made a same-site request that was redirected to data:,. Previously, in --site-per-process, it worked due to the ordering problem that this CL fixes: we first decided to transfer the request (IsRendererTransferNeededForNavigation returned true), then we returned the current RFH early due to CanSubframeSwapProcess returning |false| before calling Transfer(). With the fixed ordering, we were now also calling Transfer(), as we were expecting the data: request to be put into a new SiteInstance which didn't match the current one, yet the request was never transferred (we weren't spinning up a new pending RFH, and we weren't sending a Navigate IPC to the current RFH due to the is_transfer_to_same case in NavigateToEntry, so no one actually made a request to the data: URL). To address this, CanSubframeSwapProcess is changed to allow a process swap for such transfers to data: and about:. This changes behavior where these redirects were previously kept in the original SiteInstance, but this should be safer, and it should not break any scripting relationships (the redirect URLs are provided by an extension; the site that made the request that's being redirected can't expect to script the redirect target). BUG=681077,660407,659613 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
alexmos@chromium.org changed reviewers: + creis@chromium.org
Charlie, can you please take a look? This is a fairly small fix but involves lots of sanity checking. :) We've discussed that the option of leaving the transferred data:/about: URLs in the original SiteInstance isn't as safe, so I'm trying to always kick them out into a new process, but only during transfers (to avoid lots of extra OOPIFs in session restore, for example, which also has source_instance and dest_instance as empty in CanSubframeSwapProcess). https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2325: bool allowed_to_swap_process = I turned this into a flag so that there's just one spot where the current RFH is returned. Now it's always returned at the bottom of this function. There's some more work (WebUI/view-source) that is done on the current RFH below; it seems that it was another bug that we weren't doing those things previously when returning the current RFH early due to CanSubframeSwapProcess. https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2762: // ExtensionWebRequestApiTest.WebRequestDeclarative1). It's safest to For reference, here's where these redirects are performed and explicitly allowed: https://cs.chromium.org/chromium/src/extensions/browser/api/web_request/web_r... // Explicitly mark the URL as safe for redirection, to prevent the request // from being blocked because of net::ERR_UNSAFE_REDIRECT. *allowed_unsafe_redirect_url = new_url; The APIs docs are here: https://developer.chrome.com/extensions/declarativeWebRequest Namely, RedirectToEmptyDocument is implemented as a redirect to "data:," (this is what that test was hitting), and RedirectRequest can be used to redirect an arbitrary URL to another arbitrary URL (I've tried both data: URLs and about:blank in a sample extension). I noticed that these APIs are currently only available on beta and dev, but not stable. https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2770: if (!for_transfer) I don't know if passing in this as a flag is strictly necessary; I did it to match the transfer condition in UpdateStateForNavigate, but perhaps we could also just directly check whether transfer_navigation_handle_ exists? (Not sure if it's possible to get here with it being valid but referencing another global request ID)
Thanks for investigating this, and sorry for the late review! I think the PlzNavigate transfer concern below is the main issue (though I'm also curious about the extension API question, which you might ask Devlin about). https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (left): https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2406: nit: Let's avoid this churn. https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:809: request.dest_site_instance(), false); I think we might have a problem here for PlzNavigate. As mentioned below on line 813, this method is called both before and after redirects in PlzNavigate mode (once when picking a speculative RFH, and once when it's time to commit). I imagine that the transfer bug with redirects to data URLs will apply during the second call. In that sense, we should probably avoid having the fix depend on whether it's a transfer or not (especially since PlzNavigate doesn't have the same concept of transfers). Is there something else that we can use to distinguish the case that data/about URLs require a process swap? https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2325: bool allowed_to_swap_process = On 2017/01/25 22:30:55, alexmos wrote: > I turned this into a flag so that there's just one spot where the current RFH is > returned. Now it's always returned at the bottom of this function. There's > some more work (WebUI/view-source) that is done on the current RFH below; it > seems that it was another bug that we weren't doing those things previously when > returning the current RFH early due to CanSubframeSwapProcess. Acknowledged. https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2762: // ExtensionWebRequestApiTest.WebRequestDeclarative1). It's safest to On 2017/01/25 22:30:55, alexmos wrote: > For reference, here's where these redirects are performed and explicitly > allowed: > > https://cs.chromium.org/chromium/src/extensions/browser/api/web_request/web_r... > > // Explicitly mark the URL as safe for redirection, to prevent the request > // from being blocked because of net::ERR_UNSAFE_REDIRECT. > *allowed_unsafe_redirect_url = new_url; > > The APIs docs are here: > > https://developer.chrome.com/extensions/declarativeWebRequest > > Namely, RedirectToEmptyDocument is implemented as a redirect to "data:," (this > is what that test was hitting), and RedirectRequest can be used to redirect an > arbitrary URL to another arbitrary URL (I've tried both data: URLs and > about:blank in a sample extension). > > I noticed that these APIs are currently only available on beta and dev, but not > stable. Interesting! I did just hear Devlin mention that the declarativeWebRequest API didn't ship. Is that the only way for extensions to do this, or can they do it via the normal web request API? (I'm wondering if we can avoid this problem by disallowing the behavior in extensions. That might be the case if the declarativeWebRequest API is going to be removed and if nothing else can do this. Probably overly optimistic, but worth checking.) https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2764: // OOPIF-enabled mode. Sounds like we want this to return true for these cases, but it looks like it depends on what IsRendererTransferNeededForNavigation returns for dest_url. There's a lot of ways for that to return false, so I'm wondering if that can be a problem. https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2770: if (!for_transfer) On 2017/01/25 22:30:55, alexmos (at BlinkOn until 2-2) wrote: > I don't know if passing in this as a flag is strictly necessary; I did it to > match the transfer condition in UpdateStateForNavigate, but perhaps we could > also just directly check whether transfer_navigation_handle_ exists? (Not sure > if it's possible to get here with it being valid but referencing another global > request ID) (See my earlier question about PlzNavigate.)
The CQ bit was checked by alexmos@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Thanks for the review, Charlie! I've fixed the approach to also work for PlzNavigate. Let me know what you think. https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (left): https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2406: On 2017/02/02 22:40:46, Charlie Reis wrote: > nit: Let's avoid this churn. Done. https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:809: request.dest_site_instance(), false); On 2017/02/02 22:40:46, Charlie Reis wrote: > I think we might have a problem here for PlzNavigate. As mentioned below on > line 813, this method is called both before and after redirects in PlzNavigate > mode (once when picking a speculative RFH, and once when it's time to commit). > I imagine that the transfer bug with redirects to data URLs will apply during > the second call. > > In that sense, we should probably avoid having the fix depend on whether it's a > transfer or not (especially since PlzNavigate doesn't have the same concept of > transfers). Is there something else that we can use to distinguish the case > that data/about URLs require a process swap? Yes, getting this working with PlzNavigate needed a bit more work. I switched my approach to be based on whether or not a redirect happened, rather than a transfer, which also makes sense with PlzNavigate, and better matches the scenario we care about (redirects to data: or about:). With PlzNavigate, however, passing that into CanSubframeSwapProcess still didn't lead to a new SiteInstance for the data: URL. This is because PlzNavigate preserves the |source_instance| through redirects (unlike the transfer path), and as part of figuring out |dest_site_instance| above, the following path ran in RFHM::DetermineSiteInstanceForURL: // Use the source SiteInstance in case of data URLs, about:srcdoc pages and // about:blank pages because the content is then controlled and/or scriptable // by the source SiteInstance. GURL about_blank(url::kAboutBlankURL); GURL about_srcdoc(content::kAboutSrcDocURL); if (source_instance && (dest_url == about_srcdoc || dest_url == about_blank || dest_url.scheme() == url::kDataScheme)) { return SiteInstanceDescriptor(source_instance); } I.e., if we have A-embed-B, and A navigates the subframe to a URL that gets redirected to data:, this would cause the data: URL to be placed into the source (A) SiteInstance. The |source_instance| shouldn't apply if we reach this via a redirect (since the content is controlled by an extension, not source_instance), so I ended up plumbing the redirect info there as well. Let me know what you think about that. https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2762: // ExtensionWebRequestApiTest.WebRequestDeclarative1). It's safest to On 2017/02/02 22:40:46, Charlie Reis wrote: > On 2017/01/25 22:30:55, alexmos wrote: > > For reference, here's where these redirects are performed and explicitly > > allowed: > > > > > https://cs.chromium.org/chromium/src/extensions/browser/api/web_request/web_r... > > > > // Explicitly mark the URL as safe for redirection, to prevent the request > > // from being blocked because of net::ERR_UNSAFE_REDIRECT. > > *allowed_unsafe_redirect_url = new_url; > > > > The APIs docs are here: > > > > https://developer.chrome.com/extensions/declarativeWebRequest > > > > Namely, RedirectToEmptyDocument is implemented as a redirect to "data:," (this > > is what that test was hitting), and RedirectRequest can be used to redirect an > > arbitrary URL to another arbitrary URL (I've tried both data: URLs and > > about:blank in a sample extension). > > > > I noticed that these APIs are currently only available on beta and dev, but > not > > stable. > > Interesting! I did just hear Devlin mention that the declarativeWebRequest API > didn't ship. Is that the only way for extensions to do this, or can they do it > via the normal web request API? > > (I'm wondering if we can avoid this problem by disallowing the behavior in > extensions. That might be the case if the declarativeWebRequest API is going to > be removed and if nothing else can do this. Probably overly optimistic, but > worth checking.) Unfortunately, this is a problem for regular webRequest as well. I've confirmed this with a toy extension by setting |redirectUrl| in onBeforeRequest. The docs also explicitly allow this: "Redirections to non-HTTP schemes such as data: are allowed." (https://developer.chrome.com/extensions/webRequest). I've updated the comment. It might be worth adding an explicit test for this too (for regular webRequest redirecting to a data URL). I did talk to Devlin about the declarativeWebRequest also, and he mentioned that while it didn't ship, some of the stuff there might be the only way to do things for extensions on mobile, so it's better to support them if we can. https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2764: // OOPIF-enabled mode. On 2017/02/02 22:40:46, Charlie Reis wrote: > Sounds like we want this to return true for these cases, but it looks like it > depends on what IsRendererTransferNeededForNavigation returns for dest_url. > There's a lot of ways for that to return false, so I'm wondering if that can be > a problem. Yes, for instance we don't want to return true here when we're not in an appropriate OOPIF mode. (--site-per-process or TDI) As for IsRendererTransferNeededForNavigation returning false, I don't think that's a problem because the only way we could get into here for a redirect to data: is if IsRendererTransferNeededForNavigation returned true earlier when deciding whether or not a transfer was needed. (This is the call in MaybeTransferAndProceedInternal.) So it should also return true here the second time. https://codereview.chromium.org/2636193003/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2636193003/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1464: if (source_instance && dest_is_data_or_about && !was_server_redirect) Note that I fall through instead of returning a new SiteInstance if this was a data/about URL and a redirect. This is because the TDI paths will place it into the subframe process (which I think makes sense?), and --site-per-process will place it into a new (data:) SiteInstance. (I did check that this actually works like this with TDI.)
Description was changed from ========== Fix cross-site subframe navigations that transfer back to original RFH. This fixes a bug where if an OOPIF at site A navigates to site B which redirects back to A, the navigation timed out because Transfer() was never called on transfer_navigation_handle_, and the pending RFH for B was never canceled. This is because UpdateStateForNavigate returned the current RFH early, due to checking CanSubframeSwapProcess before reaching those steps. Not calling Transfer() later led to the transferred request being dropped in ResourceDispatcherHostImpl::CompleteTransfer, due to not finding a matching entry in pending_loaders_. This CL fixes the CanSubframeSwapProcess check to be performed after calling Transfer() and canceling the pending RFH (if necessary). This uncovered another complication, which is that while redirects to data: and about: URLs are explicitly disallowed as unsafe (e.g., see DataProtocolHandler::IsSafeRedirectTarget), they are allowed when an extension uses declarativeWebRequest API to redirect certain URLs. One of the tests, ExtensionWebRequestApiTest.WebRequestDeclarative1, was exercising this: it had a subframe which made a same-site request that was redirected to data:,. Previously, in --site-per-process, it worked due to the ordering problem that this CL fixes: we first decided to transfer the request (IsRendererTransferNeededForNavigation returned true), then we returned the current RFH early due to CanSubframeSwapProcess returning |false| before calling Transfer(). With the fixed ordering, we were now also calling Transfer(), as we were expecting the data: request to be put into a new SiteInstance which didn't match the current one, yet the request was never transferred (we weren't spinning up a new pending RFH, and we weren't sending a Navigate IPC to the current RFH due to the is_transfer_to_same case in NavigateToEntry, so no one actually made a request to the data: URL). To address this, CanSubframeSwapProcess is changed to allow a process swap for such transfers to data: and about:. This changes behavior where these redirects were previously kept in the original SiteInstance, but this should be safer, and it should not break any scripting relationships (the redirect URLs are provided by an extension; the site that made the request that's being redirected can't expect to script the redirect target). BUG=681077,660407,659613 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix cross-site subframe navigations that transfer back to original RFH. This fixes a bug where if an OOPIF at site A navigates to site B which redirects back to A, the navigation timed out because Transfer() was never called on transfer_navigation_handle_, and the pending RFH for B was never canceled. This is because UpdateStateForNavigate returned the current RFH early, due to checking CanSubframeSwapProcess before reaching those steps. Not calling Transfer() later led to the transferred request being dropped in ResourceDispatcherHostImpl::CompleteTransfer, due to not finding a matching entry in pending_loaders_. This CL fixes the CanSubframeSwapProcess check to be performed after calling Transfer() and canceling the pending RFH (if necessary). This uncovered another complication, which is that while redirects to data: and about: URLs are explicitly disallowed as unsafe (e.g., see DataProtocolHandler::IsSafeRedirectTarget), they are allowed when an extension uses the webRequest API to redirect certain URLs. One of the tests, ExtensionWebRequestApiTest.WebRequestDeclarative1, was exercising this: it had a subframe which made a same-site request that was redirected to data:,. Previously, in --site-per-process, it worked due to the ordering problem that this CL fixes: we first decided to transfer the request (IsRendererTransferNeededForNavigation returned true), then we returned the current RFH early due to CanSubframeSwapProcess returning |false| before calling Transfer(). With the fixed ordering, we were now also calling Transfer(), as we were expecting the data: request to be put into a new SiteInstance which didn't match the current one, yet the request was never transferred (we weren't spinning up a new pending RFH, and we weren't sending a Navigate IPC to the current RFH due to the is_transfer_to_same case in NavigateToEntry, so no one actually made a request to the data: URL). To address this, CanSubframeSwapProcess is changed to allow a process swap for such transfers to data: and about:. This changes behavior where these redirects were previously kept in the original SiteInstance, but this should be safer, and it should not break any scripting relationships (the redirect URLs are provided by an extension; the site that made the request that's being redirected can't expect to script the redirect target). BUG=681077,660407,659613 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Hey Charlie - just a gentle ping to bump this up in your review queue. I think this is ready for another look whenever you get a chance. Thanks!
Thanks for working through the PlzNavigate case, and sorry for the delay. New approach LGTM! https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2636193003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:809: request.dest_site_instance(), false); On 2017/02/14 21:16:06, alexmos wrote: > On 2017/02/02 22:40:46, Charlie Reis wrote: > > I think we might have a problem here for PlzNavigate. As mentioned below on > > line 813, this method is called both before and after redirects in PlzNavigate > > mode (once when picking a speculative RFH, and once when it's time to commit). > > > I imagine that the transfer bug with redirects to data URLs will apply during > > the second call. > > > > In that sense, we should probably avoid having the fix depend on whether it's > a > > transfer or not (especially since PlzNavigate doesn't have the same concept of > > transfers). Is there something else that we can use to distinguish the case > > that data/about URLs require a process swap? > > Yes, getting this working with PlzNavigate needed a bit more work. I switched > my approach to be based on whether or not a redirect happened, rather than a > transfer, which also makes sense with PlzNavigate, and better matches the > scenario we care about (redirects to data: or about:). > > With PlzNavigate, however, passing that into CanSubframeSwapProcess still didn't > lead to a new SiteInstance for the data: URL. This is because PlzNavigate > preserves the |source_instance| through redirects (unlike the transfer path), > and as part of figuring out |dest_site_instance| above, the following path ran > in RFHM::DetermineSiteInstanceForURL: > > // Use the source SiteInstance in case of data URLs, about:srcdoc pages and > // about:blank pages because the content is then controlled and/or scriptable > // by the source SiteInstance. > GURL about_blank(url::kAboutBlankURL); > GURL about_srcdoc(content::kAboutSrcDocURL); > if (source_instance && (dest_url == about_srcdoc || dest_url == about_blank || > dest_url.scheme() == url::kDataScheme)) { > return SiteInstanceDescriptor(source_instance); > } > > I.e., if we have A-embed-B, and A navigates the subframe to a URL that gets > redirected to data:, this would cause the data: URL to be placed into the source > (A) SiteInstance. The |source_instance| shouldn't apply if we reach this via a > redirect (since the content is controlled by an extension, not source_instance), > so I ended up plumbing the redirect info there as well. Let me know what you > think about that. Yes, that sounds like the right thing to do. Thanks! https://codereview.chromium.org/2636193003/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2636193003/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1464: if (source_instance && dest_is_data_or_about && !was_server_redirect) On 2017/02/14 21:16:07, alexmos wrote: > Note that I fall through instead of returning a new SiteInstance if this was a > data/about URL and a redirect. This is because the TDI paths will place it into > the subframe process (which I think makes sense?), and --site-per-process will > place it into a new (data:) SiteInstance. (I did check that this actually works > like this with TDI.) Acknowledged.
The CQ bit was checked by alexmos@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by alexmos@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...
The CQ bit was unchecked by alexmos@chromium.org
The CQ bit was checked by alexmos@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/2636193003/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexmos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488329339912550,
"parent_rev": "3fc8c8329936a042def389cd18e95269dec21f21", "commit_rev":
"0431b6bc6c9bf5624cccf1121285392b07b74ec6"}
Message was sent while issue was closed.
Description was changed from ========== Fix cross-site subframe navigations that transfer back to original RFH. This fixes a bug where if an OOPIF at site A navigates to site B which redirects back to A, the navigation timed out because Transfer() was never called on transfer_navigation_handle_, and the pending RFH for B was never canceled. This is because UpdateStateForNavigate returned the current RFH early, due to checking CanSubframeSwapProcess before reaching those steps. Not calling Transfer() later led to the transferred request being dropped in ResourceDispatcherHostImpl::CompleteTransfer, due to not finding a matching entry in pending_loaders_. This CL fixes the CanSubframeSwapProcess check to be performed after calling Transfer() and canceling the pending RFH (if necessary). This uncovered another complication, which is that while redirects to data: and about: URLs are explicitly disallowed as unsafe (e.g., see DataProtocolHandler::IsSafeRedirectTarget), they are allowed when an extension uses the webRequest API to redirect certain URLs. One of the tests, ExtensionWebRequestApiTest.WebRequestDeclarative1, was exercising this: it had a subframe which made a same-site request that was redirected to data:,. Previously, in --site-per-process, it worked due to the ordering problem that this CL fixes: we first decided to transfer the request (IsRendererTransferNeededForNavigation returned true), then we returned the current RFH early due to CanSubframeSwapProcess returning |false| before calling Transfer(). With the fixed ordering, we were now also calling Transfer(), as we were expecting the data: request to be put into a new SiteInstance which didn't match the current one, yet the request was never transferred (we weren't spinning up a new pending RFH, and we weren't sending a Navigate IPC to the current RFH due to the is_transfer_to_same case in NavigateToEntry, so no one actually made a request to the data: URL). To address this, CanSubframeSwapProcess is changed to allow a process swap for such transfers to data: and about:. This changes behavior where these redirects were previously kept in the original SiteInstance, but this should be safer, and it should not break any scripting relationships (the redirect URLs are provided by an extension; the site that made the request that's being redirected can't expect to script the redirect target). BUG=681077,660407,659613 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix cross-site subframe navigations that transfer back to original RFH. This fixes a bug where if an OOPIF at site A navigates to site B which redirects back to A, the navigation timed out because Transfer() was never called on transfer_navigation_handle_, and the pending RFH for B was never canceled. This is because UpdateStateForNavigate returned the current RFH early, due to checking CanSubframeSwapProcess before reaching those steps. Not calling Transfer() later led to the transferred request being dropped in ResourceDispatcherHostImpl::CompleteTransfer, due to not finding a matching entry in pending_loaders_. This CL fixes the CanSubframeSwapProcess check to be performed after calling Transfer() and canceling the pending RFH (if necessary). This uncovered another complication, which is that while redirects to data: and about: URLs are explicitly disallowed as unsafe (e.g., see DataProtocolHandler::IsSafeRedirectTarget), they are allowed when an extension uses the webRequest API to redirect certain URLs. One of the tests, ExtensionWebRequestApiTest.WebRequestDeclarative1, was exercising this: it had a subframe which made a same-site request that was redirected to data:,. Previously, in --site-per-process, it worked due to the ordering problem that this CL fixes: we first decided to transfer the request (IsRendererTransferNeededForNavigation returned true), then we returned the current RFH early due to CanSubframeSwapProcess returning |false| before calling Transfer(). With the fixed ordering, we were now also calling Transfer(), as we were expecting the data: request to be put into a new SiteInstance which didn't match the current one, yet the request was never transferred (we weren't spinning up a new pending RFH, and we weren't sending a Navigate IPC to the current RFH due to the is_transfer_to_same case in NavigateToEntry, so no one actually made a request to the data: URL). To address this, CanSubframeSwapProcess is changed to allow a process swap for such transfers to data: and about:. This changes behavior where these redirects were previously kept in the original SiteInstance, but this should be safer, and it should not break any scripting relationships (the redirect URLs are provided by an extension; the site that made the request that's being redirected can't expect to script the redirect target). BUG=681077,660407,659613 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2636193003 Cr-Commit-Position: refs/heads/master@{#453800} Committed: https://chromium.googlesource.com/chromium/src/+/0431b6bc6c9bf5624cccf1121285... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/0431b6bc6c9bf5624cccf1121285... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
