Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
510943003:
Register granting of previously withheld permissions as a permissions (Closed)
DescriptionRegister granting of previously withheld permissions as a permissions
increase
BUG=408257
Committed: https://crrev.com/48f08c4fbc8ea99059ceee676be2327b01fdd080
Cr-Commit-Position: refs/heads/master@{#295578}
Patch Set 1 #Patch Set 2 : Add initialization calls before granting permissions #
Total comments: 3
Patch Set 3 : Moved Init calls #Patch Set 4 : Resolve threading issues, add crx installer browser test #
Total comments: 9
Patch Set 5 : Move init call, test updates #
Total comments: 4
Patch Set 6 : Test updates #Patch Set 7 : Readded init call for unpacked installer #
Messages
Total messages: 31 (3 generated)
gpdavis.chromium@gmail.com changed reviewers: + kalman@chromium.org
Here are my findings. Hope this isn't too much information :) With the user consent flag enabled, I installed LinkClump and observed the log output (I used a conditional to only print permissions for this extension to make life easier-- that's why I perform this check in the patchset). Withheld, granted, and active permissions were checked before and after InitializePermissions was called in CheckPermissionsIncrease. Before initialize: Withheld was empty Granted contained all host permissions Active also contained all host permissions After initialize: Withheld contained all host permissions Granted contained all host permissions Active was empty So to answer your question from the other CL, yes, it looks like the all-host permissions that should get withheld are first added to both active and granted permissions. Then InitializePermissions does the segregation, stripping all-host permissions from active permissions, but not granted. Note that CheckPermissionsIncrease compares granted and active permissions after initialization to check for elevation. The natural next question is: what happens when the flag is disabled and the browser is restarted? Before initialize: Withheld was empty Granted contained all host permissions Active also contained all host permissions After initialize: Withheld was empty Granted contained all host permissions Active also contained all host permissions So, as expected, InitializePermissions didn't withhold anything, so granted and active permissions are the same before and after initialization. This is why we aren't getting a triggered elevation increase warning. Active permissions have indeed changed when the browser was restarted, but the granted permissions contained all-host all along, and that's the permission set that's used as the "old permissions" to check against for any increase. Lemme know if I can clarify anything. It looks like the permissions updater was set up specifically not to trigger a permissions elevation. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
That's a lotta logging :) Ok, sounds like we need to withhold before granting.
On 2014/08/27 20:32:00, kalman wrote: > That's a lotta logging :) > > Ok, sounds like we need to withhold before granting. Yep, that did it. Now withheld permissions do not show up in granted permissions. What do you think?
https://codereview.chromium.org/510943003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/crx_installer.cc (right): https://codereview.chromium.org/510943003/diff/20001/chrome/browser/extension... chrome/browser/extensions/crx_installer.cc:806: perms_updater.InitializePermissions(extension()); Can we do this when extension() gets created? https://codereview.chromium.org/510943003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/unpacked_installer.cc (right): https://codereview.chromium.org/510943003/diff/20001/chrome/browser/extension... chrome/browser/extensions/unpacked_installer.cc:312: perms_updater.InitializePermissions(extension()); Ditto.
https://codereview.chromium.org/510943003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/crx_installer.cc (right): https://codereview.chromium.org/510943003/diff/20001/chrome/browser/extension... chrome/browser/extensions/crx_installer.cc:806: perms_updater.InitializePermissions(extension()); On 2014/08/28 03:18:46, kalman wrote: > Can we do this when extension() gets created? Looks like this gets created in sandboxed_unpacker.cc (or in a couple of other places within this class for special situations), and there doesn't appear to be any way to get a profile to pass to the permissions updater. However, the extension gets passed back into this class through OnUnpackSuccess, and then set_extension is called on install_checker_ (extension() returns install_checker_.extension().get()), so we could initialize the permissions there. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... OnUnpackSuccess seems to be the common denominator for all the cases of extension creation here. Extensions created by user scripts get passed in here as well. So I feel like if we want to minimize Initialize calls, we should either do it here or in OnUnpackSuccess. Thoughts?
OnUnpackSuccess sounds right.
kalman@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Looks pretty good overall, but we need a couple of tests for this.
On 2014/09/02 20:48:28, Devlin wrote: > Looks pretty good overall, but we need a couple of tests for this. I've been working on a browser test for this, but I'm struggling with threading issues. InitializePermissions accesses prefs, and the call is made from the wrong thread. Any advice for how to work around this? I'm trying to post the task to the proper thread, but it's not working out too well, and I'm starting to feel like I'm on the wrong track.
On 2014/09/05 01:56:56, gpdavis wrote: > On 2014/09/02 20:48:28, Devlin wrote: > > Looks pretty good overall, but we need a couple of tests for this. > > I've been working on a browser test for this, but I'm struggling with threading > issues. InitializePermissions accesses prefs, and the call is made from the > wrong thread. Any advice for how to work around this? I'm trying to post the > task to the proper thread, but it's not working out too well, and I'm starting > to feel like I'm on the wrong track. Hey, sorry for the delay. The CL title was similar enough to the install prompt one that I didn't realize they were different issues. :/ So, it looks like you're calling InitalizePermissions from the file thread in the install process, when you need to call it from the UI thread. You'll need to move them to be on the UI thread (there should be DCHECKs for threads spattered around unpacked/crx installer so you can tell which function runs where).
Patchset #4 (id:60001) has been deleted
On 2014/09/08 16:54:16, Devlin wrote: > On 2014/09/05 01:56:56, gpdavis wrote: > > On 2014/09/02 20:48:28, Devlin wrote: > > > Looks pretty good overall, but we need a couple of tests for this. > > > > I've been working on a browser test for this, but I'm struggling with > threading > > issues. InitializePermissions accesses prefs, and the call is made from the > > wrong thread. Any advice for how to work around this? I'm trying to post the > > task to the proper thread, but it's not working out too well, and I'm starting > > to feel like I'm on the wrong track. > > Hey, sorry for the delay. The CL title was similar enough to the install prompt > one that I didn't realize they were different issues. :/ > > So, it looks like you're calling InitalizePermissions from the file thread in > the install process, when you need to call it from the UI thread. You'll need > to move them to be on the UI thread (there should be DCHECKs for threads > spattered around unpacked/crx installer so you can tell which function runs > where). Not a problem (: I finally figured out the threading issues. I also created a browser test for the crx installer. Not really sure where to add a test for the unpacked installer, so I figured I'd show you what I've got to see if it's reasonable. Should this be tested elsewhere? FYI: withheld.crx is a stripped down manifest that just requests all host permissions.
https://codereview.chromium.org/510943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/crx_installer.cc (right): https://codereview.chromium.org/510943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/crx_installer.cc:465: base::Bind(&CrxInstaller::InitializeExtensionPermissions, this))) Ew. We already bounce around a ton in extension installation, but I'd love to avoid any extra. Can we instead move the permissions update to OnInstallChecksComplete()? InstallChecker doesn't check anything with permissions, and, if you're worried, we can put in a comment mentioning that the permissions in the extension in installchecker might not reflect the total permissions. https://codereview.chromium.org/510943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/crx_installer_browsertest.cc (right): https://codereview.chromium.org/510943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/crx_installer_browsertest.cc:568: // Enable consent flag and install extension. All host permissions will be This is actually wrong. It's not that "All host permissions will be withheld", but rather that "the <all_hosts> permission will be withheld". There's a pretty sizable difference there. https://codereview.chromium.org/510943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/crx_installer_browsertest.cc:574: base::FilePath crx_path = test_data_dir_.AppendASCII("withheld.crx"); If we can, let's avoid introducing more crxs into the test data dir. They're not human-readable, so it makes the tests a bit harder to read (even if you comment). Instead, let's go ahead and use a combination of TestExtensionDir and ExtensionBrowserTest::PackExtension(). https://codereview.chromium.org/510943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/crx_installer_browsertest.cc:581: EXPECT_TRUE(service->GetExtensionById(id, false)); This really and truly and honestly is fully deprecated. I made sure when you told me last time there wasn't a comment. :) https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
https://codereview.chromium.org/510943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/crx_installer.cc (right): https://codereview.chromium.org/510943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/crx_installer.cc:465: base::Bind(&CrxInstaller::InitializeExtensionPermissions, this))) On 2014/09/09 16:04:26, Devlin wrote: > Ew. We already bounce around a ton in extension installation, but I'd love to > avoid any extra. Can we instead move the permissions update to > OnInstallChecksComplete()? InstallChecker doesn't check anything with > permissions, and, if you're worried, we can put in a comment mentioning that the > permissions in the extension in installchecker might not reflect the total > permissions. Well, the goal here is to call InitializePermissions before IsPrivilegeIncrease is called in AllowInstall, because that's what's catching the permissions elevation. This is why I was having threading trouble; the timing is quite sensitive. If we move it to OnInstallChecksComplete, we'd have to make another call to IsPrivilegeIncrease (or move the call) to catch it with the appropriate permissions, which is certainly possible, but I'm not sure if it would have any unintended consequences. I certainly agree that the thread jumping is ugly, though. It just seemed like the least impactful change. What do you think? https://codereview.chromium.org/510943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/crx_installer_browsertest.cc (right): https://codereview.chromium.org/510943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/crx_installer_browsertest.cc:568: // Enable consent flag and install extension. All host permissions will be On 2014/09/09 16:04:26, Devlin wrote: > This is actually wrong. It's not that "All host permissions will be withheld", > but rather that "the <all_hosts> permission will be withheld". There's a pretty > sizable difference there. Ah, understood. Will change. https://codereview.chromium.org/510943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/crx_installer_browsertest.cc:574: base::FilePath crx_path = test_data_dir_.AppendASCII("withheld.crx"); On 2014/09/09 16:04:26, Devlin wrote: > If we can, let's avoid introducing more crxs into the test data dir. They're > not human-readable, so it makes the tests a bit harder to read (even if you > comment). Instead, let's go ahead and use a combination of TestExtensionDir and > ExtensionBrowserTest::PackExtension(). Sure thing. https://codereview.chromium.org/510943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/crx_installer_browsertest.cc:581: EXPECT_TRUE(service->GetExtensionById(id, false)); On 2014/09/09 16:04:26, Devlin wrote: > This really and truly and honestly is fully deprecated. I made sure when you > told me last time there wasn't a comment. :) > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... Ah, sorry about that. I borrowed code from another test in the file, and didn't notice that it used that deprecated method. I'm gonna go ahead and switch the rest of them in the file over.
https://codereview.chromium.org/510943003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/crx_installer.cc (right): https://codereview.chromium.org/510943003/diff/80001/chrome/browser/extension... chrome/browser/extensions/crx_installer.cc:465: base::Bind(&CrxInstaller::InitializeExtensionPermissions, this))) On 2014/09/09 17:48:53, gpdavis wrote: > On 2014/09/09 16:04:26, Devlin wrote: > > Ew. We already bounce around a ton in extension installation, but I'd love to > > avoid any extra. Can we instead move the permissions update to > > OnInstallChecksComplete()? InstallChecker doesn't check anything with > > permissions, and, if you're worried, we can put in a comment mentioning that > the > > permissions in the extension in installchecker might not reflect the total > > permissions. > > Well, the goal here is to call InitializePermissions before IsPrivilegeIncrease > is called in AllowInstall, because that's what's catching the permissions > elevation. This is why I was having threading trouble; the timing is quite > sensitive. If we move it to OnInstallChecksComplete, we'd have to make another > call to IsPrivilegeIncrease (or move the call) to catch it with the appropriate > permissions, which is certainly possible, but I'm not sure if it would have any > unintended consequences. > > I certainly agree that the thread jumping is ugly, though. It just seemed like > the least impactful change. What do you think? Hmm... that makes me wonder if this is actually the right place for this. The goal is to have this trigger a permissions increase via ExtensionService::CheckPermissionsIncrease(). This actually looks like all it will do is not install the extension (and throw an error saying the manifest is invalid). That's not quite right, right?
On 2014/09/09 17:57:45, Devlin wrote: > https://codereview.chromium.org/510943003/diff/80001/chrome/browser/extension... > File chrome/browser/extensions/crx_installer.cc (right): > > https://codereview.chromium.org/510943003/diff/80001/chrome/browser/extension... > chrome/browser/extensions/crx_installer.cc:465: > base::Bind(&CrxInstaller::InitializeExtensionPermissions, this))) > On 2014/09/09 17:48:53, gpdavis wrote: > > On 2014/09/09 16:04:26, Devlin wrote: > > > Ew. We already bounce around a ton in extension installation, but I'd love > to > > > avoid any extra. Can we instead move the permissions update to > > > OnInstallChecksComplete()? InstallChecker doesn't check anything with > > > permissions, and, if you're worried, we can put in a comment mentioning that > > the > > > permissions in the extension in installchecker might not reflect the total > > > permissions. > > > > Well, the goal here is to call InitializePermissions before > IsPrivilegeIncrease > > is called in AllowInstall, because that's what's catching the permissions > > elevation. This is why I was having threading trouble; the timing is quite > > sensitive. If we move it to OnInstallChecksComplete, we'd have to make > another > > call to IsPrivilegeIncrease (or move the call) to catch it with the > appropriate > > permissions, which is certainly possible, but I'm not sure if it would have > any > > unintended consequences. > > > > I certainly agree that the thread jumping is ugly, though. It just seemed > like > > the least impactful change. What do you think? > > Hmm... that makes me wonder if this is actually the right place for this. > > The goal is to have this trigger a permissions increase via > ExtensionService::CheckPermissionsIncrease(). This actually looks like all it > will do is not install the extension (and throw an error saying the manifest is > invalid). That's not quite right, right? ExtensionService::CheckPermissionsIncrease() is a private method. Do you mean that the goal is to set things up such that when it's called in AddExtension, it'll trigger the increase, or that we should be making a separate call to it? If you look back at the changes to crx_installer.cc in patch set 2, I initialize the permissions just before granting active permissions. This is where the root of the problem lies; when we check for an increase, we've granted the active permissions before they've been segregated via InitializePermissions, so it's not registered as a permissions increase. Testing with that change in patch set 2, the extension does indeed silently get disabled. But I tested with the latest changes (with the thread jumping), and it turns out it didn't even work, despite passing the browser test that indicated that the extension got disabled. Curious... I think I prefer InitializePermissions to be called immediately before GrantActivePermissions with a comment that indicates that this must be done in order for CheckPermissionsIncrease to pick up on the fact that previously withheld permissions are now being granted. But we still have the issue where it's being disabled silently. I'll look into that. I do want your feedback on this, though. I tried calling InitializePermissions in OnInstallChecksComplete as you suggested, and that didn't work.
The goal is to get the Extension disabled in the normal flow as a Permissions increase. This should not mean calling into ExtensionService anywhere; it should happen naturally as part of the extension initialization process. To confirm, this works in PS2? (I have no problem initializing permissions there, and almost thought it was cleaner - but I understand Kalman's desire to do it as soon as the extension is created.) Re the test, I'm not surprised it passed - there are a few things that aren't quite right. For instance, we probably shouldn't be calling "InstallExtension" since the extension is already installed, and we should explicitly check that it's disabled (vs not being present in the enabled).
On 2014/09/09 19:09:07, Devlin wrote: > The goal is to get the Extension disabled in the normal flow as a Permissions > increase. This should not mean calling into ExtensionService anywhere; it should > happen naturally as part of the extension initialization process. Okay, cool, just wanted to make sure we weren't intending on adding explicit calls. > To confirm, > this works in PS2? (I have no problem initializing permissions there, and > almost thought it was cleaner - but I understand Kalman's desire to do it as > soon as the extension is created.) It works as in it triggers the permissions increase and disables the extension. There's no notification about it, though, so it's just silently disabled. > Re the test, I'm not surprised it passed - there are a few things that aren't > quite right. For instance, we probably shouldn't be calling "InstallExtension" > since the extension is already installed, and we should explicitly check that > it's disabled (vs not being present in the enabled). Yeah, I also noticed a bit of weirdness with the test as well. But InstallExtension expects an int that corresponds to expected behavior: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... So I passed in a -1 after disabling the flag to indicate that the upgrade is supposed to fail, and it behaved as expected. But I'm still not too sure why it didn't work in a manual test. It seems like the way I had it before (jumping between threads) should have still initialized the permissions before granting active permissions, which would mean it should work the same way. Maybe there's some asynch issue I'm not noticing? In any case, I do think initializing right before granting makes the most sense, and it does trigger the desired behavior (save for not notifying the user). Any ideas on why that might not be working the way we want it to? BTW, the withheld browser test passes with the init call as is in patchset 2.
On 2014/09/09 19:18:24, gpdavis wrote: > On 2014/09/09 19:09:07, Devlin wrote: > > The goal is to get the Extension disabled in the normal flow as a Permissions > > increase. This should not mean calling into ExtensionService anywhere; it > should > > happen naturally as part of the extension initialization process. > > Okay, cool, just wanted to make sure we weren't intending on adding explicit > calls. > > > To confirm, > > this works in PS2? (I have no problem initializing permissions there, and > > almost thought it was cleaner - but I understand Kalman's desire to do it as > > soon as the extension is created.) > > It works as in it triggers the permissions increase and disables the extension. > There's no notification about it, though, so it's just silently disabled. > > > Re the test, I'm not surprised it passed - there are a few things that aren't > > quite right. For instance, we probably shouldn't be calling > "InstallExtension" > > since the extension is already installed, and we should explicitly check that > > it's disabled (vs not being present in the enabled). > > Yeah, I also noticed a bit of weirdness with the test as well. But > InstallExtension expects an int that corresponds to expected behavior: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > So I passed in a -1 after disabling the flag to indicate that the upgrade is > supposed to fail, and it behaved as expected. But I'm still not too sure why it > didn't work in a manual test. It seems like the way I had it before (jumping > between threads) should have still initialized the permissions before granting > active permissions, which would mean it should work the same way. Maybe there's > some asynch issue I'm not noticing? > > In any case, I do think initializing right before granting makes the most sense, > and it does trigger the desired behavior (save for not notifying the user). Any > ideas on why that might not be working the way we want it to? > > BTW, the withheld browser test passes with the init call as is in patchset 2. Cool, let's initialize where you had it in PS2, then. Re why it's not prompting the user, I don't know. You could probably put in a few well-placed logs and find out, though. The code in question happens right after the Extension is added, here. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... We would expect that the extension is disabled with the appropriate reason (DISABLED_PERMISSION_INCREASE) and then to add an error, and show the user. It'd be great to know why it's not.
On 2014/09/09 19:34:01, Devlin wrote: > On 2014/09/09 19:18:24, gpdavis wrote: > > On 2014/09/09 19:09:07, Devlin wrote: > > > The goal is to get the Extension disabled in the normal flow as a > Permissions > > > increase. This should not mean calling into ExtensionService anywhere; it > > should > > > happen naturally as part of the extension initialization process. > > > > Okay, cool, just wanted to make sure we weren't intending on adding explicit > > calls. > > > > > To confirm, > > > this works in PS2? (I have no problem initializing permissions there, and > > > almost thought it was cleaner - but I understand Kalman's desire to do it as > > > soon as the extension is created.) > > > > It works as in it triggers the permissions increase and disables the > extension. > > There's no notification about it, though, so it's just silently disabled. > > > > > Re the test, I'm not surprised it passed - there are a few things that > aren't > > > quite right. For instance, we probably shouldn't be calling > > "InstallExtension" > > > since the extension is already installed, and we should explicitly check > that > > > it's disabled (vs not being present in the enabled). > > > > Yeah, I also noticed a bit of weirdness with the test as well. But > > InstallExtension expects an int that corresponds to expected behavior: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > > > So I passed in a -1 after disabling the flag to indicate that the upgrade is > > supposed to fail, and it behaved as expected. But I'm still not too sure why > it > > didn't work in a manual test. It seems like the way I had it before (jumping > > between threads) should have still initialized the permissions before granting > > active permissions, which would mean it should work the same way. Maybe > there's > > some asynch issue I'm not noticing? > > > > In any case, I do think initializing right before granting makes the most > sense, > > and it does trigger the desired behavior (save for not notifying the user). > Any > > ideas on why that might not be working the way we want it to? > > > > BTW, the withheld browser test passes with the init call as is in patchset 2. > > Cool, let's initialize where you had it in PS2, then. > > Re why it's not prompting the user, I don't know. You could probably put in a > few well-placed logs and find out, though. The code in question happens right > after the Extension is added, here. > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > We would expect that the extension is disabled with the appropriate reason > (DISABLED_PERMISSION_INCREASE) and then to add an error, and show the user. > It'd be great to know why it's not. Ah, turns out it is pushing global errors. I just wasn't seeing them because I'm getting a higher priority error about API keys when I start up chromium. But I see them now in the menu on the top right (it's orange to indicate those errors). So it looks like it does in fact work for the CRX installer, but it isn't working for the unpacked installer. I just want to clarify that the same behavior should be seen with the unpacked installer. Here's what's happening: I install an unpacked extension with <all_hosts> privilege with the consent flag enabled. Disable the flag, restart browser. IsPrivilegeIncrease doesn't get called at all, and it's because the extension CanSilentlyIncreasePermissions: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... Should this be changed, or should unpacked extensions be able to bypass this?
https://codereview.chromium.org/510943003/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/crx_installer_browsertest.cc (right): https://codereview.chromium.org/510943003/diff/100001/chrome/browser/extensio... chrome/browser/extensions/crx_installer_browsertest.cc:591: const extensions::Extension* extension = InstallExtension(crx_path, 1); nit: no prefix. https://codereview.chromium.org/510943003/diff/100001/chrome/browser/extensio... chrome/browser/extensions/crx_installer_browsertest.cc:602: extension = InstallExtension(crx_path, -1); This part still isn't quite right. Ideally, what we'd test is restarting the browser with the switch off - but those types of tests tend to be a bit flaky. Instead, let's go ahead and use ExtensionService::ReloadExtensionsForTest() - that's a pretty close approximation. Also, we shouldn't just check that it's not in enabled extensions - that could mean that it doesn't exist in the system at all (which is wrong). We should check a few things here, to be extra thorough: - The extension should not be in enabled_extensions() (the check you have) - The extension *should* be in disabled_extensions() - The extension should have a disable reason of Extension::DISABLE_PERMISSIONS_INCREASE.
Re unpacked extensions - it's fine for an unpacked extension to increase its permissions, so don't worry about that.
Re: unpacked extensions, Sounds good; it seemed like it wouldn't be an issue as I looked into how unpacked extensions are treated, but I wanted to make sure. I'll go ahead and remove the changes to that file in the next patch set. https://codereview.chromium.org/510943003/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/crx_installer_browsertest.cc (right): https://codereview.chromium.org/510943003/diff/100001/chrome/browser/extensio... chrome/browser/extensions/crx_installer_browsertest.cc:591: const extensions::Extension* extension = InstallExtension(crx_path, 1); On 2014/09/10 21:52:02, Devlin wrote: > nit: no prefix. Done (will arrive in next patch set). https://codereview.chromium.org/510943003/diff/100001/chrome/browser/extensio... chrome/browser/extensions/crx_installer_browsertest.cc:602: extension = InstallExtension(crx_path, -1); On 2014/09/10 21:52:02, Devlin wrote: > This part still isn't quite right. Ideally, what we'd test is restarting the > browser with the switch off - but those types of tests tend to be a bit flaky. > Instead, let's go ahead and use ExtensionService::ReloadExtensionsForTest() - > that's a pretty close approximation. > > Also, we shouldn't just check that it's not in enabled extensions - that could > mean that it doesn't exist in the system at all (which is wrong). We should > check a few things here, to be extra thorough: > - The extension should not be in enabled_extensions() (the check you have) > - The extension *should* be in disabled_extensions() > - The extension should have a disable reason of > Extension::DISABLE_PERMISSIONS_INCREASE. ReloadExtensionsForTest is problematic in the browser test environment. It ends up calling ExtensionUpdater::Start(), which has already been called when ExtensionSystem initializes for the browser test, and ExtensionUpdater::Stop() never gets called inbetween. So we end up in a situation where Start is called twice, and that fails a DCHECK. There seems to be no simple way to get around this, unfortunately. I had tried this before, but didn't dig as deeply into it since I found I could use InstallExtension instead. Using InstallExtension as is in this patch set, it passes all three of the checks you just outlined. Is there any better way to simulate a browser restart that doesn't use ReloadExtensionsForTest? Or is there something I'm missing about the lifetimes of these objects in the browser test environment?
Changed test to check for the things you outlined in the last set of comments. Hopefully it's okay as-is without switching over to ReloadExtensionsForTest, which I ran into issues with as outlined above. Tomorrow is my last day, so I won't be able to continue working on this. I'm hoping to have it ready before I leave, so PTAL :) Thanks!
On 2014/09/17 21:51:42, gpdavis wrote: > Changed test to check for the things you outlined in the last set of comments. > Hopefully it's okay as-is without switching over to ReloadExtensionsForTest, > which I ran into issues with as outlined above. > > Tomorrow is my last day, so I won't be able to continue working on this. I'm > hoping to have it ready before I leave, so PTAL :) > > Thanks! Don't know how the last comment slipped through - sorry for the delay. This LGTM, modulo one thing: We should still have the UnpackedInstaller update the permissions (even if it won't disable the extension). This way we're consistent and doing the right thing (just in case permissions behavior for unpacked extensions changes).
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/510943003/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as 5b324dfa5c3d2bbd63dbd40c0f7555df1e05064b
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/48f08c4fbc8ea99059ceee676be2327b01fdd080 Cr-Commit-Position: refs/heads/master@{#295578} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
