SiteInstanceImpl::DoesSiteRequireDedicatedProcess needs to accommodate non-UI-thread security checks |
||
Issue descriptionToday, some security checks (i.e. ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin) are done outside of the UI thread. Some examples: - content::WebDatabaseHostImpl::ValidateOrigin which runs on TaskSchedulerForegroundWorker (not on UI nor IO thread). - content::ResourceDispatcherHostImpl::ContinuePendingBeginRequest which runs on the IO thread uses CPSPI::CanAccessDataForOrigin (in addition to CPSPI::CanReadRawCookies). - content::SessionStorageNamespaceImplMojo::OpenArea which runs on the IO thread - content::RenderFrameMessageFilter::GetCookies which runs on the IO thread Having ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin run outside of the UI thread is problematic, because: 1. In the future GetSiteForURL (see issue 787576) may need to call into DoesSiteRequireDedicatedProcess and its overrides (//headless, isolate-extensions, *.is isolation in some content_browsertests) only work on the UI thread 2. In the future isolated origins may be associated with a specific BrowserContext (instead of being global) and that may require either 2a) checking isolated origins on the UI thread only or 2b) making isolated origins accessible from any thread (keying them off of a newly introduced profile_id that is accessible (via BrowserContext/ResourceContext) on both the UI and IO threads (and TaskSchedulerForegroundWorker thread!?). Let's use this bug to figure out what can be done here. Options we have considered so far (not necessarily independent / can be done together): A. Move the checks to the UI thread B. Remove DoesSiteRequireDedicatedProcess C. Introduce thread-safe profile_id/browser_or_resource_context_id
,
Oct 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1bc1284bc697e389c93f6768b0bdd0e8da3f47f2 commit 1bc1284bc697e389c93f6768b0bdd0e8da3f47f2 Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Wed Oct 31 17:50:41 2018 Removing LayoutTestContentBrowserClient::DoesSiteRequireDedicatedProcess Why remove DoesSiteRequireDedicatedProcess ========================================== This CL removes LayoutTestContentBrowserClient::DoesSiteRequireDedicatedProcess. We plan to remove other overrides of this ContentBrowserClient method in follow-up CLs. We want to remove this ContentBrowserClient method altogether, because 1) it is currently the only reason SiteInstanceImpl::DetermineProcessLockURL needs to take BrowserContext* as an argument (and therefore is problematic on threads other than UI thread) 2) the method was initially introduced to support --isolate-extensions which has been obsolete since shipping --site-per-process in M67. Removal mechanics ================= This CL replaces LayoutTestContentBrowserClient::DoesSiteRequireDedicatedProcess with LayoutTestContentBrowserClient::GetOriginsRequiringDedicatedProcess (i.e. mechanism based on --isolate-origin functionality). Bug: 898281 Change-Id: I3d7cfa7cce6077d2d7b5af24089d6e3d212b8e73 Reviewed-on: https://chromium-review.googlesource.com/c/1307848 Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#604322} [modify] https://crrev.com/1bc1284bc697e389c93f6768b0bdd0e8da3f47f2/content/shell/browser/layout_test/layout_test_content_browser_client.cc [modify] https://crrev.com/1bc1284bc697e389c93f6768b0bdd0e8da3f47f2/content/shell/browser/layout_test/layout_test_content_browser_client.h
,
Nov 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bcb53c0c4adcbb4c4855610c78d999c26dcd3573 commit bcb53c0c4adcbb4c4855610c78d999c26dcd3573 Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Sat Nov 03 00:13:22 2018 Remove WebAuthBrowserTest...Client::DoesSiteRequireDedicatedProcess. Why remove DoesSiteRequireDedicatedProcess ========================================== This CL removes ShellContentBrowserClient::DoesSiteRequireDedicatedProcess. We plan to remove 2 other overrides of this ContentBrowserClient method in other CLs. We want to remove this ContentBrowserClient method altogether, because 1) it is currently the only reason SiteInstanceImpl::DetermineProcessLockURL needs to take BrowserContext* as an argument (and therefore is problematic on threads other than UI thread) 2) the method was initially introduced to support --isolate-extensions which has been obsolete since shipping --site-per-process in M67. Removal mechanics ================= agl@ (original author of the test) suggested to simply remove WebAuthJavascriptClientBrowserTest.RegisterDuringUnload for now. Change-Id: I5aeb3b113de642ef96d7a9abc35d0e9c42c45f86 Bug: 898281 Reviewed-on: https://chromium-review.googlesource.com/c/1308256 Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Adam Langley <agl@chromium.org> Cr-Commit-Position: refs/heads/master@{#605102} [modify] https://crrev.com/bcb53c0c4adcbb4c4855610c78d999c26dcd3573/content/browser/webauth/webauth_browsertest.cc
,
Nov 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e7c87d16c277d479a83e35089edbe3d32a912653 commit e7c87d16c277d479a83e35089edbe3d32a912653 Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Sat Nov 03 02:53:34 2018 Removing ShellContentBrowserClient::DoesSiteRequireDedicatedProcess. Why remove DoesSiteRequireDedicatedProcess ========================================== This CL removes ShellContentBrowserClient::DoesSiteRequireDedicatedProcess. We plan to remove 2 other overrides of this ContentBrowserClient method in other CLs. We want to remove this ContentBrowserClient method altogether, because 1) it is currently the only reason SiteInstanceImpl::DetermineProcessLockURL needs to take BrowserContext* as an argument (and therefore is problematic on threads other than UI thread) 2) the method was initially introduced to support --isolate-extensions which has been obsolete since shipping --site-per-process in M67. Removal mechanics ================= It seems that NavigationControllerOopifBrowserTest test never relied on switches::kIsolateSitesForTesting. In particular, I can't find any references to ".is" in https://codereview.chromium.org/1505343002/patch/100001/110001 IsolateIcelandFrameTreeBrowserTest can switch from using switches::kIsolateSitesForTesting to using switches::kIsolateOrigins. Bug: 898281 Change-Id: Idc8e45361e791ff30b9391666c55cf10159e793a Reviewed-on: https://chromium-review.googlesource.com/c/1307876 Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Cr-Commit-Position: refs/heads/master@{#605137} [modify] https://crrev.com/e7c87d16c277d479a83e35089edbe3d32a912653/content/browser/frame_host/frame_tree_browsertest.cc [modify] https://crrev.com/e7c87d16c277d479a83e35089edbe3d32a912653/content/browser/frame_host/navigation_controller_impl_browsertest.cc [modify] https://crrev.com/e7c87d16c277d479a83e35089edbe3d32a912653/content/shell/browser/shell_content_browser_client.cc [modify] https://crrev.com/e7c87d16c277d479a83e35089edbe3d32a912653/content/shell/browser/shell_content_browser_client.h [modify] https://crrev.com/e7c87d16c277d479a83e35089edbe3d32a912653/content/shell/common/shell_switches.cc [modify] https://crrev.com/e7c87d16c277d479a83e35089edbe3d32a912653/content/shell/common/shell_switches.h
,
Dec 6
,
Dec 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/babe8235c5c274ece0e7e76bdfd1bebb4e98b6cd commit babe8235c5c274ece0e7e76bdfd1bebb4e98b6cd Author: Aaron Colwell <acolwell@google.com> Date: Fri Dec 07 19:38:00 2018 Restrict CanAccessDataForOrigin() calls to the UI and IO threads. - Adding DCHECK to make sure CanAccessDataForOrigin() is only called on the UI or IO thread. This is needed for future CLs that will need to use information in the BrowserContext or ResourceContext to determine if data access is allowed. - Fix existing code that was calling CanAccessDataForOrigin() on worker threads so that it doesn't trigger the new DCHECK. Bug: 898281 Change-Id: I03ab137dad3fcec5c7c2152855816156489f91c0 Reviewed-on: https://chromium-review.googlesource.com/c/1359463 Commit-Queue: Aaron Colwell <acolwell@chromium.org> Reviewed-by: Joshua Bell <jsbell@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Cr-Commit-Position: refs/heads/master@{#614785} [modify] https://crrev.com/babe8235c5c274ece0e7e76bdfd1bebb4e98b6cd/content/browser/child_process_security_policy_impl.cc [modify] https://crrev.com/babe8235c5c274ece0e7e76bdfd1bebb4e98b6cd/content/browser/child_process_security_policy_unittest.cc [modify] https://crrev.com/babe8235c5c274ece0e7e76bdfd1bebb4e98b6cd/content/browser/fileapi/browser_file_system_helper.cc [modify] https://crrev.com/babe8235c5c274ece0e7e76bdfd1bebb4e98b6cd/content/browser/fileapi/browser_file_system_helper.h [modify] https://crrev.com/babe8235c5c274ece0e7e76bdfd1bebb4e98b6cd/content/browser/fileapi/file_system_manager_impl.cc [modify] https://crrev.com/babe8235c5c274ece0e7e76bdfd1bebb4e98b6cd/content/browser/media/android/media_resource_getter_impl.cc [modify] https://crrev.com/babe8235c5c274ece0e7e76bdfd1bebb4e98b6cd/content/browser/renderer_host/web_database_host_impl.cc [modify] https://crrev.com/babe8235c5c274ece0e7e76bdfd1bebb4e98b6cd/content/browser/renderer_host/web_database_host_impl.h [modify] https://crrev.com/babe8235c5c274ece0e7e76bdfd1bebb4e98b6cd/content/browser/renderer_host/web_database_host_impl_unittest.cc
,
Dec 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ec08cda18f9773396a600b2f974a9242df8b3be5 commit ec08cda18f9773396a600b2f974a9242df8b3be5 Author: Aaron Colwell <acolwell@google.com> Date: Fri Dec 14 00:36:35 2018 Remove missing security state workaround in CanAccessDataForOrigin(). - Remove an old workaround that was put in place before site isolation was on by default. - Fixed a test that depended on this behavior to pass. Bug: 898281, 600441 Change-Id: I1f08a0d7af59514c84f7eebd5f48027758a0f63b Reviewed-on: https://chromium-review.googlesource.com/c/1374831 Commit-Queue: Aaron Colwell <acolwell@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Cr-Commit-Position: refs/heads/master@{#616524} [modify] https://crrev.com/ec08cda18f9773396a600b2f974a9242df8b3be5/content/browser/child_process_security_policy_impl.cc [modify] https://crrev.com/ec08cda18f9773396a600b2f974a9242df8b3be5/content/browser/dom_storage/session_storage_context_mojo_unittest.cc
,
Dec 14
Unfortunately, r616524 has triggered a number of cookie/storage renderer kills in today's canary, 73.0.3640.0. So far, there are 39 GetCookies kills, 11 SetCookie kills, and 13 localStorage kills. All the individual crashes I've checked show killed_process_origin_lock as "(child id not found)" (introduced in r616524), and request_site_url is random web sites. Given this is fairly high, I'll revert, and maybe we can investigate why this could be happening next week. Sample crashes: https://crash.corp.google.com/browse?q=expanded_custom_data.ChromeCrashProto.channel%3D%27canary%27+AND+product.Version%3E%3D%2773.0.3640.0%27+AND+expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D%27%5BRenderer+kill+79%5D+content%3A%3ARenderFrameMessageFilter%3A%3AGetCookies%27&stbtiq=&reportid=79a9abed9f3a855e&index=0#2
,
Dec 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0435a1c98b06a29493ce5207d3bf144ae4def1f4 commit 0435a1c98b06a29493ce5207d3bf144ae4def1f4 Author: Alex Moshchuk <alexmos@chromium.org> Date: Fri Dec 14 22:02:59 2018 Revert "Remove missing security state workaround in CanAccessDataForOrigin()." This reverts commit ec08cda18f9773396a600b2f974a9242df8b3be5. Reason for revert: Caused renderer kills. See https://crbug.com/898281#c8. Original change's description: > Remove missing security state workaround in CanAccessDataForOrigin(). > > - Remove an old workaround that was put in place before site isolation > was on by default. > - Fixed a test that depended on this behavior to pass. > > Bug: 898281, 600441 > Change-Id: I1f08a0d7af59514c84f7eebd5f48027758a0f63b > Reviewed-on: https://chromium-review.googlesource.com/c/1374831 > Commit-Queue: Aaron Colwell <acolwell@chromium.org> > Reviewed-by: Charlie Reis <creis@chromium.org> > Reviewed-by: Alex Moshchuk <alexmos@chromium.org> > Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> > Cr-Commit-Position: refs/heads/master@{#616524} TBR=creis@chromium.org,acolwell@chromium.org,mek@chromium.org,alexmos@chromium.org Change-Id: Ic67afdbd498f1484ca728431427909a049985550 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 898281, 600441 Reviewed-on: https://chromium-review.googlesource.com/c/1379032 Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#616840} [modify] https://crrev.com/0435a1c98b06a29493ce5207d3bf144ae4def1f4/content/browser/child_process_security_policy_impl.cc [modify] https://crrev.com/0435a1c98b06a29493ce5207d3bf144ae4def1f4/content/browser/dom_storage/session_storage_context_mojo_unittest.cc
,
Dec 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/263ce44bb356617e098d2cbad147146e0a36fc64 commit 263ce44bb356617e098d2cbad147146e0a36fc64 Author: Aaron Colwell <acolwell@google.com> Date: Wed Dec 19 20:39:02 2018 Replace security state workaround in CanAccessDataForOrigin() - Replace workaround with code that is more strict about enforcing security policy during child process shutdown. The old code would always allow data access for IDs not in the security_state_ map. The new code adds a pending map so we can deal with UI/IO thread races during child process removal AND rejects any unknown IDs. - Fixed a test that depended on the old behavior where unknown IDs always allowed access. Bug: 898281, 600441 , 915203 Change-Id: I26ca1e48536672b05d2310d8a17be47d5b6ef5c7 Reviewed-on: https://chromium-review.googlesource.com/c/1382855 Commit-Queue: Aaron Colwell <acolwell@chromium.org> Reviewed-by: Tom Sepez <tsepez@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#617937} [modify] https://crrev.com/263ce44bb356617e098d2cbad147146e0a36fc64/content/browser/child_process_security_policy_impl.cc [modify] https://crrev.com/263ce44bb356617e098d2cbad147146e0a36fc64/content/browser/child_process_security_policy_impl.h [modify] https://crrev.com/263ce44bb356617e098d2cbad147146e0a36fc64/content/browser/child_process_security_policy_unittest.cc [modify] https://crrev.com/263ce44bb356617e098d2cbad147146e0a36fc64/content/browser/dom_storage/session_storage_context_mojo_unittest.cc [modify] https://crrev.com/263ce44bb356617e098d2cbad147146e0a36fc64/content/browser/dom_storage/test/mojo_test_with_file_service.h [modify] https://crrev.com/263ce44bb356617e098d2cbad147146e0a36fc64/content/browser/fileapi/browser_file_system_helper_unittest.cc [modify] https://crrev.com/263ce44bb356617e098d2cbad147146e0a36fc64/content/browser/network_service_client_unittest.cc
,
Dec 22
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b06974abd43560ae11fe5a98fa63245d0fe3d5f8 commit b06974abd43560ae11fe5a98fa63245d0fe3d5f8 Author: Nasko Oskov <nasko@chromium.org> Date: Sat Dec 22 01:04:27 2018 Revert "Replace security state workaround in CanAccessDataForOrigin()" This reverts commit 263ce44bb356617e098d2cbad147146e0a36fc64. Reason for revert: There are still renderer process terminations due to this CL, see details in https://crbug.com/915203#c9. Original change's description: > Replace security state workaround in CanAccessDataForOrigin() > > - Replace workaround with code that is more strict about enforcing > security policy during child process shutdown. The old code would > always allow data access for IDs not in the security_state_ map. The > new code adds a pending map so we can deal with UI/IO thread races > during child process removal AND rejects any unknown IDs. > > - Fixed a test that depended on the old behavior where unknown IDs > always allowed access. > > Bug: 898281, 600441 , 915203 > Change-Id: I26ca1e48536672b05d2310d8a17be47d5b6ef5c7 > Reviewed-on: https://chromium-review.googlesource.com/c/1382855 > Commit-Queue: Aaron Colwell <acolwell@chromium.org> > Reviewed-by: Tom Sepez <tsepez@chromium.org> > Reviewed-by: Alex Moshchuk <alexmos@chromium.org> > Cr-Commit-Position: refs/heads/master@{#617937} TBR=acolwell@chromium.org,tsepez@chromium.org,alexmos@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 898281, 600441 , 915203 Change-Id: I3fa9efdede12c4403ace0df0678049358009e4e4 Reviewed-on: https://chromium-review.googlesource.com/c/1389025 Reviewed-by: Nasko Oskov <nasko@chromium.org> Commit-Queue: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#618700} [modify] https://crrev.com/b06974abd43560ae11fe5a98fa63245d0fe3d5f8/content/browser/child_process_security_policy_impl.cc [modify] https://crrev.com/b06974abd43560ae11fe5a98fa63245d0fe3d5f8/content/browser/child_process_security_policy_impl.h [modify] https://crrev.com/b06974abd43560ae11fe5a98fa63245d0fe3d5f8/content/browser/child_process_security_policy_unittest.cc [modify] https://crrev.com/b06974abd43560ae11fe5a98fa63245d0fe3d5f8/content/browser/dom_storage/session_storage_context_mojo_unittest.cc [modify] https://crrev.com/b06974abd43560ae11fe5a98fa63245d0fe3d5f8/content/browser/dom_storage/test/mojo_test_with_file_service.h [modify] https://crrev.com/b06974abd43560ae11fe5a98fa63245d0fe3d5f8/content/browser/fileapi/browser_file_system_helper_unittest.cc [modify] https://crrev.com/b06974abd43560ae11fe5a98fa63245d0fe3d5f8/content/browser/network_service_client_unittest.cc
,
Jan 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/220d502c977ee3d8617f42eef7aea68e6d8c77ac commit 220d502c977ee3d8617f42eef7aea68e6d8c77ac Author: Aaron Colwell <acolwell@google.com> Date: Wed Jan 16 04:56:55 2019 Replace security state workaround in CanAccessDataForOrigin() - Replace workaround with code that is more strict about enforcing security policy during child process shutdown. The old code would always allow data access for IDs not in the security_state_ map. The new code adds a pending map so we can deal with UI/IO thread races during child process removal AND rejects any unknown IDs. - Fixed a test that depended on the old behavior where unknown IDs always allowed access. Bug: 898281, 600441 , 915203 Change-Id: I4b164eb3ec1cbb110479b633e73bcd883ef9a604 Reviewed-on: https://chromium-review.googlesource.com/c/1409732 Commit-Queue: Aaron Colwell <acolwell@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#623114} [modify] https://crrev.com/220d502c977ee3d8617f42eef7aea68e6d8c77ac/content/browser/child_process_security_policy_impl.cc [modify] https://crrev.com/220d502c977ee3d8617f42eef7aea68e6d8c77ac/content/browser/child_process_security_policy_impl.h [modify] https://crrev.com/220d502c977ee3d8617f42eef7aea68e6d8c77ac/content/browser/child_process_security_policy_unittest.cc [modify] https://crrev.com/220d502c977ee3d8617f42eef7aea68e6d8c77ac/content/browser/dom_storage/session_storage_context_mojo_unittest.cc [modify] https://crrev.com/220d502c977ee3d8617f42eef7aea68e6d8c77ac/content/browser/dom_storage/test/mojo_test_with_file_service.h [modify] https://crrev.com/220d502c977ee3d8617f42eef7aea68e6d8c77ac/content/browser/fileapi/browser_file_system_helper_unittest.cc [modify] https://crrev.com/220d502c977ee3d8617f42eef7aea68e6d8c77ac/content/browser/network_service_client_unittest.cc |
||
►
Sign in to add a comment |
||
Comment 1 by bugdroid1@chromium.org
, Oct 30