Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2686943002: New WebContents created via ctrl-click should be in a new process. (Closed)

Created:
1 year, 2 months ago by Łukasz Anforowicz
Modified:
1 year ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, site-isolation-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

New WebContents created via ctrl-click should be in a new process. This CL puts web contents created via ctrl-click (or shift-click, etc.) into a new process (fixing https://crbug.com/23815) and makes sure the new web contents are in a new browsing instance (fixing https://crbug.com/658386 and bringing consistency with the behavior of other browsers). This CL also adds a test that verifies that the new web contents created by chrome.windows.create API are in the same "browsing instance" as the caller (i.e. that the windows can find and script each other). This is a regression test for https://crbug.com/597750 that was broken by my earlier attempts to fix https://crbug.com/23815 (and AFAICT the regression wasn't caught by existing tests / tryjobs). FWIW, I've also manually tested the Hangouts extension when launching Chrome (built with this CL) with --isolate-extensions flag. BUG=23815, 597750, 658386 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2686943002 Cr-Commit-Position: refs/heads/master@{#466187} Committed: https://chromium.googlesource.com/chromium/src/+/f206da906ef1e94ee03feeb67e11a82c565baa3b

Patch Set 1 #

Patch Set 2 : . #

Total comments: 8

Patch Set 3 : Test comments. #

Total comments: 8

Patch Set 4 : Fixing affected layout tests. #

Patch Set 5 : Tweaking the tests to repond to CR feedback from alexmos@. #

Patch Set 6 : Rebasing... #

Patch Set 7 : Rebasing... #

Patch Set 8 : Really rebasing... #

Total comments: 12

Patch Set 9 : Addressed CR feedback from alexmos@. #

Patch Set 10 : git cl format #

Patch Set 11 : Passing an actual URL to window.open call made by one of new tests. #

Total comments: 10

Patch Set 12 : Made the test ctrl-click twice (to verify the behavior stays the same / that we don't reuse the new process). #

Patch Set 13 : Introduced use_new_renderer_for_new_contents field. #

Patch Set 14 : Addressed CR feedback from creis@ in chrome_navigation_browsertest.cc #

Patch Set 15 : Rebasing... #

Patch Set 16 : Fixing compilation of content/browser/security_exploit_browsertest.cc #

Patch Set 17 : No need to send the new boolean flag over IPC. #

Total comments: 10

Patch Set 18 : Addressed CR feedback from creis@. #

Total comments: 4

Patch Set 19 : Verify |window.opener| in the new ExtensionApiTest.WindowsCreateVsSiteInstance test. #

Total comments: 1

Patch Set 20 : s/use_new_renderer.../force_new_renderer.../ + comment changes. #

Total comments: 6

Patch Set 21 : s/renderer/process/ in the field name + initializing the field and variables. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+229 lines, -4 lines) Patch
M chrome/browser/chrome_navigation_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +113 lines, -0 lines 0 comments Download
M chrome/browser/extensions/api/tabs/tabs_api.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +9 lines, -0 lines 0 comments Download
M chrome/browser/extensions/api/tabs/tabs_test.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 3 chunks +43 lines, -1 line 0 comments Download
M chrome/browser/ui/browser_navigator.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/browser_navigator_params.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/ui/browser_navigator_params.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 5 chunks +6 lines, -0 lines 0 comments Download
A chrome/test/data/frame_tree/anchor_to_same_site_location.html View 1 2 3 4 1 chunk +22 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigator.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/frame_host/navigator_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/frame_host/navigator_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +3 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +13 lines, -1 line 0 comments Download
M content/browser/security_exploit_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -1 line 0 comments Download
M content/public/browser/page_navigator.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +6 lines, -0 lines 0 comments Download
M content/public/browser/page_navigator.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 4 chunks +4 lines, -0 lines 0 comments Download

Messages

Total messages: 89 (59 generated)
Łukasz Anforowicz
Alex, can you take a first, brief look please? Let's chat about this CL later ...
1 year, 2 months ago (2017-02-09 16:53:01 UTC) #10
Łukasz Anforowicz
+site-isolation-reviews@ to CC
1 year, 2 months ago (2017-02-09 16:57:54 UTC) #11
alexmos
Thanks for tackling this! I think the tests make sense, thanks for writing them! For ...
1 year, 2 months ago (2017-02-09 22:58:37 UTC) #15
alexmos
> I skimmed 743773003, and it did add some tests for this, NavigateRemoteToDataURL > and ...
1 year, 2 months ago (2017-02-09 23:55:26 UTC) #16
Łukasz Anforowicz
On 2017/02/09 22:58:37, alexmos wrote: > Thanks for tackling this! I think the tests make ...
1 year, 2 months ago (2017-02-10 00:38:18 UTC) #17
Łukasz Anforowicz
https://codereview.chromium.org/2686943002/diff/20001/chrome/test/data/frame_tree/anchor_to_same_site_location.html File chrome/test/data/frame_tree/anchor_to_same_site_location.html (right): https://codereview.chromium.org/2686943002/diff/20001/chrome/test/data/frame_tree/anchor_to_same_site_location.html#newcode19 chrome/test/data/frame_tree/anchor_to_same_site_location.html:19: id="test-anchor-no-target">Test link to click while holding ctrl key</a> On ...
1 year, 2 months ago (2017-02-10 01:13:50 UTC) #18
Łukasz Anforowicz
Alex, can you take another look please? I think that back in February we agreed ...
1 year ago (2017-04-10 21:23:15 UTC) #32
Łukasz Anforowicz
+creis@ to reviewers There were questions what happens when RequestOpenURL is used in cases other ...
1 year ago (2017-04-11 20:34:48 UTC) #34
alexmos
A couple more thoughts below. Overall I think we can give this a try, but ...
1 year ago (2017-04-11 20:42:40 UTC) #35
Łukasz Anforowicz
Thanks for the review. I've tried to gather replies to your and my comments below. ...
1 year ago (2017-04-11 23:10:37 UTC) #37
alexmos
Thanks! LGTM, but let's wait for Charlie to also review. And thanks for all the ...
1 year ago (2017-04-12 22:08:29 UTC) #38
Łukasz Anforowicz
Thanks Alex. Charlie - can you PTAL? (Friday is fine :-). https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/chrome_navigation_browsertest.cc File chrome/browser/chrome_navigation_browsertest.cc (right): ...
1 year ago (2017-04-12 22:40:04 UTC) #40
Charlie Reis
On 2017/04/12 22:40:04, Łukasz A. wrote: > Thanks Alex. Charlie - can you PTAL? (Friday ...
1 year ago (2017-04-12 22:53:13 UTC) #42
Charlie Reis
Let's explore the idea we chatted about, where we add a boolean to say whether ...
1 year ago (2017-04-14 22:28:32 UTC) #45
Łukasz Anforowicz
Thanks for the feedback - can you take another look please? On 2017/04/14 22:28:32, Charlie ...
1 year ago (2017-04-18 16:50:22 UTC) #58
alexmos
> I like the new boolean field, but I wanted to highlight that > source_site_instance ...
1 year ago (2017-04-18 17:26:27 UTC) #59
Łukasz Anforowicz
On 2017/04/18 17:26:27, alexmos wrote: > > I like the new boolean field, but I ...
1 year ago (2017-04-18 17:48:51 UTC) #60
Łukasz Anforowicz
FYI - I've uploaded a new patchset where I've added explicit initializer for the new ...
1 year ago (2017-04-18 21:09:16 UTC) #61
Charlie Reis
Thanks! This LGTM, with one important question in CreateTargetContents. On 2017/04/18 17:48:51, Łukasz A. wrote: ...
1 year ago (2017-04-19 20:11:11 UTC) #62
Łukasz Anforowicz
Thanks Charlie. https://codereview.chromium.org/2686943002/diff/320001/chrome/browser/chrome_navigation_browsertest.cc File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2686943002/diff/320001/chrome/browser/chrome_navigation_browsertest.cc#newcode200 chrome/browser/chrome_navigation_browsertest.cc:200: // browsing instance from |main|contents. Returns contents ...
1 year ago (2017-04-20 01:11:29 UTC) #65
Łukasz Anforowicz
+rdevlin.cronin@ for chrome/browser/extensions/api/tabs/tabs* +sky@ for chrome/browser/ui/browser_navigator* Can you PTAL?
1 year ago (2017-04-20 02:17:14 UTC) #69
Devlin
What about tabs created by chrome.tabs.create()? Tabs updated via chrome.tabs.update()? https://codereview.chromium.org/2686943002/diff/340001/chrome/browser/extensions/api/tabs/tabs_api.cc File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2686943002/diff/340001/chrome/browser/extensions/api/tabs/tabs_api.cc#newcode627 ...
1 year ago (2017-04-20 16:22:30 UTC) #70
Łukasz Anforowicz
Thanks Devlin - can you take another look please? Charlie - note that I've changed ...
1 year ago (2017-04-20 19:03:57 UTC) #73
sky
https://codereview.chromium.org/2686943002/diff/380001/chrome/browser/ui/browser_navigator_params.h File chrome/browser/ui/browser_navigator_params.h (right): https://codereview.chromium.org/2686943002/diff/380001/chrome/browser/ui/browser_navigator_params.h#newcode139 chrome/browser/ui/browser_navigator_params.h:139: bool force_new_renderer_for_new_contents; I find the use of renderer mildly ...
1 year ago (2017-04-20 21:00:58 UTC) #76
Devlin
extensions lgtm. Can you file a bug where we can track the inconsistency of opener ...
1 year ago (2017-04-20 21:06:10 UTC) #77
Łukasz Anforowicz
Thanks for the feedback (and especially for catching the lack of initialization... :-/ ). Devlin: ...
1 year ago (2017-04-20 21:56:05 UTC) #78
sky
LGTM
1 year ago (2017-04-20 22:24:55 UTC) #81
Łukasz Anforowicz
Thanks for the reviews everyone - I'll push to CQ in a few minutes. On ...
1 year ago (2017-04-20 22:45:28 UTC) #82
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2686943002/400001
1 year ago (2017-04-20 22:46:24 UTC) #86
commit-bot: I haz the power
1 year ago (2017-04-20 23:27:05 UTC) #89
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/f206da906ef1e94ee03feeb67e11...

Powered by Google App Engine
This is Rietveld 408576698