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

Issue 2809653008: Don't create unnecessary OOPIFs for subframe navigations that don't require a dedicated process. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 1 week ago by alexmos
Modified:
4 months ago
CC:
chromium-reviews, extensions-reviews_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, site-isolation-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't create unnecessary OOPIFs for subframe navigations that don't require a dedicated process. Previously, if we had a.com embedding chrome-extension://foo, and the subframe was navigated to b.com (with --isolate-extensions being on by default but without any other OOPIF modes), the subframe stayed in an OOPIF in a new process for b.com, even though technically the b.com OOPIF is not necessary per --isolate-extensions security model. This CL fixes that. BUG=711006 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2809653008 Cr-Commit-Position: refs/heads/master@{#465055} Committed: https://chromium.googlesource.com/chromium/src/+/24409702df38cbd9505f1cae180be98d2f7e148d

Patch Set 1 #

Patch Set 2 : Fix devtools extensions tests #

Total comments: 6

Patch Set 3 : Rebase #

Patch Set 4 : Revise comment #

Total comments: 4

Messages

Total messages: 34 (21 generated)
alexmos
Charlie, can you please take a look? https://codereview.chromium.org/2809653008/diff/20001/content/browser/frame_host/render_frame_host_manager.cc File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2809653008/diff/20001/content/browser/frame_host/render_frame_host_manager.cc#newcode1478 content/browser/frame_host/render_frame_host_manager.cc:1478: !parent_url_requires_dedicated_process) { ...
4 months ago (2017-04-14 00:15:46 UTC) #12
Charlie Reis (slow)
Thanks for fixing this! LGTM with some thoughts below. Also, while I'd love to see ...
4 months ago (2017-04-14 16:57:05 UTC) #13
alexmos
https://codereview.chromium.org/2809653008/diff/20001/chrome/browser/extensions/process_manager_browsertest.cc File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2809653008/diff/20001/chrome/browser/extensions/process_manager_browsertest.cc#newcode1226 chrome/browser/extensions/process_manager_browsertest.cc:1226: IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest, On 2017/04/14 16:57:05, Charlie Reis wrote: > Yep, ...
4 months ago (2017-04-17 17:36:02 UTC) #16
Charlie Reis (slow)
Sounds good, thanks! LGTM.
4 months ago (2017-04-17 20:31:49 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2809653008/60001
4 months ago (2017-04-17 21:04:49 UTC) #21
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/413060)
4 months ago (2017-04-17 21:12:41 UTC) #23
alexmos
Devlin, can you please review the new test for OWNERS?
4 months ago (2017-04-17 21:26:31 UTC) #25
Devlin
lgtm https://codereview.chromium.org/2809653008/diff/60001/chrome/browser/extensions/process_manager_browsertest.cc File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2809653008/diff/60001/chrome/browser/extensions/process_manager_browsertest.cc#newcode1229 chrome/browser/extensions/process_manager_browsertest.cc:1229: if (content::AreAllSitesIsolatedForTesting()) Would it be worth enforcing this ...
4 months ago (2017-04-17 21:49:32 UTC) #26
alexmos
Thanks! https://codereview.chromium.org/2809653008/diff/60001/chrome/browser/extensions/process_manager_browsertest.cc File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2809653008/diff/60001/chrome/browser/extensions/process_manager_browsertest.cc#newcode1229 chrome/browser/extensions/process_manager_browsertest.cc:1229: if (content::AreAllSitesIsolatedForTesting()) On 2017/04/17 21:49:32, Devlin wrote: > ...
4 months ago (2017-04-17 22:39:00 UTC) #27
Devlin
https://codereview.chromium.org/2809653008/diff/60001/chrome/browser/extensions/process_manager_browsertest.cc File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2809653008/diff/60001/chrome/browser/extensions/process_manager_browsertest.cc#newcode1229 chrome/browser/extensions/process_manager_browsertest.cc:1229: if (content::AreAllSitesIsolatedForTesting()) On 2017/04/17 22:38:59, alexmos wrote: > On ...
4 months ago (2017-04-17 23:03:37 UTC) #28
alexmos
https://codereview.chromium.org/2809653008/diff/60001/chrome/browser/extensions/process_manager_browsertest.cc File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2809653008/diff/60001/chrome/browser/extensions/process_manager_browsertest.cc#newcode1229 chrome/browser/extensions/process_manager_browsertest.cc:1229: if (content::AreAllSitesIsolatedForTesting()) On 2017/04/17 23:03:37, Devlin wrote: > On ...
4 months ago (2017-04-17 23:07:58 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2809653008/60001
4 months ago (2017-04-17 23:08:41 UTC) #31
commit-bot: I haz the power
4 months ago (2017-04-17 23:13:48 UTC) #34
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/24409702df38cbd9505f1cae180b...
Sign in to reply to this message.

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