Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2686943002:
New WebContents created via ctrl-click should be in a new process. (Closed)
|
Created:
3 years, 10 months ago by Łukasz Anforowicz Modified:
3 years, 8 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. |
DescriptionNew 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. #Messages
Total messages: 89 (59 generated)
Description was changed from ========== Test for having chrome.windows.create use caller's site instance. This CL 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). BUG=597750 ========== to ========== Test for having chrome.windows.create use caller's site instance. This CL 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). BUG=597750 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Test for having chrome.windows.create use caller's site instance. This CL 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). BUG=597750 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== New WebContents created via ctrl-click should be in a new process. This CL puts web contents created via ctrl-click 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). BUG=23815, 597750, 658386 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by lukasza@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...
Description was changed from ========== New WebContents created via ctrl-click should be in a new process. This CL puts web contents created via ctrl-click 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). BUG=23815, 597750, 658386 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== New WebContents created via ctrl-click should be in a new process. This CL puts web contents created via ctrl-click 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 ==========
Description was changed from ========== New WebContents created via ctrl-click should be in a new process. This CL puts web contents created via ctrl-click 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 ========== to ========== New WebContents created via ctrl-click should be in a new process. This CL puts web contents created via ctrl-click 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
lukasza@chromium.org changed reviewers: + alexmos@chromium.org
Alex, can you take a first, brief look please? Let's chat about this CL later today. In particular: - Do the tests make sense? (interestingly in the hangouts regression test the only thing that fails is the expectation of site-instance equality - everything else would pass even after my other, wrong fix [i.e. processes would be the same and window.open would still work, but hangouts would hang and would open an extra empty tab [maybe via window.open that cannot find the old window at a *later* point]) - Does the fix make sense? - What can I be possibly breaking with the fix? The bots are green FWIW :-). Do you think there are some other tests that I should automate or try manually? I am aware of failures in the red (not purple) linux_site_isolation results. The following 2 failures are expected: http/tests/security/contentSecurityPolicy/frame-src-vs-shift-click.html http/tests/navigation/post-with-modifier.html 2 layout tests above depended (I did it... :-/) on window.open working after ctrl-click - I need to preland a separate CL that rewrites the tests on top of dump-custom-text [which I need to replicate across OOPIFs and secondary test windows]). I have not yet investigated the failure in http/tests/media/video-play-stall.html https://codereview.chromium.org/2686943002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2686943002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_test.cc:2167: } 1. I am not sure if this is the right location for the new test. 2. I see that other tests around here instantiate and directly invoke WindowsCreateFunction. I hope that invoking via javascript is just as good (FWIW it seems simpler to me - for example it wasn't immediately clear to me how to ensure the function runs against/inside a particular frame). https://codereview.chromium.org/2686943002/diff/20001/chrome/test/data/frame_... File chrome/test/data/frame_tree/anchor_to_same_site_location.html (right): https://codereview.chromium.org/2686943002/diff/20001/chrome/test/data/frame_... 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> Charlie was pointing out that another interesting scenario here is target="_blank". Currently this scenario still leads to the old process being reused. https://codereview.chromium.org/2686943002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (left): https://codereview.chromium.org/2686943002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:778: params.source_site_instance = current_site_instance; My initial fix was to never use params.source_site_instance in CreateTargetContents in chrome/browser/ui/browser_navigator.cc, but this went too far and broke hangouts when --isolate-extensions flag is present.
+site-isolation-reviews@ to CC
Description was changed from ========== New WebContents created via ctrl-click should be in a new process. This CL puts web contents created via ctrl-click 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 ========== to ========== New WebContents created via ctrl-click should be in a new process. This CL puts web contents created via ctrl-click 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. This CL also fixes/refactors two layout tests that used to depend on ability to find the opener via window.open, even in case of ctrl-click or shift-click. The tests had to be refactored so that they don't depend on ability to comminicate back with the main test window. After this CL the tests report their results via testRunner.setCustomTextOutput (rather than sending the results back to the main window using window.postMessage). BUG=23815, 597750, 658386 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by lukasza@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...
Thanks for tackling this! I think the tests make sense, thanks for writing them! For the fix, I dug around a little and found that the source_site_instance plumbing you're removing was added in https://codereview.chromium.org/743773003 to fix https://crbug.com/433014. So I'd try anything you can think of with data: URLs, which should stay in the frame that initiated the navigation. Specifically, have a subframe with data URL, navigate it cross-site, go back, and check that the subframe isn't kicked out into a separate process. Other things to try would be session restore with data URLs, a frame navigating sibling frame to data URL, etc. about:blank is probably worth testing too. Both of those receive some special treatment that uses the source SiteInstance in CanSubframeSwapProcess, https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_.... I skimmed 743773003, and it did add some tests for this, NavigateRemoteToDataURL and avigateRemoteToBlankURL, but unfortunately they are disabled at the moment :( (due to a bug that's now fixed, so we should try re-enabling them). Other than those, perhaps try chrome.windows.create with multiple URLs; I've run into trouble there in the past, see https://codereview.chromium.org/2372323003/diff/140001/chrome/browser/ui/brow... in the context of https://crbug.com/590068 https://codereview.chromium.org/2686943002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2686943002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_test.cc:2167: } On 2017/02/09 16:53:01, Łukasz Anforowicz wrote: > 1. I am not sure if this is the right location for the new test. > > 2. I see that other tests around here instantiate and directly invoke > WindowsCreateFunction. I hope that invoking via javascript is just as good > (FWIW it seems simpler to me - for example it wasn't immediately clear to me how > to ensure the function runs against/inside a particular frame). This seems like a reasonable place for it. Another option could be window_open_apitest.cc, which has other tests that create popups, including some that use chrome.windows.create in JS. I'm not sure which one would be preferred -- might be good to double-check with Devlin. https://codereview.chromium.org/2686943002/diff/20001/chrome/test/data/frame_... File chrome/test/data/frame_tree/anchor_to_same_site_location.html (right): https://codereview.chromium.org/2686943002/diff/20001/chrome/test/data/frame_... 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 2017/02/09 16:53:01, Łukasz Anforowicz wrote: > Charlie was pointing out that another interesting scenario here is > target="_blank". Currently this scenario still leads to the old process being > reused. Interesting, so ctrl-click on <a target="_blank"> would reuse the process? Do you know why? What if the target is something else, like an existing (or non-existing) name? What's the behavior of ctrl-click there? https://codereview.chromium.org/2686943002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (left): https://codereview.chromium.org/2686943002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:778: params.source_site_instance = current_site_instance; On 2017/02/09 16:53:01, Łukasz Anforowicz wrote: > My initial fix was to never use params.source_site_instance in > CreateTargetContents in chrome/browser/ui/browser_navigator.cc, but this went > too far and broke hangouts when --isolate-extensions flag is present. Acknowledged. https://codereview.chromium.org/2686943002/diff/40001/chrome/browser/chrome_n... File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2686943002/diff/40001/chrome/browser/chrome_n... chrome/browser/chrome_navigation_browsertest.cc:235: new_contents->GetMainFrame()->GetProcess()); Might be good to compare their SiteInstances as well. https://codereview.chromium.org/2686943002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2686943002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_test.cc:2140: // Window created by chrome.windows.create should be in the same site instance nit: s/site instance/SiteInstance/ https://codereview.chromium.org/2686943002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_test.cc:2169: EXPECT_EQ(extension_url.spec(), url_of_found_window); I'm a bit confused here: is this testing that old_contents can find itself by name? Did you mean to look up the new_contents by name instead? But then the two windows have the same URL, so if window.open returned the wrong window, the URL comparison wouldn't catch it. https://codereview.chromium.org/2686943002/diff/40001/chrome/test/data/frame_... File chrome/test/data/frame_tree/anchor_to_same_site_location.html (right): https://codereview.chromium.org/2686943002/diff/40001/chrome/test/data/frame_... chrome/test/data/frame_tree/anchor_to_same_site_location.html:16: that points to the same site. See also https://crbug.com/238156. Typo in bug number?
> I skimmed 743773003, and it did add some tests for this, NavigateRemoteToDataURL > and avigateRemoteToBlankURL, but unfortunately they are disabled at the moment > :( (due to a bug that's now fixed, so we should try re-enabling them). Sorry, I was sloppy with the code archealogy here -- these tests were committed disabled, but are actually enabled right now, apparently moved and merged in NavigateRemoteFrameToBlankAndDataURLs. The session history case is also covered in NavigateSubframeToDataUrlInSessionHistory. So I guess these cases do work with your fix :) - and that makes sense, as the proxy navigations will go through the transfer path, which still has the source_site_instance plumbing. lfg@'s CL might have plumbed the source_site_instance through RequestOpenURL because at the time that was implemented using RequestTransferURL. That's no longer the case, so yes, I see how source_site_instance might not actually be necessary in RequestOpenURL anymore.
On 2017/02/09 22:58:37, alexmos wrote: > Thanks for tackling this! I think the tests make sense, thanks for writing > them! Ok. I especially was unsure about the new ExtensionApiTest.WindowCreateVsSiteInstance - it seemed weird to care about SiteInstance, but not about browsing instance (i.e. whether window.open works) and not about process allocation. I guess, I don't understand the significance of SiteInstance difference in this case (or its relation to browsing instance). > For the fix, I dug around a little and found that the source_site_instance > plumbing you're removing was added in https://codereview.chromium.org/743773003 > to fix https://crbug.com/433014. So I'd try anything you can think of with > data: URLs, which should stay in the frame that initiated the navigation. > Specifically, have a subframe with data URL, navigate it cross-site, go back, > and check that the subframe isn't kicked out into a separate process. > > Other things to try would be session restore with data URLs, a frame navigating > sibling frame to data URL, etc. about:blank is probably worth testing too. > Both of those receive some special treatment that uses the source SiteInstance > in CanSubframeSwapProcess, > https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_.... Thanks for pointing out RenderFrameHostManager::CanSubframeSwapProcess. I think the main thing we need to worry about here is that without a site instance we might *avoid* a necessary process swap. Let me try to enumarate all the conditions necessary to do the wrong thing here: 1. Navigation has to go through OpenURL (which used to set source_site_instance before this CL, but doesn't anymore after this CL). 2. Navigation has to be to data: or about:blank 3. Navigation has to be initiated from frameA, target different frameB, and frameA has to be cross-site from frameB (so that RenderFrameHostManager::CanSubframeSwapProcess will say that a process swap is okay). Can we chat how the conditions above might be trigerred? I am especially blurry on how to trigger #1 (this would be hit for web<->extension navigations, but it seems at odds with #2 and #3, right?). (OTOH, I am also open to the idea of just landing the CL if the bots are green :-). > I skimmed 743773003, and it did add some tests for this, NavigateRemoteToDataURL > and avigateRemoteToBlankURL, but unfortunately they are disabled at the moment > :( (due to a bug that's now fixed, so we should try re-enabling them). Good news - it turns out that these tests got enabled in r319567 (and apparently are still passing even with this CL - yay!). > Other than those, perhaps try chrome.windows.create with multiple URLs; I've run > into trouble there in the past, see > https://codereview.chromium.org/2372323003/diff/140001/chrome/browser/ui/brow... > in the context of https://crbug.com/590068 I don't worry about chrome.windows.create - it goes through another code path, which still sets source_site_instance here: https://chromium.googlesource.com/chromium/src/+/ed629aecdde213fd8b49296582fa... chrome/browser/extensions/api/tabs/tabs_api.cc:612: navigate_params.source_site_instance = render_frame_host()->GetSiteInstance(); PS. Please note that I've also sneakily uploaded a patched where I've fixed/refactored the affected layout tests (which depended on the undesired behavior that we are fixing in this CL). https://codereview.chromium.org/2686943002/diff/20001/chrome/test/data/frame_... File chrome/test/data/frame_tree/anchor_to_same_site_location.html (right): https://codereview.chromium.org/2686943002/diff/20001/chrome/test/data/frame_... 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 2017/02/09 22:58:36, alexmos wrote: > On 2017/02/09 16:53:01, Łukasz Anforowicz wrote: > > Charlie was pointing out that another interesting scenario here is > > target="_blank". Currently this scenario still leads to the old process being > > reused. > > Interesting, so ctrl-click on <a target="_blank"> would reuse the process? Yes, the new window/tab has: - same process - same site-instance - window.opener is set - window.open is able to find the opener by name creis@ pointed out in https://crbug.com/658386#c9 that this is different from the behavior in Firefox (which clears out window.opener in this case). > Do you know why? I am not sure, but I've verified that in case of target="..." we go through createWindowForRequest in Blink. We don't go through createWindowForRequest (and go through OpenURL instead) in case of ctrl-click without target="...". > What if the target is something else, like an existing (or > non-existing) name? What's the behavior of ctrl-click there? This is a great question - thanks for asking. It turns out that in case of an existing name, things work as expected (no opener, different process, window.open doesn't find the opener, etc.). I see in FrameLoader::load that createWindowForRequest path is only taken if the target frame doesn't exist yet and otherwise, we end up in OpenURL path: #2 0x7f7aeb4de036 content::RenderFrameImpl::OpenURL() #3 0x7f7aeb4dd464 content::RenderFrameImpl::loadURLExternally() #4 0x7f7adeb3bba1 blink::FrameLoaderClientImpl::loadURLExternally() #5 0x7f7adacd08fb blink::FrameLoader::shouldContinueForNavigationPolicy() #6 0x7f7adacd0f6e blink::FrameLoader::checkLoadCanStart() #7 0x7f7adaccb61b blink::FrameLoader::startLoad() #8 0x7f7adacbf003 blink::FrameLoader::load() #9 0x7f7ad99fc499 blink::HTMLAnchorElement::handleClick() For non-existent targets we take a different path in FrameLoader::load (and return early - createWindowForRequest handles continuing the navigation): #2 0x7fcdb9de6acf blink::createWindowForRequest() #3 0x7fcdb9ce7100 blink::FrameLoader::load() #4 0x7fcdb8a24499 blink::HTMLAnchorElement::handleClick() The case with target="_blank" is important to fix as well (AFAICT this is what https://crbug.com/23815 deals with [according to #c1]), but I hope to do this in a separate CL (probably tweaking how FrameLoader::load decides whether to take the createWindowForRequest path). https://codereview.chromium.org/2686943002/diff/40001/chrome/browser/chrome_n... File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2686943002/diff/40001/chrome/browser/chrome_n... chrome/browser/chrome_navigation_browsertest.cc:235: new_contents->GetMainFrame()->GetProcess()); On 2017/02/09 22:58:36, alexmos wrote: > Might be good to compare their SiteInstances as well. Done. https://codereview.chromium.org/2686943002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2686943002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_test.cc:2140: // Window created by chrome.windows.create should be in the same site instance On 2017/02/09 22:58:36, alexmos wrote: > nit: s/site instance/SiteInstance/ Done. https://codereview.chromium.org/2686943002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_test.cc:2169: EXPECT_EQ(extension_url.spec(), url_of_found_window); On 2017/02/09 22:58:36, alexmos wrote: > I'm a bit confused here: is this testing that old_contents can find itself by > name? Did you mean to look up the new_contents by name instead? But then the > two windows have the same URL, so if window.open returned the wrong window, the > URL comparison wouldn't catch it. Doh... thanks for catching this. I meant to execute this script in new_contents. OTOH, I see that window.open doesn't find |new_contents| (i.e. url_of_found_window is "about:blank") at ToT / without any fix (or with this CL). This is probably okay, although it means that I don't quite understand the impact of different SiteInstance (mind you - same process) on the hangouts extension. Also - when window.open doesn't find the other window, it opens yet another window which is frowned upon by WebContentsAddedObserver - this is why I've nested it into a smaller code block, so its lifetime ends earlier (OTOH, this is not needed after removing the call to window.open... still - this is probably better form to popularize for copy&paste by others later). https://codereview.chromium.org/2686943002/diff/40001/chrome/test/data/frame_... File chrome/test/data/frame_tree/anchor_to_same_site_location.html (right): https://codereview.chromium.org/2686943002/diff/40001/chrome/test/data/frame_... chrome/test/data/frame_tree/anchor_to_same_site_location.html:16: that points to the same site. See also https://crbug.com/238156. On 2017/02/09 22:58:36, alexmos wrote: > Typo in bug number? Ooops. Done.
https://codereview.chromium.org/2686943002/diff/20001/chrome/test/data/frame_... File chrome/test/data/frame_tree/anchor_to_same_site_location.html (right): https://codereview.chromium.org/2686943002/diff/20001/chrome/test/data/frame_... 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 2017/02/10 00:38:18, Łukasz Anforowicz wrote: > On 2017/02/09 22:58:36, alexmos wrote: > > On 2017/02/09 16:53:01, Łukasz Anforowicz wrote: > > > Charlie was pointing out that another interesting scenario here is > > > target="_blank". Currently this scenario still leads to the old process > being > > > reused. > > > > Interesting, so ctrl-click on <a target="_blank"> would reuse the process? > > Yes, the new window/tab has: > - same process > - same site-instance > - window.opener is set > - window.open is able to find the opener by name > > creis@ pointed out in https://crbug.com/658386#c9 that this is different from > the behavior in Firefox (which clears out window.opener in this case). > > > Do you know why? > > I am not sure, but I've verified that in case of target="..." we go through > createWindowForRequest in Blink. We don't go through createWindowForRequest > (and go through OpenURL instead) in case of ctrl-click without target="...". > > > What if the target is something else, like an existing (or > > non-existing) name? What's the behavior of ctrl-click there? > > This is a great question - thanks for asking. It turns out that in case of an > existing name, things work as expected (no opener, different process, > window.open doesn't find the opener, etc.). I see in FrameLoader::load that > createWindowForRequest path is only taken if the target frame doesn't exist yet > and otherwise, we end up in OpenURL path: > > #2 0x7f7aeb4de036 content::RenderFrameImpl::OpenURL() > #3 0x7f7aeb4dd464 content::RenderFrameImpl::loadURLExternally() > #4 0x7f7adeb3bba1 blink::FrameLoaderClientImpl::loadURLExternally() > #5 0x7f7adacd08fb blink::FrameLoader::shouldContinueForNavigationPolicy() > #6 0x7f7adacd0f6e blink::FrameLoader::checkLoadCanStart() > #7 0x7f7adaccb61b blink::FrameLoader::startLoad() > #8 0x7f7adacbf003 blink::FrameLoader::load() > #9 0x7f7ad99fc499 blink::HTMLAnchorElement::handleClick() > > For non-existent targets we take a different path in FrameLoader::load (and > return early - createWindowForRequest handles continuing the navigation): > > #2 0x7fcdb9de6acf blink::createWindowForRequest() > #3 0x7fcdb9ce7100 blink::FrameLoader::load() > #4 0x7fcdb8a24499 blink::HTMLAnchorElement::handleClick() > > > The case with target="_blank" is important to fix as well (AFAICT this is what > https://crbug.com/23815 deals with [according to #c1]), but I hope to do this in > a separate CL (probably tweaking how FrameLoader::load decides whether to take > the createWindowForRequest path). > BTW: I am trying to address the target=_blank case in https://codereview.chromium.org/2680353005 (I am currently waiting for tryjob results before sending it out for broader review - either to you or to japhet@).
The CQ bit was checked by lukasza@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lukasza@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 ========== New WebContents created via ctrl-click should be in a new process. This CL puts web contents created via ctrl-click 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. This CL also fixes/refactors two layout tests that used to depend on ability to find the opener via window.open, even in case of ctrl-click or shift-click. The tests had to be refactored so that they don't depend on ability to comminicate back with the main test window. After this CL the tests report their results via testRunner.setCustomTextOutput (rather than sending the results back to the main window using window.postMessage). BUG=23815, 597750, 658386 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== 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 ==========
The CQ bit was checked by lukasza@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...)
Alex, can you take another look please? I think that back in February we agreed that the main change in content/browser/frame_host/navigator_impl.cc is desirable + I've addressed your CR feedback, but didn't have a chance to follow-up until coming back from a paternity leave a few days ago.
lukasza@chromium.org changed reviewers: + creis@chromium.org
+creis@ to reviewers There were questions what happens when RequestOpenURL is used in cases other than ctrl-click. I believe that in these other cases chrome::NavigateParams::source_site_instance won't get looked at (and therefore these other cases should not be affected by this CL). To try to confirm the above, I looked for references to chrome::NavigateParams::source_site_instance and found 9. Note that we only care about cases that *read* the value (to see in what cases *outside of ctrl-click* the value is used). 1. chrome/browser/extensions/api/tabs/tabs_api.cc: WindowsCreateFunction::Run - this case *sets* the value and does depend on source_site_instance to have the newly opened tab in the same process as the extension calling this API. Regressions for this case should be prevented by the new test being added in this CL - ExtensionApiTest.WindowCreateVsSiteInstance in chrome/browser/extensions/api/tabs/tabs_test.cc 2. chrome/browser/ui/browser_navigator.cc: GetSourceProfile: used to prevent cross-profile information sharing. 3. chrome/browser/ui/browser_navigator.cc: NavigationController::LoadURLParams::source_site_instance: used to set source_site_instance on content::NavigationEntryImpl. 4. chrome/browser/ui/browser_navigator.cc: CreateTargetContents: This is the only place that we want to influence by this CL / this is where new contents for ctrl-click get created. 5. chrome/browser/ui/browser_navigator_params.cc: This just copies the value between instances of chrome::NavigateParams. We might want to spend some more time thinking whether 2 and 3 are really okay after this CL.
A couple more thoughts below. Overall I think we can give this a try, but I'm also curious to see what Charlie says. > Thanks for pointing out RenderFrameHostManager::CanSubframeSwapProcess. I > think the main thing we need to worry about here is that without a site > instance we might *avoid* a necessary process swap. Let me try to enumarate > all the conditions necessary to do the wrong thing here: > 1. Navigation has to go through OpenURL (which used to set source_site_instance > before this CL, but doesn't anymore after this CL). > 2. Navigation has to be to data: or about:blank > 3. Navigation has to be initiated from frameA, target different frameB, and > frameA has to be cross-site from frameB (so that > RenderFrameHostManager::CanSubframeSwapProcess will say that a process swap > is okay). > Can we chat how the conditions above might be trigerred? I am especially > blurry on how to trigger #1 (this would be hit for web<->extension navigations, > but it seems at odds with #2 and #3, right?). A few more random thoughts related to your comments above. 1. Although we can get into RequestOpenURL for subframes, this really only matters for main frames, as the source_site_instance is only used in two places: - CreateTargetContents (creates new WebContents) - GetSourceProfile for chrome::Navigate. I think I put that in for chrome.windows.create with multiple URLs, but it shouldn't matter for subframes, as they shouldn't be ending up in a different profile. 2. Since CreateTargetContents will create a new BrowsingInstance without source_site_instance, we might also need to worry about any cases that used to be able to open a window (without ctrl-click) and then access window.opener, but now the opener will be null - right? Or would CreateTargetContents never be reached for a regular window.open or anything other than ctrl-click? 3. I recently learned that we can go through OpenURL for some obscure kinds of data: URLs, see https://crbug.com/697513#c1. So one potential case that might differ is loading one of those URLs in a new window without ctrl-click. Putting it into a new process seems fine (the data: window won't be able to script the opener anyway), but if it tries to postMessage its window.opener, that might be problematic. Again, just pointing this out; I'm not sure if that actually matters in practice if window.open doesn't use CreateTargetContents (which it probably doesn't). 4. Looking at decidePolicyForNavigation, looks like there's a pref that we set for app windows and some other cases, browser_handles_all_top_level_requests, which punts all top-level navigations to the browser. I'm not familiar with that flag - maybe Charlie knows what it's for? Might be worth checking if window.open() from an app window would be subject to this, and whether an app's popups could lose the ability to communicate with the opener. (I also recently discovered that some renderer-initiated navigations from an app URL, namely reloads to the same URL, go through OpenURL path - see https://bugs.chromium.org/p/chromium/issues/detail?id=691941#c7) https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/chrome_... File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:240: // Verify that |new_contents| truly is in a brand new browsing instance. Might be nice to also check that main_instance->IsRelatedSiteInstance(new_instance) returns false https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:246: main_contents->GetMainFrame(), optional: this will also work if you just pass in main_contents (similarly below) https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:272: // Expecting "false" -> expecting to be at a non-PAGE_TYPE_NORMAL page. I'm curious why this is the case? I.e., why wouldn't it just be a normal navigation to "about:blank"? https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:2151: IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowCreateVsSiteInstance) { nit: WindowCreate -> WindowsCreate in the name (to correspond to windows.create) https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:2174: // Note: both test assertion are important - one observed failure mode was nit: s/assertion/assertions/
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Thanks for the review. I've tried to gather replies to your and my comments below. Also - one thing that I want to point out is that I plan to land this CL after M59 branch point (so Friday at the earliest). RE: |browser_handles_all_top_level_requests| I've seen this used in tests (and wrote 1 or 2 tests using it myself). I mentioned this to creis@ and he confirmed that this is mainly a test knob, but pointed out that some embedders might be using this flag. He didn't feel particularily worried though (especially since such embedders will need to adjust anyhow once PlzNavigate ships). RE: GetSourceProfile This seems to work fine when I tried to manually test if "Open link in incognito window" works (while logging whether chrome::Navigate performs the cleanup expected in cross-profile navigation case). This worked fine, because GetSourceProfile falls back to chrome::NavigationParams::source_conents (and a comment in GetSourceProfile says that inspecting chrome::NavigationParams::source_site_instance is mainly needed for chrome.windows.create extension API which we know is not affected by thie CL). RE: Which cases are affected by this CL AFAIK creation of a new WebContents for window.open is handled by a different path in Blink - via CreateWindowForRequest. Also - in this case *creation* of new WebContents is done separately (by CreateWindowForRequest) from actually *navigating* the WebContents. This CL affects cases where 1) RequestOpenURL is used and 2) setting of source_site_instance is important. I think we are saying that #2 is not important anymore (after switching to RequestTransferURL for handling remote frames). In a chat with creis@ I also pointed out that we hope that RequestOpenURL will go away entirely after the switch to PlzNavigate - in this case the tests from the CL-under-review will be nice but the product changes (and side effects of product changes) will be irrelevant. RE: content::NavigationEntryImpl::source_site_instance creis@ and me couldn't come up with a use case where this would matter. In fact, in a chat between you/alexmos@, me/lukasza@ and creis@ we've hypothesized that maybe significant parts of https://crrev.com/743773003 are not needed anymore and can be removed in a follow-up CL. https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/chrome_... File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:240: // Verify that |new_contents| truly is in a brand new browsing instance. On 2017/04/11 20:42:39, alexmos wrote: > Might be nice to also check that > main_instance->IsRelatedSiteInstance(new_instance) returns false Done. https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:246: main_contents->GetMainFrame(), On 2017/04/11 20:42:39, alexmos wrote: > optional: this will also work if you just pass in main_contents (similarly > below) Done. https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:272: // Expecting "false" -> expecting to be at a non-PAGE_TYPE_NORMAL page. On 2017/04/11 20:42:39, alexmos wrote: > I'm curious why this is the case? I.e., why wouldn't it just be a normal > navigation to "about:blank"? IsLastCommittedEntryOfPageType returns false, because web_contents->GetController().GetLastCommittedEntry() returns nullptr, because |last_committed_entry_index_ == -1|. Does that make sense? I am just reporting what I am seeing, I don't really know if going to about:blank should count as a committed entry or not... https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:2151: IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowCreateVsSiteInstance) { On 2017/04/11 20:42:40, alexmos wrote: > nit: WindowCreate -> WindowsCreate in the name (to correspond to windows.create) Done. https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:2174: // Note: both test assertion are important - one observed failure mode was On 2017/04/11 20:42:40, alexmos wrote: > nit: s/assertion/assertions/ Ooops. Done.
Thanks! LGTM, but let's wait for Charlie to also review. And thanks for all the detailed explanations on what this CL might affect -- I agree with your reasoning. https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/chrome_... File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:272: // Expecting "false" -> expecting to be at a non-PAGE_TYPE_NORMAL page. On 2017/04/11 23:10:37, Łukasz A. wrote: > On 2017/04/11 20:42:39, alexmos wrote: > > I'm curious why this is the case? I.e., why wouldn't it just be a normal > > navigation to "about:blank"? > > IsLastCommittedEntryOfPageType returns false, because > web_contents->GetController().GetLastCommittedEntry() returns nullptr, because > |last_committed_entry_index_ == -1|. > > Does that make sense? I am just reporting what I am seeing, I don't really know > if going to about:blank should count as a committed entry or not... Strange. I'd expect the new popup to commit "about:blank" here. Does it work if you do window.open("/title1.html",...) rather than an empty URL? It might be that we're calling WaitForLoadStop after the new WC is created but before the navigation we want to wait for is started, so it returns immediately? (If the two windows *were* in the same BrowsingInstance, then window.open('','main_contents') wouldn't commit anything, whereas window.open('about:blank','main_contents') would commit about:blank in the existing window. But if window.open opens a new window, I'd expect both cases to commit "about:blank".)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Thanks Alex. Charlie - can you PTAL? (Friday is fine :-). https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/chrome_... File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2686943002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:272: // Expecting "false" -> expecting to be at a non-PAGE_TYPE_NORMAL page. On 2017/04/12 22:08:29, alexmos wrote: > On 2017/04/11 23:10:37, Łukasz A. wrote: > > On 2017/04/11 20:42:39, alexmos wrote: > > > I'm curious why this is the case? I.e., why wouldn't it just be a normal > > > navigation to "about:blank"? > > > > IsLastCommittedEntryOfPageType returns false, because > > web_contents->GetController().GetLastCommittedEntry() returns nullptr, because > > |last_committed_entry_index_ == -1|. > > > > Does that make sense? I am just reporting what I am seeing, I don't really > know > > if going to about:blank should count as a committed entry or not... > > Strange. I'd expect the new popup to commit "about:blank" here. Does it work > if you do window.open("/title1.html",...) rather than an empty URL? It might be > that we're calling WaitForLoadStop after the new WC is created but before the > navigation we want to wait for is started, so it returns immediately? > > (If the two windows *were* in the same BrowsingInstance, then > window.open('','main_contents') wouldn't commit anything, whereas > window.open('about:blank','main_contents') would commit about:blank in the > existing window. But if window.open opens a new window, I'd expect both cases > to commit "about:blank".) Good call: - Previously found_contents->IsLoading() was false. - Things work as expected if I pass an actual URL (i.e. "about:blank") to window.open call. I've wondered about removing WaitForLoadStop and URL comparisons below, but I think they are somewhat desirable here (to make sure the opened window is a new window, not the old window).
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/12 22:40:04, Łukasz A. wrote: > Thanks Alex. Charlie - can you PTAL? (Friday is fine :-). Yes, I'll definitely take a look on Friday. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Let's explore the idea we chatted about, where we add a boolean to say whether this should be a new BrowsingInstance (similar to the opener_suppressed we have elsewhere, perhaps?). My main hesitation with using source_site_instance here is that it's kind of indirect, and we end up overloading the meaning of source_site_instance even further. That could affect places which actually want to know who started the navigation. I'm not entirely opposed to this approach if it behaves correctly, especially if RequestOpenURL really will go away post-PlzNavigate, but it does seem harder to reason about, and the length of this discussion tells me we don't want to make this code even more subtle if we can avoid it. :) On a side note, thanks for the extra tests! https://codereview.chromium.org/2686943002/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2686943002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:233: // Verify that the new tab is in a different process and SiteInstance from the nit: different process, SiteInstance, and BrowsingInstance from (Line 239 explicitly checks the BrowsingInstance.) https://codereview.chromium.org/2686943002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:242: // Verify that |new_contents| truly is in a brand new browsing instance. Let's say: Verify that the new BrowsingInstance can't see windows from the old one. https://codereview.chromium.org/2686943002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:245: // (this is a sanity check of test setup; this is not a product test). nit: Capitalize first "this", remove extra space after semicolon. https://codereview.chromium.org/2686943002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:266: "w = window.open('about:blank', 'main_contents');" This looks wrong to me-- my manual testing shows that passing about:blank here will force the window to navigate to about:blank even if it's in the same BrowsingInstance, so I'm guessing this part of the test will never fail. Did you mean to pass the empty string instead? Ah, from your earlier conversation, maybe this was so that you got a commit event in the popup? I don't think you'll get one with the empty string, but I do think we need to use the empty string here if we can. https://codereview.chromium.org/2686943002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2686943002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:2120: IN_PROC_BROWSER_TEST_F(ExtensionApiTest, TemporaryAddressSpoof) { Heh, thanks. Looks like I missed that in review. :)
The CQ bit was checked by lukasza@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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by lukasza@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.
The CQ bit was checked by lukasza@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.
Thanks for the feedback - can you take another look please? On 2017/04/14 22:28:32, Charlie Reis wrote: > Let's explore the idea we chatted about, where we add a boolean to say whether > this should be a new BrowsingInstance (similar to the opener_suppressed we have > elsewhere, perhaps?). Right - I've made a few changes since patchset #11 that you've looked at, but the major one is the introduction of use_new_renderer_for_new_contents boolean field in: - chrome::NavigateParams - content::OpenURLParams - corresponding parameter in content::Navigator::RequestOpenURL I've initially also added this field to the renderer->browser OpenURL IPC, but it doesn't seem needed (this field is always true for renderer initiated navigations with a window disposition that requests creation of a new content). I could go even further and limit presence of this field to the //chrome layer - in theory it is sufficient to always set chrome::NavigateParams::use_new_renderer_for_new_conents to true inside chrome::Browser::OpenURLFromTab (and always set it to false inside extensions::WindowsCreateFunction::Run). This felt a bit too magical/implicit so I didn't go that far. Also - some notes about the |use_new_renderer_for_new_contents| name: - I wanted to convey that this flag only matters for creation of new WebContents (and won't influence navigations with CURRENT_TAB disposition). - I wasn't sure if "new_renderer" bit is right, but it is 1) what is happening today and 2) what the intent of the caller is (even if it is achieved in a slightly indirect way - by avoiding setting of the initial site instance of the new WebContents). > My main hesitation with using source_site_instance here is that it's kind of > indirect, and we end up overloading the meaning of source_site_instance even > further. That could affect places which actually want to know who started the > navigation. I like the new boolean field, but I wanted to highlight that source_site_instance is not a very reliable source of information today (so I am not sure if "places which actually want to know who started the navigation" can rely on source_site_instance). For example, it seems that most of ~60 instantiations of content::OpenURLParams do not set source_site_instance (even in cases when the navigation is kind of initiated by clicking a specific web contents - for example in ExtensionMessageBubbleController::OnLinkClicked and/or WebstoreInlineInstaller::CheckInlineInstallPermitted). On one hand, we could try to make |source_site_instance| more robust (i.e. make sure it is set wherever applicable), but we could also try to get rid of this field / replace this field with other fields where this field is consumed: - Maybe CreateTargetContents and GetSourceProfile in chrome/browser/ui/browser_navigator.cc could use chrome::NavigateParams::source_contents rather than (redundant in this case?) chrome::NavigateParams::source_site_instance. To achieve this maybe extensions::WindowsCreateFunction::Run should set |source_contents| instead of setting |source_site_instance|. - OTOH, I am still not quite clear what is NavigationController::LoadURLParams::source_site_instance (this is the only other place that consumes chrome::NavigateParams::source_site_instance). > I'm not entirely opposed to this approach if it behaves correctly, especially if > RequestOpenURL really will go away post-PlzNavigate, but it does seem harder to > reason about, and the length of this discussion tells me we don't want to make > this code even more subtle if we can avoid it. :) I am happy we have this discussion and I agree that an explicit boolean field is clearer than playing with |source_site_instance|. At the same time I think that the most important piece of this CL are the new tests (the tests should stay more-or-less the same even if the implementation changes because of PlzNavigate or for other reasons). https://codereview.chromium.org/2686943002/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2686943002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:233: // Verify that the new tab is in a different process and SiteInstance from the On 2017/04/14 22:28:33, Charlie Reis wrote: > nit: different process, SiteInstance, and BrowsingInstance from > (Line 239 explicitly checks the BrowsingInstance.) Done. https://codereview.chromium.org/2686943002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:245: // (this is a sanity check of test setup; this is not a product test). On 2017/04/14 22:28:33, Charlie Reis wrote: > nit: Capitalize first "this", remove extra space after semicolon. Done. https://codereview.chromium.org/2686943002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:266: "w = window.open('about:blank', 'main_contents');" On 2017/04/14 22:28:33, Charlie Reis wrote: > This looks wrong to me-- my manual testing shows that passing about:blank here > will force the window to navigate to about:blank even if it's in the same > BrowsingInstance, so I'm guessing this part of the test will never fail. Did > you mean to pass the empty string instead? > > > Ah, from your earlier conversation, maybe this was so that you got a commit > event in the popup? I don't think you'll get one with the empty string, but I > do think we need to use the empty string here if we can. Argh... you're right - I should be using an empty string.
> I like the new boolean field, but I wanted to highlight that > source_site_instance is not a very reliable source of information today (so I am > not sure if "places which actually want to know who started the navigation" can > rely on source_site_instance). For example, it seems that most of ~60 > instantiations of content::OpenURLParams do not set source_site_instance (even > in cases when the navigation is kind of initiated by clicking a specific web > contents - for example in ExtensionMessageBubbleController::OnLinkClicked and/or > WebstoreInlineInstaller::CheckInlineInstallPermitted). On one hand, we could > try to make |source_site_instance| more robust (i.e. make sure it is set > wherever applicable), but we could also try to get rid of this field / replace > this field with other fields where this field is consumed: > - Maybe CreateTargetContents and GetSourceProfile in > chrome/browser/ui/browser_navigator.cc could use > chrome::NavigateParams::source_contents rather than (redundant in this case?) > chrome::NavigateParams::source_site_instance. To achieve this maybe > extensions::WindowsCreateFunction::Run should set |source_contents| instead of > setting |source_site_instance|. I'd be very happy if we could do this, but I recall source_contents had some intricate dependencies, e.g., being modified in the case where chrome.windows.create is used with multiple URLs and incognito mode, so it might be tricky to reset it in the loop. I ran into this when I introduced the source_site_instance check in GetSourceProfile, see the discussion on https://codereview.chromium.org/2372323003/diff/140001/chrome/browser/ui/brow... and https://codereview.chromium.org/2372323003/diff/140001/content/browser/frame_.... And there's this comment on source_contents and its behavior: // [in] The WebContents that initiated the Navigate() request if such // context is necessary. Default is NULL, i.e. no context. // [out] If NULL, this value will be set to the selected WebContents in // the originating browser prior to the operation performed by // Navigate(). However, if the originating page is from a different // profile (e.g. an OFF_THE_RECORD page originating from a non-OTR // window), then |source_contents| is reset to NULL. content::WebContents* source_contents; But if it works, I'd be very happy to get rid of one of these. https://codereview.chromium.org/2686943002/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2686943002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:266: "w = window.open('about:blank', 'main_contents');" On 2017/04/18 16:50:22, Łukasz A. wrote: > On 2017/04/14 22:28:33, Charlie Reis wrote: > > This looks wrong to me-- my manual testing shows that passing about:blank here > > will force the window to navigate to about:blank even if it's in the same > > BrowsingInstance, so I'm guessing this part of the test will never fail. Did > > you mean to pass the empty string instead? > > > > > > Ah, from your earlier conversation, maybe this was so that you got a commit > > event in the popup? I don't think you'll get one with the empty string, but I > > do think we need to use the empty string here if we can. > > Argh... you're right - I should be using an empty string. I'm a bit confused here. :) Isn't the point of this to check whether or not the a WebContents would be created? If "main_contents" resolves to an existing window (as would be the case for the same BrowsingInstance), we will time out on the GetWebContents() call. If it doesn't, it seems we'll get a new window whether we use an empty URL or "about:blank". That said, your new approach without the observer is even better. :)
On 2017/04/18 17:26:27, alexmos wrote: > > I like the new boolean field, but I wanted to highlight that > > source_site_instance is not a very reliable source of information today (so I > am > > not sure if "places which actually want to know who started the navigation" > can > > rely on source_site_instance). For example, it seems that most of ~60 > > instantiations of content::OpenURLParams do not set source_site_instance (even > > in cases when the navigation is kind of initiated by clicking a specific web > > contents - for example in ExtensionMessageBubbleController::OnLinkClicked > and/or > > WebstoreInlineInstaller::CheckInlineInstallPermitted). On one hand, we could > > try to make |source_site_instance| more robust (i.e. make sure it is set > > wherever applicable), but we could also try to get rid of this field / replace > > this field with other fields where this field is consumed: > > - Maybe CreateTargetContents and GetSourceProfile in > > chrome/browser/ui/browser_navigator.cc could use > > chrome::NavigateParams::source_contents rather than (redundant in this case?) > > chrome::NavigateParams::source_site_instance. To achieve this maybe > > extensions::WindowsCreateFunction::Run should set |source_contents| instead of > > setting |source_site_instance|. > > I'd be very happy if we could do this, but I recall source_contents had some > intricate dependencies, e.g., being modified in the case where > chrome.windows.create is used with multiple URLs and incognito mode, so it might > be tricky to reset it in the loop. I ran into this when I introduced the > source_site_instance check in GetSourceProfile, see the discussion on > https://codereview.chromium.org/2372323003/diff/140001/chrome/browser/ui/brow... > and > https://codereview.chromium.org/2372323003/diff/140001/content/browser/frame_.... > And there's this comment on source_contents and its behavior: > > // [in] The WebContents that initiated the Navigate() request if such > // context is necessary. Default is NULL, i.e. no context. > // [out] If NULL, this value will be set to the selected WebContents in > // the originating browser prior to the operation performed by > // Navigate(). However, if the originating page is from a different > // profile (e.g. an OFF_THE_RECORD page originating from a non-OTR > // window), then |source_contents| is reset to NULL. > content::WebContents* source_contents; > > But if it works, I'd be very happy to get rid of one of these. Charlie pointed out that |source_site_instance| can properly track a *frame* that initiates a navigation (which can be in a different SiteInstance from the main frame of |source_contents|) - this shows that what I outlined above is wrong. > https://codereview.chromium.org/2686943002/diff/200001/chrome/browser/chrome_... > File chrome/browser/chrome_navigation_browsertest.cc (right): > > https://codereview.chromium.org/2686943002/diff/200001/chrome/browser/chrome_... > chrome/browser/chrome_navigation_browsertest.cc:266: "w = > window.open('about:blank', 'main_contents');" > On 2017/04/18 16:50:22, Łukasz A. wrote: > > On 2017/04/14 22:28:33, Charlie Reis wrote: > > > This looks wrong to me-- my manual testing shows that passing about:blank > here > > > will force the window to navigate to about:blank even if it's in the same > > > BrowsingInstance, so I'm guessing this part of the test will never fail. > Did > > > you mean to pass the empty string instead? > > > > > > > > > Ah, from your earlier conversation, maybe this was so that you got a commit > > > event in the popup? I don't think you'll get one with the empty string, but > I > > > do think we need to use the empty string here if we can. > > > > Argh... you're right - I should be using an empty string. > > I'm a bit confused here. :) Isn't the point of this to check whether or not the > a WebContents would be created? If "main_contents" resolves to an existing > window (as would be the case for the same BrowsingInstance), we will time out on > the GetWebContents() call. If it doesn't, it seems we'll get a new window > whether we use an empty URL or "about:blank". > > That said, your new approach without the observer is even better. :) Hmmm... I guess that since we don't expect new contents, then navigating the (always new) contents should be okay. Still - I think it is desirable to avoid the unnecessary navigation in case the old contents are found - this makes it easier to 1) reason about test failures and 2) understand what the test is trying to do.
FYI - I've uploaded a new patchset where I've added explicit initializer for the new field in all the various overloads of chrome::NavigateParams and content::OpenURLParams constructors. Shame on me for missing that earlier. FWIW, I think initializing to "false" is the right thing here, because this effectively preserves the old behavior everywhere, except for OpenURL IPC.
Thanks! This LGTM, with one important question in CreateTargetContents. On 2017/04/18 17:48:51, Łukasz A. wrote: > On 2017/04/18 17:26:27, alexmos wrote: > > > I like the new boolean field, but I wanted to highlight that > > > source_site_instance is not a very reliable source of information today (so > I > > am > > > not sure if "places which actually want to know who started the navigation" > > can > > > rely on source_site_instance). For example, it seems that most of ~60 > > > instantiations of content::OpenURLParams do not set source_site_instance > (even > > > in cases when the navigation is kind of initiated by clicking a specific web > > > contents - for example in ExtensionMessageBubbleController::OnLinkClicked > > and/or > > > WebstoreInlineInstaller::CheckInlineInstallPermitted). On one hand, we > could > > > try to make |source_site_instance| more robust (i.e. make sure it is set > > > wherever applicable), but we could also try to get rid of this field / > replace > > > this field with other fields where this field is consumed: > > > - Maybe CreateTargetContents and GetSourceProfile in > > > chrome/browser/ui/browser_navigator.cc could use > > > chrome::NavigateParams::source_contents rather than (redundant in this > case?) > > > chrome::NavigateParams::source_site_instance. To achieve this maybe > > > extensions::WindowsCreateFunction::Run should set |source_contents| instead > of > > > setting |source_site_instance|. > > > > I'd be very happy if we could do this, but I recall source_contents had some > > intricate dependencies, e.g., being modified in the case where > > chrome.windows.create is used with multiple URLs and incognito mode, so it > might > > be tricky to reset it in the loop. I ran into this when I introduced the > > source_site_instance check in GetSourceProfile, see the discussion on > > > https://codereview.chromium.org/2372323003/diff/140001/chrome/browser/ui/brow... > > and > > > https://codereview.chromium.org/2372323003/diff/140001/content/browser/frame_.... > > And there's this comment on source_contents and its behavior: > > > > // [in] The WebContents that initiated the Navigate() request if such > > // context is necessary. Default is NULL, i.e. no context. > > // [out] If NULL, this value will be set to the selected WebContents in > > // the originating browser prior to the operation performed by > > // Navigate(). However, if the originating page is from a different > > // profile (e.g. an OFF_THE_RECORD page originating from a non-OTR > > // window), then |source_contents| is reset to NULL. > > content::WebContents* source_contents; > > > > But if it works, I'd be very happy to get rid of one of these. > > Charlie pointed out that |source_site_instance| can properly track a *frame* > that initiates a navigation (which can be in a different SiteInstance from the > main frame of |source_contents|) - this shows that what I outlined above is > wrong. While source_contents may not be the right replacement, I agree that source_site_instance has some issues and may be worth replacing in the future. I imagine Daniel's new initiator plumbing might be an option, or we could track the source RenderFrameHost. At any rate, we don't need to solve that now, and I'm glad we're not adding more meaning into source_site_instance (and when it's absent). https://codereview.chromium.org/2686943002/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2686943002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:266: "w = window.open('about:blank', 'main_contents');" On 2017/04/18 17:26:27, alexmos (OOO until 5-2) wrote: > On 2017/04/18 16:50:22, Łukasz A. wrote: > > On 2017/04/14 22:28:33, Charlie Reis wrote: > > > This looks wrong to me-- my manual testing shows that passing about:blank > here > > > will force the window to navigate to about:blank even if it's in the same > > > BrowsingInstance, so I'm guessing this part of the test will never fail. > Did > > > you mean to pass the empty string instead? > > > > > > > > > Ah, from your earlier conversation, maybe this was so that you got a commit > > > event in the popup? I don't think you'll get one with the empty string, but > I > > > do think we need to use the empty string here if we can. > > > > Argh... you're right - I should be using an empty string. > > I'm a bit confused here. :) Isn't the point of this to check whether or not the > a WebContents would be created? If "main_contents" resolves to an existing > window (as would be the case for the same BrowsingInstance), we will time out on > the GetWebContents() call. If it doesn't, it seems we'll get a new window > whether we use an empty URL or "about:blank". > > That said, your new approach without the observer is even better. :) On 2017/04/18 17:48:51, Łukasz A. wrote: > Hmmm... I guess that since we don't expect new contents, then navigating the > (always new) contents should be okay. Still - I think it is desirable to avoid > the unnecessary navigation in case the old contents are found - this makes it > easier to 1) reason about test failures and 2) understand what the test is > trying to do. Right-- I'm happy with the new approach. Alex is probably right that the GetWebContents() call would hang, but the EXPECT_EQ(url::kAboutBlankURL...) line below stood out to me as something that would never fail. Glad you've updated it. https://codereview.chromium.org/2686943002/diff/320001/chrome/browser/chrome_... File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2686943002/diff/320001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:200: // browsing instance from |main|contents. Returns contents of the newly nit: |main_contents| https://codereview.chromium.org/2686943002/diff/320001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:294: // Test that each subsequent ctrl-click also gets a new process. Nice-- I like this (especially given the unexpected behavior we saw recently for subsequent ctrl-clicks)! https://codereview.chromium.org/2686943002/diff/320001/chrome/browser/ui/brow... File chrome/browser/ui/browser_navigator.cc (right): https://codereview.chromium.org/2686943002/diff/320001/chrome/browser/ui/brow... chrome/browser/ui/browser_navigator.cc:359: create_params.site_instance = nullptr; I missed this before, but is there a reason we're not using GetSiteInstanceForNewTab like below? That one looks at the destination URL and creates a SiteInstance in advance in case it's a WebUI or extension URL, so that we don't immediately have to do a process swap. For example, I'm curious if a control click on a web-accessible extension URL (of an extension with a background page) would first create an empty SiteInstance and then swap to the extension's existing process. GetSiteInstanceForNewTab will use a new BrowsingInstance in these cases, so I think it might be safe to use. That would basically change the condition here to: (params.source_site_instance && !params.use_new_renderer_for_new_contents) ? params.source_site_instance : tab_util::GetSiteInstanceForNewTab(params.browser->profile(), url)); https://codereview.chromium.org/2686943002/diff/320001/chrome/browser/ui/brow... File chrome/browser/ui/browser_navigator_params.h (right): https://codereview.chromium.org/2686943002/diff/320001/chrome/browser/ui/brow... chrome/browser/ui/browser_navigator_params.h:135: // Controls whether newly created web contents (in case if |disposition| asks nit: Drop "if" https://codereview.chromium.org/2686943002/diff/320001/chrome/browser/ui/brow... chrome/browser/ui/browser_navigator_params.h:141: bool use_new_renderer_for_new_contents; I'm trying to think of alternatives for the name (e.g., new_contents_is_unrelated?), but the name you have seems clear enough. I'm ok with it. https://codereview.chromium.org/2686943002/diff/320001/content/public/browser... File content/public/browser/page_navigator.h (right): https://codereview.chromium.org/2686943002/diff/320001/content/public/browser... content/public/browser/page_navigator.h:85: // Controls whether newly created web contents (in case if |disposition| asks nit: Drop "if"
The CQ bit was checked by lukasza@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...
Thanks Charlie. https://codereview.chromium.org/2686943002/diff/320001/chrome/browser/chrome_... File chrome/browser/chrome_navigation_browsertest.cc (right): https://codereview.chromium.org/2686943002/diff/320001/chrome/browser/chrome_... chrome/browser/chrome_navigation_browsertest.cc:200: // browsing instance from |main|contents. Returns contents of the newly On 2017/04/19 20:11:11, Charlie Reis wrote: > nit: |main_contents| Ooops. Not sure how that happened. Done. Also - s/name/id/ (regarding how the anchor is identified). https://codereview.chromium.org/2686943002/diff/320001/chrome/browser/ui/brow... File chrome/browser/ui/browser_navigator.cc (right): https://codereview.chromium.org/2686943002/diff/320001/chrome/browser/ui/brow... chrome/browser/ui/browser_navigator.cc:359: create_params.site_instance = nullptr; On 2017/04/19 20:11:11, Charlie Reis wrote: > I missed this before, but is there a reason we're not using > GetSiteInstanceForNewTab like below? That one looks at the destination URL and > creates a SiteInstance in advance in case it's a WebUI or extension URL, so that > we don't immediately have to do a process swap. For example, I'm curious if a > control click on a web-accessible extension URL (of an extension with a > background page) would first create an empty SiteInstance and then swap to the > extension's existing process. > > GetSiteInstanceForNewTab will use a new BrowsingInstance in these cases, so I > think it might be safe to use. That would basically change the condition here > to: > > (params.source_site_instance && !params.use_new_renderer_for_new_contents) > ? params.source_site_instance > : tab_util::GetSiteInstanceForNewTab(params.browser->profile(), url)); Thanks for catching this - you're right this should use tab_util::GetSiteInstanceForNewTab if possible. https://codereview.chromium.org/2686943002/diff/320001/chrome/browser/ui/brow... File chrome/browser/ui/browser_navigator_params.h (right): https://codereview.chromium.org/2686943002/diff/320001/chrome/browser/ui/brow... chrome/browser/ui/browser_navigator_params.h:135: // Controls whether newly created web contents (in case if |disposition| asks On 2017/04/19 20:11:11, Charlie Reis wrote: > nit: Drop "if" Done. https://codereview.chromium.org/2686943002/diff/320001/content/public/browser... File content/public/browser/page_navigator.h (right): https://codereview.chromium.org/2686943002/diff/320001/content/public/browser... content/public/browser/page_navigator.h:85: // Controls whether newly created web contents (in case if |disposition| asks On 2017/04/19 20:11:11, Charlie Reis wrote: > nit: Drop "if" Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lukasza@chromium.org changed reviewers: + rdevlin.cronin@chromium.org, sky@chromium.org
+rdevlin.cronin@ for chrome/browser/extensions/api/tabs/tabs* +sky@ for chrome/browser/ui/browser_navigator* Can you PTAL?
What about tabs created by chrome.tabs.create()? Tabs updated via chrome.tabs.update()? https://codereview.chromium.org/2686943002/diff/340001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2686943002/diff/340001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:627: navigate_params.use_new_renderer_for_new_contents = false; This is probably checked later, but maybe we should set this to be true iff |url| is within the extension? (At the very least, we should document why this is safe, since at first glance it looks scary.) https://codereview.chromium.org/2686943002/diff/340001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (left): https://codereview.chromium.org/2686943002/diff/340001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:2119: IN_PROC_BROWSER_TEST_F(ExtensionApiTest, Foo) { Thanks! This was bothering me for awhile and just never got around to fixing it.
The CQ bit was checked by lukasza@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...
Thanks Devlin - can you take another look please? Charlie - note that I've changed the comments documenting the new field - you might want to double-check that they still look okay in https://codereview.chromium.org/2686943002/patch/380001/390014 (same comment in page_navigator.h and browser_navigator_params.h). On 2017/04/20 16:22:30, Devlin wrote: > What about tabs created by chrome.tabs.create()? Interesting - thanks for mentioning it. I see that ExtensionTabUtil::OpenTab (called by TabsCreateFunction::Run) calls chrome::Navigate in a way similar to WindowsCreateFunction::Run, but there are some differences: 1. Both set chrome::NavigateParams::disposition to something other than CURRENT_TAB (WindowsCreateFunction always sets it to NEW_FOREGROUND_TAB [yes, not NEW_WINDOW] and TabsCreateFunction/ExtensionTabUtil::OpenTab sets it to either NEW_FOREGROUND_TAB or NEW_BACKGROUND_TAB. 2. WindowsCreateFunction sets chrome::NavigateParams::source_site_instance to make sure the newly created WebContents can find the opener. This does not happen from TabsCreateFunction/ExtensionTabUtil::OpenTab. Is the discrepancy in (2) above a bug? I know that the hangouts extension depends on the behavior in WindowsCreateFunction. I am guessing that so far nobody cared about the behavior of TabsCreateFunction (wrt findability of windows). FWIW, I think the issues raised above shouldn't block the current CL (because the current CL simply preserves the old behavior raised above). > Tabs updated via chrome.tabs.update()? AFAICT chrome.tabs.update only navigates an *existing* tab (so will not be affected by this CL, which only changes how CreateTargetContents works when new WebContents need to be created). chrome.windows.update doesn't touch the committed URL. https://codereview.chromium.org/2686943002/diff/340001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2686943002/diff/340001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:627: navigate_params.use_new_renderer_for_new_contents = false; On 2017/04/20 16:22:30, Devlin wrote: > This is probably checked later, but maybe we should set this to be true iff > |url| is within the extension? (At the very least, we should document why this > is safe, since at first glance it looks scary.) 1. Note that the current CL doesn't change the behavior here (although the change above makes it more explicit what is going on). 2. I've reworded the comment above to make it clearer what is going on. 3. I've also changed |use_new_renderer_for_new_contents| into |force_new_renderer_for_new_contents| - hopefully this better conveys that we might still use a new renderer (for cross-site contents) even if |force_new_renderer_for_new_contents| is false. https://codereview.chromium.org/2686943002/diff/340001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2686943002/diff/340001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:2179: new_contents->GetMainFrame()->GetSiteInstance()); Charlie suggested that we should also be explicit about |window.opener| here - I've added this verification in a recent patchset. I see that I already have verification of |window.opener| being null in the other test being added in this CL. https://codereview.chromium.org/2686943002/diff/360001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2686943002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:2186: EXPECT_FALSE(window_opener_cast_to_bool); I find the current behavior above a bit weird (note that the current CL doesn't change this behavior - it only adds explicit test to prevent hangouts regressions). It is weird that 1) windows can find each other by window.open('', other_window_name), but 2) window.opener is null. Maybe things would be cleaner if chrome.windows.create would populate window.opener (since it does put the new window in the same BrowsingInstance). I am guessing that this might be possibly accomplished by having chrome.windows.create (somehow) set WebContents::CreateParams::opener_xxx fields, rather than relying on the (somewhat indirect/magical) influence that WebContents::CreateParams::site_instance has on BrowsingInstance/window-findability.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2686943002/diff/380001/chrome/browser/ui/brow... File chrome/browser/ui/browser_navigator_params.h (right): https://codereview.chromium.org/2686943002/diff/380001/chrome/browser/ui/brow... chrome/browser/ui/browser_navigator_params.h:139: bool force_new_renderer_for_new_contents; I find the use of renderer mildly confusing here. WDYT of new_process? Also, please initialize this value. I'm ok with doing it right here.
extensions lgtm. Can you file a bug where we can track the inconsistency of opener relationships via tabs.create and windows.create? https://codereview.chromium.org/2686943002/diff/380001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2686943002/diff/380001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:2163: content::WebContents* new_contents; nit: initialize (to nullptr) https://codereview.chromium.org/2686943002/diff/380001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:2182: bool window_opener_cast_to_bool; nit: initialize
Thanks for the feedback (and especially for catching the lack of
initialization... :-/ ).
Devlin: I've also discussed with Charlie whether it might make sense to avoid
starting with an extension process if we know that chrome.windows.create is
opening web content. This would avoid a process transfer (desirable), but AFAIU
without the extra transfer window.open('', 'name_of_opener') will not work
anymore (which makes it a no-go I think).
https://codereview.chromium.org/2686943002/diff/380001/chrome/browser/extensi...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):
https://codereview.chromium.org/2686943002/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/api/tabs/tabs_test.cc:2163: content::WebContents*
new_contents;
On 2017/04/20 21:06:10, Devlin wrote:
> nit: initialize (to nullptr)
Done.
https://codereview.chromium.org/2686943002/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/api/tabs/tabs_test.cc:2182: bool
window_opener_cast_to_bool;
On 2017/04/20 21:06:10, Devlin wrote:
> nit: initialize
Done.
https://codereview.chromium.org/2686943002/diff/380001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_navigator_params.h (right):
https://codereview.chromium.org/2686943002/diff/380001/chrome/browser/ui/brow...
chrome/browser/ui/browser_navigator_params.h:139: bool
force_new_renderer_for_new_contents;
On 2017/04/20 21:00:57, sky wrote:
> I find the use of renderer mildly confusing here. WDYT of new_process?
Done.
> Also, please initialize this value.
Doh... Thanks for catching this. I've thought I've added initialization when
posting https://codereview.chromium.org/2686943002/#msg61, but apparently I've
made changes and uploaded in a wrong CL
(https://codereview.chromium.org/2680353005/#ps140001) and then these changes
got lost in rebasing...
> I'm ok with doing it right here.
I've added initializers to the constructors for consistency with the old code.
The CQ bit was checked by lukasza@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...
LGTM
Thanks for the reviews everyone - I'll push to CQ in a few minutes. On 2017/04/20 21:06:10, Devlin wrote: > Can you file a bug where we can track the inconsistency of > opener relationships via tabs.create and windows.create? I've opened https://crbug.com/713888
The CQ bit was unchecked by lukasza@chromium.org
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org, creis@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2686943002/#ps400001 (title: "s/renderer/process/ in the field name + initializing the field and variables.")
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": 400001, "attempt_start_ts": 1492728346921630,
"parent_rev": "92dc022ddbd78f9f7ca77d9fc072b636c96b9c06", "commit_rev":
"f206da906ef1e94ee03feeb67e11a82c565baa3b"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/f206da906ef1e94ee03feeb67e11... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as https://chromium.googlesource.com/chromium/src/+/f206da906ef1e94ee03feeb67e11... |
