Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2461693002:
Change Isolate Extensions to be off by default. (Closed)
DescriptionChange Isolate Extensions to be off by default.
This CL changes the behavior of Isolate Extensions to be off by default,
unless it is explicitly enabled by field trial.
BUG=545200
Committed: https://crrev.com/9729c2081fc2daeb1bb487fe329aaf8458fac898
Cr-Commit-Position: refs/heads/master@{#428775}
Patch Set 1 #Patch Set 2 : Mostly a revert of r414879 #Patch Set 3 : Fix compile break. #Patch Set 4 : Fix tests which fail without Isolate Extensions. #
Total comments: 8
Patch Set 5 : Fixes based on review comments. #
Messages
Total messages: 39 (21 generated)
The CQ bit was checked by nasko@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...
nasko@chromium.org changed reviewers: + creis@chromium.org
Hey Charlie, Can you review this CL for me? It flips the default for Isolate Extensions to be disabled. Thanks in advance! Nasko
creis@chromium.org changed reviewers: + nick@chromium.org
creis@chromium.org changed required reviewers: + nick@chromium.org
[+nick, site-isolation-reviews] LGTM, but let's get Nick's thoughts as well. (He's good at spotting unexpected effects from changes like this.) I do think we should make this off by default before M55 reaches stable.
On 2016/10/28 17:01:33, Charlie Reis wrote: > [+nick, site-isolation-reviews] > > LGTM, but let's get Nick's thoughts as well. (He's good at spotting unexpected > effects from changes like this.) I do think we should make this off by default > before M55 reaches stable. Couple thoughts: - I expect this will cause, at least, ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked to fail. The policy->CanCommitURL() asserts need to be wrapped in if (IsIsolateExtensionsEnabled()). - I think this means we totally lose trybot coverage for browser_tests under --isolate-extensions, right? We migrated our FYI bot to be testing --site-per-process. Can we get that coverage back somehow (running a bot w/ --force-fieldtrials=)?
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_...)
Couple of drive-bys: On 2016/10/28 17:26:51, ncarter wrote: > On 2016/10/28 17:01:33, Charlie Reis wrote: > > [+nick, site-isolation-reviews] > > > > LGTM, but let's get Nick's thoughts as well. (He's good at spotting > unexpected > > effects from changes like this.) I do think we should make this off by > default > > before M55 reaches stable. > > Couple thoughts: > > - I expect this will cause, at least, > ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked to fail. The > policy->CanCommitURL() asserts need to be wrapped in if > (IsIsolateExtensionsEnabled()). I'm actually doing that in an in-progress CL to fix WAR checks: https://codereview.chromium.org/2454563003/. So we could either land this after that, or land that wrapping here and I'll rebase my CL on this. > > - I think this means we totally lose trybot coverage for browser_tests under > --isolate-extensions, right? We migrated our FYI bot to be testing > --site-per-process. Can we get that coverage back somehow (running a bot w/ > --force-fieldtrials=)? +1 that we should do this. I can switch back the Win bot to run with --force-fieldtrials=SiteIsolationExtensions/Enabled (or just --isolate-extensions).
On 2016/10/28 17:26:51, ncarter wrote: > On 2016/10/28 17:01:33, Charlie Reis wrote: > > [+nick, site-isolation-reviews] > > > > LGTM, but let's get Nick's thoughts as well. (He's good at spotting > unexpected > > effects from changes like this.) I do think we should make this off by > default > > before M55 reaches stable. > > Couple thoughts: > > - I expect this will cause, at least, > ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked to fail. The > policy->CanCommitURL() asserts need to be wrapped in if > (IsIsolateExtensionsEnabled()). Yes, there are a few tests that are failing. > - I think this means we totally lose trybot coverage for browser_tests under > --isolate-extensions, right? Yes. However, I don't think this should stick on trunk. My goal is to merge this back to M55 and then undo until we are ready to branch for M57, when we can reevaluate how far we are. > We migrated our FYI bot to be testing > --site-per-process. Can we get that coverage back somehow (running a bot w/ > --force-fieldtrials=)? If there is agreement on reverting this after merge, do we want to switch bots around for over the weekend?
On 2016/10/28 17:36:41, nasko (slow) wrote: > On 2016/10/28 17:26:51, ncarter wrote: > > On 2016/10/28 17:01:33, Charlie Reis wrote: > > > [+nick, site-isolation-reviews] > > > > > > LGTM, but let's get Nick's thoughts as well. (He's good at spotting > > unexpected > > > effects from changes like this.) I do think we should make this off by > > default > > > before M55 reaches stable. > > > > Couple thoughts: > > > > - I expect this will cause, at least, > > ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked to fail. The > > policy->CanCommitURL() asserts need to be wrapped in if > > (IsIsolateExtensionsEnabled()). > > Yes, there are a few tests that are failing. > > > - I think this means we totally lose trybot coverage for browser_tests under > > --isolate-extensions, right? > > Yes. However, I don't think this should stick on trunk. My goal is to merge this > back to M55 and then undo until we are ready to branch for M57, when we can > reevaluate how far we are. In that case, should we have a bot that's running tests in non-isolate-extensions mode? What if we regress some behavior with --isolate-extensions off? > > > We migrated our FYI bot to be testing > > --site-per-process. Can we get that coverage back somehow (running a bot w/ > > --force-fieldtrials=)? > > If there is agreement on reverting this after merge, do we want to switch bots > around for over the weekend?
The CQ bit was checked by nasko@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...
Restoring back to having the command line flag and off by default. I've included explicitly the --site-per-process flag as well, since it is what tests were depending on. I've also put up a CL to convert the Windows FYI bot to test with --isolate-extensions (https://codereview.chromium.org/2459813003) once this CL lands.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nasko@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks! Looks like we'll also need to update these relatively new tests, which are failing: ChromeSecurityExploitBrowserTest.CreateBlobInExtensionOrigin ChromeWebStoreProcessTest.NavigateWebTabToChromeWebStoreViaPost ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked
Added checks to the failing tests for IsIsolateExtensionsEnabled(). https://codereview.chromium.org/2461693002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2461693002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/process_manager_browsertest.cc:717: if (extensions::IsIsolateExtensionsEnabled()) { All of the following checks were added as part of Nick's CL, so I put them all in one block to keep parity with before.
lgtm https://codereview.chromium.org/2461693002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2461693002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/process_manager_browsertest.cc:716: // Note: this is only valid when --isolate-extensions is enabled. Omit this comment, for cleaner merges. https://codereview.chromium.org/2461693002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/process_manager_browsertest.cc:717: if (extensions::IsIsolateExtensionsEnabled()) { On 2016/10/28 21:18:42, nasko (out until nov 4) wrote: > All of the following checks were added as part of Nick's CL, so I put them all > in one block to keep parity with before. In the branches, "if (extensions::IsIsolateExtensionsEnabled()) {" is already present, but it's only around the second, CanCommitURL group of statements. Alex also has a pending CL that will reintroduce it this way.
LGTM. Thanks for going further and switching the default to be off (with support for the --isolate-extensions flag). It feels like a step backwards, but it's the right thing to do if we're delaying the stable launch until a later milestone. https://codereview.chromium.org/2461693002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/process_management_browsertest.cc (right): https://codereview.chromium.org/2461693002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:373: // is going to be terminated. Is this saying that we'll kill the process in non-isolate-extensions mode? That's ok for this CL, but we should fix that in a followup, since it sounds like it's regressing https://crbug.com/652708. :) https://codereview.chromium.org/2461693002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2461693002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/process_manager_browsertest.cc:717: if (extensions::IsIsolateExtensionsEnabled()) { On 2016/10/28 21:49:53, ncarter wrote: > On 2016/10/28 21:18:42, nasko (out until nov 4) wrote: > > All of the following checks were added as part of Nick's CL, so I put them all > > in one block to keep parity with before. > > In the branches, "if (extensions::IsIsolateExtensionsEnabled()) {" is already > present, but it's only around the second, CanCommitURL group of statements. Alex > also has a pending CL that will reintroduce it this way. +1 to matching Alex's https://codereview.chromium.org/2454563003/ for this part of the patch. (Side note, of no consequence: I'm finding the Rietveld diff fascinating, in terms of light green vs dark green. I wonder why it treats the EXPECT_TRUE lines as modified rather than just indented.)
The CQ bit was checked by nasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, nick@chromium.org Link to the patchset: https://codereview.chromium.org/2461693002/#ps80001 (title: "Fixes based on review comments.")
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
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_presub...)
nasko@chromium.org changed reviewers: + lazyboy@chromium.org
Adding lazyboy@ for OWNERS review of extensions code. https://codereview.chromium.org/2461693002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/process_management_browsertest.cc (right): https://codereview.chromium.org/2461693002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:373: // is going to be terminated. On 2016/10/28 23:46:33, Charlie Reis wrote: > Is this saying that we'll kill the process in non-isolate-extensions mode? > > That's ok for this CL, but we should fix that in a followup, since it sounds > like it's regressing https://crbug.com/652708. :) Yes, it will regress that, since the fix was dependent on --isolate-extensions. I will reopen the bug once this lands. https://codereview.chromium.org/2461693002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2461693002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/process_manager_browsertest.cc:716: // Note: this is only valid when --isolate-extensions is enabled. On 2016/10/28 21:49:53, ncarter wrote: > Omit this comment, for cleaner merges. Done. https://codereview.chromium.org/2461693002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/process_manager_browsertest.cc:717: if (extensions::IsIsolateExtensionsEnabled()) { On 2016/10/28 23:46:33, Charlie Reis wrote: > On 2016/10/28 21:49:53, ncarter wrote: > > On 2016/10/28 21:18:42, nasko (out until nov 4) wrote: > > > All of the following checks were added as part of Nick's CL, so I put them > all > > > in one block to keep parity with before. > > > > In the branches, "if (extensions::IsIsolateExtensionsEnabled()) {" is already > > present, but it's only around the second, CanCommitURL group of statements. > Alex > > also has a pending CL that will reintroduce it this way. > > +1 to matching Alex's https://codereview.chromium.org/2454563003/ for this part > of the patch. > > (Side note, of no consequence: I'm finding the Rietveld diff fascinating, in > terms of light green vs dark green. I wonder why it treats the EXPECT_TRUE > lines as modified rather than just indented.) Done.
extensions RS LGTM.
The CQ bit was checked by nasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Change Isolate Extensions to be off by default. This CL changes the behavior of Isolate Extensions to be off by default, unless it is explicitly enabled by field trial. BUG=545200 ========== to ========== Change Isolate Extensions to be off by default. This CL changes the behavior of Isolate Extensions to be off by default, unless it is explicitly enabled by field trial. BUG=545200 Committed: https://crrev.com/9729c2081fc2daeb1bb487fe329aaf8458fac898 Cr-Commit-Position: refs/heads/master@{#428775} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9729c2081fc2daeb1bb487fe329aaf8458fac898 Cr-Commit-Position: refs/heads/master@{#428775} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
