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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 months, 2 weeks ago by Łukasz Anforowicz
Modified:
6 months 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
Commit queue not available (can’t edit this change).

Messages

Total messages: 89 (59 generated)
Łukasz Anforowicz
Alex, can you take a first, brief look please? Let's chat about this CL later ...
8 months, 2 weeks ago (2017-02-09 16:53:01 UTC) #10
Łukasz Anforowicz
+site-isolation-reviews@ to CC
8 months, 2 weeks 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 ...
8 months, 2 weeks ago (2017-02-09 22:58:37 UTC) #15
alexmos
> I skimmed 743773003, and it did add some tests for this, NavigateRemoteToDataURL > and ...
8 months, 2 weeks 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 ...
8 months, 2 weeks 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 ...
8 months, 2 weeks 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 ...
6 months, 2 weeks 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 ...
6 months, 2 weeks 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 ...
6 months, 2 weeks 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. ...
6 months, 2 weeks 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 ...
6 months, 1 week 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): ...
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week 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: ...
6 months 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 ...
6 months 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?
6 months 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 ...
6 months 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 ...
6 months 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 ...
6 months 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 ...
6 months ago (2017-04-20 21:06:10 UTC) #77
Łukasz Anforowicz
Thanks for the feedback (and especially for catching the lack of initialization... :-/ ). Devlin: ...
6 months ago (2017-04-20 21:56:05 UTC) #78
sky
LGTM
6 months 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 ...
6 months 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
6 months ago (2017-04-20 22:46:24 UTC) #86
commit-bot: I haz the power
6 months 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa