Isolate error pages in their own process |
||||||||||
Issue descriptionIssue 448486 is aiming at refactoring interstitial pages to be regular committed navigations. This requires that interstitial pages are isolated from regular web content to avoid compromised renderer process being able to affect the interstitial page. A design doc for this work is https://docs.google.com/document/d/17IOOppwf8re2dpz4TflH_7wFkJr6Or-ltXGsTeMOhM0/edit.
,
May 1 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/25efcca94a032c04dab58f3b2aabcb5fc3356602 commit 25efcca94a032c04dab58f3b2aabcb5fc3356602 Author: Nasko Oskov <nasko@chromium.org> Date: Tue May 01 19:33:42 2018 Ensure MemoryInstrumentationTest::Navigate works correctly. The navigation performed in the MemoryInstrumentationTest::Navigate method is using a non-existing file, which means the navigation does not succeed. This CL fixes the code to use a file that exists and to check the navigation is actually successful. Bug: 838161 Change-Id: Icf0715f687a1cdf763b47e3bd3155321e761af96 Reviewed-on: https://chromium-review.googlesource.com/1037637 Reviewed-by: Siddhartha S <ssid@chromium.org> Commit-Queue: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#555146} [modify] https://crrev.com/25efcca94a032c04dab58f3b2aabcb5fc3356602/content/browser/tracing/memory_instrumentation_browsertest.cc
,
May 2 2018
,
May 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5319d9b60c08fee2ca45593c7a67688408ce5d73 commit 5319d9b60c08fee2ca45593c7a67688408ce5d73 Author: Nasko Oskov <nasko@chromium.org> Date: Thu May 03 01:54:54 2018 Fix testCrossDomainNavigationDoNotLoseImportance to navigate to existing file. Currently this test navigates to a file that is not copied over to the device, which causes it to fail on the bots inconsistently. This CL makes sure the file is present in the location copied to the device and updates the path in the test. Bug: 838161 Change-Id: Icc8f7aa4537ba89575cda19fe1fd00b84e375323 Reviewed-on: https://chromium-review.googlesource.com/1040873 Reviewed-by: Bo <boliu@chromium.org> Commit-Queue: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#555651} [modify] https://crrev.com/5319d9b60c08fee2ca45593c7a67688408ce5d73/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherIntegrationTest.java [add] https://crrev.com/5319d9b60c08fee2ca45593c7a67688408ce5d73/content/test/data/android/title1.html
,
May 3 2018
,
May 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d83b571dc700010f144b68fe3d8628923c008773 commit d83b571dc700010f144b68fe3d8628923c008773 Author: Nasko Oskov <nasko@chromium.org> Date: Fri May 04 04:50:57 2018 Implement process isolation for chrome-error:// documents. Chrome error pages currently commit in either the source or destination RenderFrameHost and are not isolated in any way. The "Committed Interstitials" project will change interstitials to work just like error pages. Since interstitial pages use renderer-initiated messaging to proceed through, the security model of them being rendered in a separate process needs to be preserved. This CL is implementing a process isolation model for error pages as described in the following design doc: https://docs.google.com/document/d/17IOOppwf8re2dpz4TflH_7wFkJr6Or-ltXGsTeMOhM0/edit Bug: 448486, 838161 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Change-Id: I58cdc8f823ffa10a7a7ee219e2ade3d69b538882 Reviewed-on: https://chromium-review.googlesource.com/762520 Commit-Queue: Nasko Oskov <nasko@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#555988} [modify] https://crrev.com/d83b571dc700010f144b68fe3d8628923c008773/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/d83b571dc700010f144b68fe3d8628923c008773/chrome/browser/chrome_content_browser_client.h [modify] https://crrev.com/d83b571dc700010f144b68fe3d8628923c008773/chrome/browser/chrome_security_exploit_browsertest.cc [modify] https://crrev.com/d83b571dc700010f144b68fe3d8628923c008773/content/browser/frame_host/navigation_handle_impl.cc [modify] https://crrev.com/d83b571dc700010f144b68fe3d8628923c008773/content/browser/frame_host/navigation_handle_impl_browsertest.cc [modify] https://crrev.com/d83b571dc700010f144b68fe3d8628923c008773/content/browser/frame_host/navigation_request.cc [modify] https://crrev.com/d83b571dc700010f144b68fe3d8628923c008773/content/browser/frame_host/navigator_impl.cc [modify] https://crrev.com/d83b571dc700010f144b68fe3d8628923c008773/content/browser/frame_host/render_frame_host_manager.cc [modify] https://crrev.com/d83b571dc700010f144b68fe3d8628923c008773/content/browser/frame_host/render_frame_host_manager.h [modify] https://crrev.com/d83b571dc700010f144b68fe3d8628923c008773/content/browser/frame_host/render_frame_host_manager_browsertest.cc [modify] https://crrev.com/d83b571dc700010f144b68fe3d8628923c008773/content/browser/isolated_origin_browsertest.cc [modify] https://crrev.com/d83b571dc700010f144b68fe3d8628923c008773/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/d83b571dc700010f144b68fe3d8628923c008773/content/browser/site_instance_impl.cc [modify] https://crrev.com/d83b571dc700010f144b68fe3d8628923c008773/content/common/url_schemes.cc [modify] https://crrev.com/d83b571dc700010f144b68fe3d8628923c008773/content/public/browser/content_browser_client.cc [modify] https://crrev.com/d83b571dc700010f144b68fe3d8628923c008773/content/public/browser/content_browser_client.h [modify] https://crrev.com/d83b571dc700010f144b68fe3d8628923c008773/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process [delete] https://crrev.com/fa1b1a37829c23c75b605c0654655d82792ff0fe/third_party/WebKit/LayoutTests/fast/loader/recursive-before-unload-crash.html
,
May 4 2018
nasko: Was that commit the last one for this? (AKA: Does it SGTY if we start enabling committed ssl interstitials at this point?)
,
May 8 2018
That is the main piece of work that was needed. It does sgtm to start poking at committed interstitials, but given we found one bug, I wouldn't rule out if there are more. So let's start but be ready to react if there are more issues discovered.
,
May 9 2018
Unfortunately, the CL in comment #6 (r555988) has caused a functional regression ( issue 840485 ) and renderer kills (issues 840794 and 840701). I will disable the isolation in https://chromium-review.googlesource.com/c/chromium/src/+/1050431 temporarily until I can investigate and fix those. In the meantime, it can be easily enabled locally for testing by undoing the changes in content/public/browser/content_browser_client.cc and chrome/browser/chrome_content_browser_client.cc from that CL. The rest was changes needed to ensure code and tests can operate in both modes.
,
May 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d515cab0d9f6913d8a139dc0484a451f6d23524a commit d515cab0d9f6913d8a139dc0484a451f6d23524a Author: Nasko Oskov <nasko@chromium.org> Date: Wed May 09 15:34:20 2018 Disable error page isolation temporarily. The isolation of error pages has caused at least one functional regression described https://crbug.com/840485 and causes various renderer process terminations. This CL disables the isolation functionality so these can be investigated and fixed before re-enabling. Bug: 838161 , 840485 , 840794, 840701 Change-Id: Id25937f6f400578c7c8bf315551b3878b4a4937c Reviewed-on: https://chromium-review.googlesource.com/1050431 Commit-Queue: Nasko Oskov <nasko@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#557191} [modify] https://crrev.com/d515cab0d9f6913d8a139dc0484a451f6d23524a/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/d515cab0d9f6913d8a139dc0484a451f6d23524a/chrome/browser/chrome_security_exploit_browsertest.cc [modify] https://crrev.com/d515cab0d9f6913d8a139dc0484a451f6d23524a/content/browser/frame_host/navigation_handle_impl_browsertest.cc [modify] https://crrev.com/d515cab0d9f6913d8a139dc0484a451f6d23524a/content/browser/frame_host/navigation_request.cc [modify] https://crrev.com/d515cab0d9f6913d8a139dc0484a451f6d23524a/content/browser/frame_host/render_frame_host_manager.cc [modify] https://crrev.com/d515cab0d9f6913d8a139dc0484a451f6d23524a/content/browser/frame_host/render_frame_host_manager_browsertest.cc [modify] https://crrev.com/d515cab0d9f6913d8a139dc0484a451f6d23524a/content/public/browser/content_browser_client.cc [modify] https://crrev.com/d515cab0d9f6913d8a139dc0484a451f6d23524a/content/public/browser/site_isolation_policy.cc [modify] https://crrev.com/d515cab0d9f6913d8a139dc0484a451f6d23524a/content/public/browser/site_isolation_policy.h
,
May 11 2018
,
May 30 2018
,
Jun 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/87ed572363ae020dccf0ef13f5d7383ea5548309 commit 87ed572363ae020dccf0ef13f5d7383ea5548309 Author: Nasko Oskov <nasko@chromium.org> Date: Tue Jun 26 00:06:38 2018 Do not change BrowsingInstance on error page commits. When error page isolation is enabled, error pages commit in their own SiteInstance. Reloading an error page should stay in the same SiteInstance it was initially placed in. However, if a navigation is to an URL that requires a BrowsingInstance swap, forcing the swap causes the error page to commit in a SiteInstance for the destination URL. This is not expected nor is it the desired behavior. This CL adds a check for error pages in ShouldSwapBrowsingInstancesForNavigation to keep the commits in the same BrowsingInstance. When a successful navigation/reload happens for the destination URL, the BrowsingInstance will be correctly swapped. Bug: 838161 Change-Id: Iaf2e708426b75881dda7065069b618f180fef7d2 Reviewed-on: https://chromium-review.googlesource.com/1113905 Commit-Queue: Nasko Oskov <nasko@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#570251} [modify] https://crrev.com/87ed572363ae020dccf0ef13f5d7383ea5548309/content/browser/frame_host/render_frame_host_manager.cc [modify] https://crrev.com/87ed572363ae020dccf0ef13f5d7383ea5548309/content/browser/frame_host/render_frame_host_manager.h [modify] https://crrev.com/87ed572363ae020dccf0ef13f5d7383ea5548309/content/browser/frame_host/render_frame_host_manager_browsertest.cc [modify] https://crrev.com/87ed572363ae020dccf0ef13f5d7383ea5548309/content/browser/frame_host/render_frame_host_manager_unittest.cc
,
Jun 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2e14c65d943b6fa1764af0543454eab78fb55b57 commit 2e14c65d943b6fa1764af0543454eab78fb55b57 Author: Nasko Oskov <nasko@chromium.org> Date: Wed Jun 27 18:43:20 2018 Update DebuggerApiTest to navigate to real file. The DebuggerNotAllowedOnOtherExtensionPages test navigates to non-existing URL and threfore results in an error. This CL fixes it to succeed and unblock isolating error pages. Bug: 838161 Change-Id: I38a8bb656fab0992d9343ba39a2c5f3da497c7fb Reviewed-on: https://chromium-review.googlesource.com/1117262 Commit-Queue: Nasko Oskov <nasko@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/heads/master@{#570851} [modify] https://crrev.com/2e14c65d943b6fa1764af0543454eab78fb55b57/chrome/browser/extensions/api/debugger/debugger_apitest.cc
,
Jun 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/416c439d63601dac492fa6de9eb2fd649aefc14b commit 416c439d63601dac492fa6de9eb2fd649aefc14b Author: Nasko Oskov <nasko@chromium.org> Date: Wed Jun 27 22:53:53 2018 Don't give error page process WebUI bindings. When error page isolation is enabled, the committing RenderFrameHost does not need WebUI object and the process does not need to have WebUI bindings. This CL changes RenderFrameHost to avoid creating WebUI state when it is for the error page SiteInstance. Bug: 838161 Change-Id: Id1989b2d6c8ec7d25b5604eda50e9a61fa0c7638 Reviewed-on: https://chromium-review.googlesource.com/1117362 Commit-Queue: Nasko Oskov <nasko@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#570920} [modify] https://crrev.com/416c439d63601dac492fa6de9eb2fd649aefc14b/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/416c439d63601dac492fa6de9eb2fd649aefc14b/content/browser/frame_host/render_frame_host_manager_browsertest.cc
,
Jun 28 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/55d21e1555e7de5dd34e8ca115f27b3267870fee commit 55d21e1555e7de5dd34e8ca115f27b3267870fee Author: Nasko Oskov <nasko@chromium.org> Date: Wed Jun 27 23:59:38 2018 Enable error page isolation. Error page isolation was introduced in https://crrev.com/c/762520 but it had some function bugs (e.g. https://crbug.com/840485 ). There have been multiple fixes since then and it should be in a shape to try and enable it again. Bug: 838161 Change-Id: Ic3815e26eca9850c14e2cd925ed037223f4ded11 Reviewed-on: https://chromium-review.googlesource.com/1115203 Commit-Queue: Nasko Oskov <nasko@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#570951} [modify] https://crrev.com/55d21e1555e7de5dd34e8ca115f27b3267870fee/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/55d21e1555e7de5dd34e8ca115f27b3267870fee/content/public/browser/content_browser_client.cc
,
Jun 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/65b3a9fb448c5b9a10f0ae06aceadb8746855366 commit 65b3a9fb448c5b9a10f0ae06aceadb8746855366 Author: Nasko Oskov <nasko@chromium.org> Date: Fri Jun 29 19:14:36 2018 Add checks for error page process being locked to origin. Error page process is locked to origin in practice and it should be more thoroughly enforced by existing tests. This CL adds such additional checks. Bug: 838161 Change-Id: If38845a67f9d1905d37b54e7dc5d53e90bbaabbe Reviewed-on: https://chromium-review.googlesource.com/1120837 Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#571582} [modify] https://crrev.com/65b3a9fb448c5b9a10f0ae06aceadb8746855366/content/browser/frame_host/render_frame_host_manager_browsertest.cc
,
Jun 29 2018
With this last CL, I think this can be considered fixed! Closing as such, but please do file new bugs if you find anything going wrong.
,
Jul 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8557acd3db2d25c96134ffcae4c9a298a1d0ffe1 commit 8557acd3db2d25c96134ffcae4c9a298a1d0ffe1 Author: Nasko Oskov <nasko@chromium.org> Date: Mon Jul 02 17:35:35 2018 Add a test for navigation away from error page. This CL adds a test case to verify that navigating away from an error page properly swaps SiteInstance when error page isolation is enabled. Bug: 838161 Change-Id: Ibe16bf39b24dd1d18666c4383bfba5777f8c965e Reviewed-on: https://chromium-review.googlesource.com/1122840 Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#571935} [modify] https://crrev.com/8557acd3db2d25c96134ffcae4c9a298a1d0ffe1/content/browser/frame_host/render_frame_host_manager_browsertest.cc
,
Aug 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8683381bad343b328c489414affd74e5466b4ae2 commit 8683381bad343b328c489414affd74e5466b4ae2 Author: Nasko Oskov <nasko@chromium.org> Date: Tue Aug 21 23:38:54 2018 Revert "Enable error page isolation." This reverts commit 55d21e1555e7de5dd34e8ca115f27b3267870fee. Bug: 838161 , 865914 Change-Id: Ib96a2336f312e0c4bcfcc658d7baa9e5b0b6c608 Reviewed-on: https://chromium-review.googlesource.com/1184272 Reviewed-by: Alex Mineer <amineer@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#757} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/8683381bad343b328c489414affd74e5466b4ae2/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/8683381bad343b328c489414affd74e5466b4ae2/content/public/browser/content_browser_client.cc
,
Aug 21
Here's a summary of the rules that were executed: - OnlyMergeApprovedChange: Rule Failed -- Revision 8683381bad343b328c489414affd74e5466b4ae2 was merged to refs/branch-heads/3497 branch with no merge approval from a TPM! Please explain why this change was merged to the branch!
,
Oct 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/10738858296b52d3eccc06f64cbd32c3b1a6331a commit 10738858296b52d3eccc06f64cbd32c3b1a6331a Author: Nasko Oskov <nasko@chromium.org> Date: Wed Oct 31 20:40:39 2018 Disable error page isolation in Android WebView. Error page isolation requires process swaps and locking processes to origin. It isn't clear Android WebView supports these quite yet, so disable this feature for now. Bug: 800544, 838161 Change-Id: Idb5c4a22aee3c6d40560a7676a7659d9101ee2e8 Reviewed-on: https://chromium-review.googlesource.com/c/1310566 Reviewed-by: Bo <boliu@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#604371} [modify] https://crrev.com/10738858296b52d3eccc06f64cbd32c3b1a6331a/android_webview/browser/aw_content_browser_client.cc [modify] https://crrev.com/10738858296b52d3eccc06f64cbd32c3b1a6331a/android_webview/browser/aw_content_browser_client.h
,
Nov 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5fed3e6ab8ae6dc7da1628e15139aa3bce50b1d0 commit 5fed3e6ab8ae6dc7da1628e15139aa3bce50b1d0 Author: Nasko Oskov <nasko@chromium.org> Date: Thu Nov 15 18:54:17 2018 [M71 merge] Disable error page isolation in Android WebView. Error page isolation requires process swaps and locking processes to origin. It isn't clear Android WebView supports these quite yet, so disable this feature for now. Bug: 800544, 838161 Change-Id: Idb5c4a22aee3c6d40560a7676a7659d9101ee2e8 Reviewed-on: https://chromium-review.googlesource.com/c/1310566 Reviewed-by: Bo <boliu@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Nasko Oskov <nasko@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#604371}(cherry picked from commit 10738858296b52d3eccc06f64cbd32c3b1a6331a) Reviewed-on: https://chromium-review.googlesource.com/c/1337283 Reviewed-by: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#698} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} [modify] https://crrev.com/5fed3e6ab8ae6dc7da1628e15139aa3bce50b1d0/android_webview/browser/aw_content_browser_client.cc [modify] https://crrev.com/5fed3e6ab8ae6dc7da1628e15139aa3bce50b1d0/android_webview/browser/aw_content_browser_client.h
,
Nov 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5fed3e6ab8ae6dc7da1628e15139aa3bce50b1d0 Commit: 5fed3e6ab8ae6dc7da1628e15139aa3bce50b1d0 Author: nasko@chromium.org Commiter: nasko@chromium.org Date: 2018-11-15 18:54:17 +0000 UTC [M71 merge] Disable error page isolation in Android WebView. Error page isolation requires process swaps and locking processes to origin. It isn't clear Android WebView supports these quite yet, so disable this feature for now. Bug: 800544, 838161 Change-Id: Idb5c4a22aee3c6d40560a7676a7659d9101ee2e8 Reviewed-on: https://chromium-review.googlesource.com/c/1310566 Reviewed-by: Bo <boliu@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Nasko Oskov <nasko@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#604371}(cherry picked from commit 10738858296b52d3eccc06f64cbd32c3b1a6331a) Reviewed-on: https://chromium-review.googlesource.com/c/1337283 Reviewed-by: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#698} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by bugdroid1@chromium.org
, Apr 30 2018