IsIsolateExtensionsEnabled check in BrowserProcessImpl constructor may not be working with the field trial's Control group |
||||||
Issue descriptionRepro steps: On Linux ToT debug build, run: browser_tests --gtest_filter=ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked --force-fieldtrials=SiteIsolationExtensions/Control This will result in a renderer kill 139 ((BDH_DISALLOWED_ORIGIN): [142272:142303:1019/143117:ERROR:bad_message.cc(23)] Terminating renderer for bad IPC message, reason 139 when the test tries to create a filesystem URL in the extension subframe. In comparison, change IsIsolateExtensionsEnabled() to always return false, and rerun the test. It will have a few expected failures for CanCommitURL, but it won't have the renderer kill and will successfully create the filesystem URL. The renderer kill apparently happens because IsIsolateExtensionsEnabled returns *true* inside BrowserProcessImpl when --force-fieldtrials=SiteIsolationExtensions/Control is specified. All subsequent IsIsolateExtensionsEnabled checks correctly return false with that flag, so it seems the FieldTrialList may not be initialized yet when BrowserProcessImpl is constructed? Here's how we get to that check: #1 0x000003c41e4b extensions::IsIsolateExtensionsEnabled() #2 0x000002f548b3 BrowserProcessImpl::BrowserProcessImpl() #3 0x0000032aaf24 ChromeBrowserMainParts::PreCreateThreadsImpl() #4 0x0000032aa885 ChromeBrowserMainParts::PreCreateThreads() #5 0x7fb4b86ec496 content::BrowserMainLoop::PreCreateThreads() I wonder if that implies that the filesystem URL origin grants/blocking won't work properly for our Control users, since this means that we'll use RegisterWebSafeIsolatedScheme instead of RegisterWebSafeScheme while the rest of the code will effectively run without --isolate-extensions. Or maybe it's only a problem if passing in the command-line override, but not for actual field trial users? Nick, do you want to take a look?
,
Oct 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a0b7b87ea093cbb45f51c4ab1465b4e4115a5d99 commit a0b7b87ea093cbb45f51c4ab1465b4e4115a5d99 Author: nick <nick@chromium.org> Date: Fri Oct 21 18:39:16 2016 BrowserProcessImpl: don't call IsIsolateExtensionsEnabled() too early This fixes a bug where IsIsolateExtensionsEnabled() had been called prior to the field trials actually being initialized, so the call returned true regardless of the finch experiment state, resulting in a possible misconfiguration of security state. PreCreateThreads() seems to be the earliest place in BrowserProcessImpl where we can do this registration. BUG= 657629 Review-Url: https://chromiumcodereview.appspot.com/2438823003 Cr-Commit-Position: refs/heads/master@{#426852} [modify] https://crrev.com/a0b7b87ea093cbb45f51c4ab1465b4e4115a5d99/chrome/browser/browser_process_impl.cc
,
Oct 21 2016
Fixed at TOT. We'd like to merge this to M55: there are already reports of renderer kills in the control group of the oopif experiment on M55 beta ( [Renderer kill 139] content::BlobDispatcherHost::OnRegisterPublicBlobURL : e.g. http://go/crash/20424cbb00000000 )
,
Oct 21 2016
,
Oct 24 2016
Your change meets the bar and is auto-approved for M55 (branch: 2883)
,
Oct 24 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5485cc6c53dc648f27329fc12944a01b03737112 commit 5485cc6c53dc648f27329fc12944a01b03737112 Author: nick <nick@chromium.org> Date: Mon Oct 24 20:13:00 2016 BrowserProcessImpl: don't call IsIsolateExtensionsEnabled() too early This fixes a bug where IsIsolateExtensionsEnabled() had been called prior to the field trials actually being initialized, so the call returned true regardless of the finch experiment state, resulting in a possible misconfiguration of security state. PreCreateThreads() seems to be the earliest place in BrowserProcessImpl where we can do this registration. BUG= 657629 NOTRY=true NOPRESUBMIT=true TBR=creis@chromium.org Review-Url: https://chromiumcodereview.appspot.com/2438823003 Cr-Commit-Position: refs/heads/master@{#426852} (cherry picked from commit a0b7b87ea093cbb45f51c4ab1465b4e4115a5d99) Review-Url: https://codereview.chromium.org/2437213004 Cr-Commit-Position: refs/branch-heads/2883@{#255} Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768} [modify] https://crrev.com/5485cc6c53dc648f27329fc12944a01b03737112/chrome/browser/browser_process_impl.cc
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5485cc6c53dc648f27329fc12944a01b03737112 commit 5485cc6c53dc648f27329fc12944a01b03737112 Author: nick <nick@chromium.org> Date: Mon Oct 24 20:13:00 2016 BrowserProcessImpl: don't call IsIsolateExtensionsEnabled() too early This fixes a bug where IsIsolateExtensionsEnabled() had been called prior to the field trials actually being initialized, so the call returned true regardless of the finch experiment state, resulting in a possible misconfiguration of security state. PreCreateThreads() seems to be the earliest place in BrowserProcessImpl where we can do this registration. BUG= 657629 NOTRY=true NOPRESUBMIT=true TBR=creis@chromium.org Review-Url: https://chromiumcodereview.appspot.com/2438823003 Cr-Commit-Position: refs/heads/master@{#426852} (cherry picked from commit a0b7b87ea093cbb45f51c4ab1465b4e4115a5d99) Review-Url: https://codereview.chromium.org/2437213004 Cr-Commit-Position: refs/branch-heads/2883@{#255} Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768} [modify] https://crrev.com/5485cc6c53dc648f27329fc12944a01b03737112/chrome/browser/browser_process_impl.cc
,
Nov 4 2016
[Automated comment] removing mislabelled merge-merged-2840
,
Nov 4 2016
[Automated comment] removing mislabelled merge-merged-2840 |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by nick@chromium.org
, Oct 19 2016Status: Started (was: Assigned)