Commit 0a5f9449 authored by Devlin Cronin's avatar Devlin Cronin Committed by Commit Bot

[MD Extensions] Handle reload failures of unpacked extensions

If a user hits "reload" on an unpacked extension, there's a chance that
reload will fail (if the local files were modified in a way that causes
a load error). Once this happens, successive calls to reload() will
fail, because ExtensionService::ReloadExtension() doesn't play nicely
with extensions that failed to reload.

Adjust devleoperPrivate.reload() to return a LoadError (similar to
loadUnpacked()) if the reload fails. The extensions page now checks for
this result, and shows the load error dialog if the reload fails. The
load error dialog can then trigger a retry of this load, which goes
through loadUnpacked() rather than reload() (thus circumventing
weirdness with ExtensionService::ReloadExtension()). This is comparable
to how the current extensions page handles this scenario.

This is *not* the ideal solution, as a) we shouldn't be going through
loadUnpacked() for a loaded extension and b) the user can still get into
a broken state (where the extension cannot be enabled, nor can it be
reloaded) by dismissing the load error without fixing the error, but it
brings us at least to parity with the old page.

Bug: 789659

Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Ib0bd6eb4bfe74b1da588a9ea78e3396703fee973
Reviewed-on: https://chromium-review.googlesource.com/798599
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: default avatarKaran Bhatia <karandeepb@chromium.org>
Reviewed-by: default avatarDemetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522205}
parent d4c244a3
......@@ -189,6 +189,29 @@ std::unique_ptr<developer::ProfileInfo> CreateProfileInfo(Profile* profile) {
return info;
}
// Creates a developer::LoadError from the provided data.
developer::LoadError CreateLoadError(
const base::FilePath& file_path,
const std::string& error,
size_t line_number,
const std::string& manifest,
const DeveloperPrivateAPI::UnpackedRetryId& retry_guid) {
base::FilePath prettified_path = path_util::PrettifyPath(file_path);
SourceHighlighter highlighter(manifest, line_number);
developer::LoadError response;
response.error = error;
response.path = base::UTF16ToUTF8(prettified_path.LossyDisplayName());
response.retry_guid = retry_guid;
response.source = std::make_unique<developer::ErrorFileSource>();
response.source->before_highlight = highlighter.GetBeforeFeature();
response.source->highlight = highlighter.GetFeature();
response.source->after_highlight = highlighter.GetAfterFeature();
return response;
}
} // namespace
namespace ChoosePath = api::developer_private::ChoosePath;
......@@ -724,6 +747,8 @@ DeveloperPrivateUpdateExtensionConfigurationFunction::Run() {
return RespondNow(NoArguments());
}
DeveloperPrivateReloadFunction::DeveloperPrivateReloadFunction()
: registry_observer_(this), error_reporter_observer_(this) {}
DeveloperPrivateReloadFunction::~DeveloperPrivateReloadFunction() {}
ExtensionFunction::ResponseAction DeveloperPrivateReloadFunction::Run() {
......@@ -734,9 +759,19 @@ ExtensionFunction::ResponseAction DeveloperPrivateReloadFunction::Run() {
if (!extension)
return RespondNow(Error(kNoSuchExtensionError));
bool fail_quietly = params->options &&
params->options->fail_quietly &&
*params->options->fail_quietly;
reloading_extension_path_ = extension->path();
bool fail_quietly = false;
bool wait_for_completion = false;
if (params->options) {
fail_quietly =
params->options->fail_quietly && *params->options->fail_quietly;
// We only wait for completion for unpacked extensions, since they are the
// only extensions for which we can show actionable feedback to the user.
wait_for_completion = params->options->populate_error_for_unpacked &&
*params->options->populate_error_for_unpacked &&
Manifest::IsUnpackedLocation(extension->location());
}
ExtensionService* service = GetExtensionService(browser_context());
if (fail_quietly)
......@@ -744,9 +779,73 @@ ExtensionFunction::ResponseAction DeveloperPrivateReloadFunction::Run() {
else
service->ReloadExtension(params->extension_id);
// TODO(devlin): We shouldn't return until the extension has finished trying
// to reload (and then we could also return the error).
return RespondNow(NoArguments());
if (!wait_for_completion)
return RespondNow(NoArguments());
// Balanced in ClearObservers(), which is called from the first observer
// method to be called with the appropriate extension (or shutdown).
AddRef();
error_reporter_observer_.Add(ExtensionErrorReporter::GetInstance());
registry_observer_.Add(ExtensionRegistry::Get(browser_context()));
return RespondLater();
}
void DeveloperPrivateReloadFunction::OnExtensionLoaded(
content::BrowserContext* browser_context,
const Extension* extension) {
if (extension->path() == reloading_extension_path_) {
// Reload succeeded!
Respond(NoArguments());
ClearObservers();
}
}
void DeveloperPrivateReloadFunction::OnShutdown(ExtensionRegistry* registry) {
Respond(Error("Shutting down."));
ClearObservers();
}
void DeveloperPrivateReloadFunction::OnLoadFailure(
content::BrowserContext* browser_context,
const base::FilePath& file_path,
const std::string& error) {
if (file_path == reloading_extension_path_) {
// Reload failed - create an error to pass back to the extension.
ExtensionLoaderHandler::GetManifestError(
error, file_path,
// TODO(devlin): Update GetManifestError to take a OnceCallback.
base::BindRepeating(&DeveloperPrivateReloadFunction::OnGotManifestError,
this)); // Creates a reference.
ClearObservers();
}
}
void DeveloperPrivateReloadFunction::OnGotManifestError(
const base::FilePath& file_path,
const std::string& error,
size_t line_number,
const std::string& manifest) {
DeveloperPrivateAPI::UnpackedRetryId retry_guid =
DeveloperPrivateAPI::Get(browser_context())
->AddUnpackedPath(GetSenderWebContents(), reloading_extension_path_);
// Respond to the caller with the load error, which allows the caller to retry
// reloading through developerPrivate.loadUnpacked().
// TODO(devlin): This is weird. Really, we should allow retrying through this
// function instead of through loadUnpacked(), but
// ExtensionService::ReloadExtension doesn't behave well with an extension
// that failed to reload, and untangling that mess is quite significant.
// See https://crbug.com/792277.
Respond(OneArgument(
CreateLoadError(file_path, error, line_number, manifest, retry_guid)
.ToValue()));
}
void DeveloperPrivateReloadFunction::ClearObservers() {
registry_observer_.RemoveAll();
error_reporter_observer_.RemoveAll();
Release(); // Balanced in Run().
}
DeveloperPrivateShowPermissionsDialogFunction::
......@@ -865,20 +964,9 @@ void DeveloperPrivateLoadUnpackedFunction::OnGotManifestError(
size_t line_number,
const std::string& manifest) {
DCHECK(!retry_guid_.empty());
base::FilePath prettified_path = path_util::PrettifyPath(file_path);
SourceHighlighter highlighter(manifest, line_number);
developer::LoadError response;
response.error = error;
response.path = base::UTF16ToUTF8(prettified_path.LossyDisplayName());
response.retry_guid = retry_guid_;
response.source = base::MakeUnique<developer::ErrorFileSource>();
response.source->before_highlight = highlighter.GetBeforeFeature();
response.source->highlight = highlighter.GetFeature();
response.source->after_highlight = highlighter.GetAfterFeature();
Respond(OneArgument(response.ToValue()));
Respond(OneArgument(
CreateLoadError(file_path, error, line_number, manifest, retry_guid_)
.ToValue()));
}
bool DeveloperPrivateChooseEntryFunction::ShowPicker(
......
......@@ -15,6 +15,7 @@
#include "chrome/browser/extensions/api/developer_private/entry_picker.h"
#include "chrome/browser/extensions/chrome_extension_function.h"
#include "chrome/browser/extensions/error_console/error_console.h"
#include "chrome/browser/extensions/extension_error_reporter.h"
#include "chrome/browser/extensions/extension_management.h"
#include "chrome/browser/extensions/extension_uninstall_dialog.h"
#include "chrome/browser/extensions/pack_extension_job.h"
......@@ -353,16 +354,50 @@ class DeveloperPrivateUpdateExtensionConfigurationFunction
ResponseAction Run() override;
};
class DeveloperPrivateReloadFunction : public DeveloperPrivateAPIFunction {
class DeveloperPrivateReloadFunction : public DeveloperPrivateAPIFunction,
public ExtensionRegistryObserver,
public ExtensionErrorReporter::Observer {
public:
DECLARE_EXTENSION_FUNCTION("developerPrivate.reload",
DEVELOPERPRIVATE_RELOAD);
DeveloperPrivateReloadFunction();
// ExtensionRegistryObserver:
void OnExtensionLoaded(content::BrowserContext* browser_context,
const Extension* extension) override;
void OnShutdown(ExtensionRegistry* registry) override;
// ExtensionErrorReporter::Observer:
void OnLoadFailure(content::BrowserContext* browser_context,
const base::FilePath& file_path,
const std::string& error) override;
protected:
~DeveloperPrivateReloadFunction() override;
// ExtensionFunction:
ResponseAction Run() override;
private:
// Callback once we parse a manifest error from a failed reload.
void OnGotManifestError(const base::FilePath& file_path,
const std::string& error,
size_t line_number,
const std::string& manifest);
// Clears the scoped observers.
void ClearObservers();
// The file path of the extension that's reloading.
base::FilePath reloading_extension_path_;
ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>
registry_observer_;
ScopedObserver<ExtensionErrorReporter, ExtensionErrorReporter::Observer>
error_reporter_observer_;
DISALLOW_COPY_AND_ASSIGN(DeveloperPrivateReloadFunction);
};
class DeveloperPrivateShowPermissionsDialogFunction
......
......@@ -10,8 +10,10 @@
#include "base/files/file_util.h"
#include "base/macros.h"
#include "base/memory/ptr_util.h"
#include "base/scoped_observer.h"
#include "base/strings/utf_string_conversions.h"
#include "chrome/browser/chrome_notification_types.h"
#include "chrome/browser/extensions/chrome_test_extension_loader.h"
#include "chrome/browser/extensions/error_console/error_console.h"
#include "chrome/browser/extensions/extension_function_test_utils.h"
#include "chrome/browser/extensions/extension_service.h"
......@@ -40,6 +42,7 @@
#include "extensions/browser/extension_error_test_util.h"
#include "extensions/browser/extension_prefs.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/browser/extension_registry_observer.h"
#include "extensions/browser/extension_system.h"
#include "extensions/browser/extension_util.h"
#include "extensions/browser/mock_external_provider.h"
......@@ -736,6 +739,128 @@ TEST_F(DeveloperPrivateApiUnitTest, LoadUnpackedRetryId) {
}
}
// Tests calling "reload" on an unpacked extension with a manifest error,
// resulting in the reload failing. The reload call should then respond with
// the load error, which includes a retry GUID to be passed to loadUnpacked().
TEST_F(DeveloperPrivateApiUnitTest, ReloadBadExtensionToLoadUnpackedRetry) {
std::unique_ptr<content::WebContents> web_contents(
content::WebContentsTester::CreateTestWebContents(profile(), nullptr));
// A broken manifest (version's value should be a string).
constexpr const char kBadManifest[] =
R"({
"name": "foo",
"description": "bar",
"version": 1,
"manifest_version": 2
})";
constexpr const char kGoodManifest[] =
R"({
"name": "foo",
"description": "bar",
"version": "1",
"manifest_version": 2
})";
// Create a good unpacked extension.
TestExtensionDir dir;
dir.WriteManifest(kGoodManifest);
base::FilePath path = dir.UnpackedPath();
api::EntryPicker::SkipPickerAndAlwaysSelectPathForTest(&path);
scoped_refptr<const Extension> extension;
{
ChromeTestExtensionLoader loader(profile());
loader.set_pack_extension(false);
extension = loader.LoadExtension(path);
}
ASSERT_TRUE(extension);
const ExtensionId id = extension->id();
std::string reload_args = base::StringPrintf(
R"(["%s", {"failQuietly": true, "populateErrorForUnpacked":true}])",
id.c_str());
{
// Try reloading while the manifest is still good. This should succeed, and
// the extension should still be enabled. Additionally, the function should
// wait for the reload to complete, so we should see an unload and reload.
class UnloadedRegistryObserver : public ExtensionRegistryObserver {
public:
UnloadedRegistryObserver(const base::FilePath& expected_path,
ExtensionRegistry* registry)
: expected_path_(expected_path), observer_(this) {
observer_.Add(registry);
}
void OnExtensionUnloaded(content::BrowserContext* browser_context,
const Extension* extension,
UnloadedExtensionReason reason) override {
ASSERT_FALSE(saw_unload_);
saw_unload_ = extension->path() == expected_path_;
}
bool saw_unload() const { return saw_unload_; }
private:
bool saw_unload_ = false;
base::FilePath expected_path_;
ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver> observer_;
DISALLOW_COPY_AND_ASSIGN(UnloadedRegistryObserver);
};
UnloadedRegistryObserver unload_observer(path, registry());
auto function =
base::MakeRefCounted<api::DeveloperPrivateReloadFunction>();
function->SetRenderFrameHost(web_contents->GetMainFrame());
api_test_utils::RunFunction(function.get(), reload_args, profile());
// Note: no need to validate a saw_load()-type method because the presence
// in enabled_extensions() indicates the extension was loaded.
EXPECT_TRUE(unload_observer.saw_unload());
EXPECT_TRUE(registry()->enabled_extensions().Contains(id));
}
dir.WriteManifest(kBadManifest);
DeveloperPrivateAPI::UnpackedRetryId retry_guid;
{
// Trying to load the extension should result in a load error with the
// retry GUID populated.
auto function = base::MakeRefCounted<api::DeveloperPrivateReloadFunction>();
function->SetRenderFrameHost(web_contents->GetMainFrame());
std::unique_ptr<base::Value> result =
api_test_utils::RunFunctionAndReturnSingleResult(
function.get(), reload_args, profile());
ASSERT_TRUE(result);
std::unique_ptr<api::developer_private::LoadError> error =
api::developer_private::LoadError::FromValue(*result);
ASSERT_TRUE(error);
EXPECT_FALSE(error->retry_guid.empty());
retry_guid = error->retry_guid;
EXPECT_TRUE(registry()->disabled_extensions().Contains(id));
}
dir.WriteManifest(kGoodManifest);
{
// Try reloading the extension by supplying the retry id. It should succeed,
// and the extension should be enabled again.
auto function =
base::MakeRefCounted<api::DeveloperPrivateLoadUnpackedFunction>();
function->SetRenderFrameHost(web_contents->GetMainFrame());
TestExtensionRegistryObserver observer(registry());
std::string args =
base::StringPrintf(R"([{"failQuietly": true, "populateError": true,
"retryGuid": "%s"}])",
retry_guid.c_str());
api_test_utils::RunFunction(function.get(), args, profile());
const Extension* extension = observer.WaitForExtensionLoaded();
ASSERT_TRUE(extension);
EXPECT_EQ(extension->path(), path);
EXPECT_TRUE(registry()->enabled_extensions().Contains(id));
}
}
// Test developerPrivate.requestFileSource.
TEST_F(DeveloperPrivateApiUnitTest, DeveloperPrivateRequestFileSource) {
// Testing of this function seems light, but that's because it basically just
......
......@@ -44,7 +44,10 @@ cr.define('extensions', function() {
*/
inspectItemView(id, view) {}
/** @param {string} id */
/**
* @param {string} id
* @return {!Promise}
*/
reloadItem(id) {}
/** @param {string} id */
......@@ -164,7 +167,9 @@ cr.define('extensions', function() {
/** @private */
onReloadTap_: function() {
this.delegate.reloadItem(this.data.id);
this.delegate.reloadItem(this.data.id).catch(loadError => {
this.fire('load-error', loadError);
});
},
/** @private */
......
......@@ -79,7 +79,8 @@
delegate="[[delegate]]" in-dev-mode="[[inDevMode]]"
filter="[[filter]]" hidden$="[[!didInitPage_]]" slot="view"
apps="[[apps_]]" extensions="[[extensions_]]"
on-show-install-warnings="onShowInstallWarnings_">
on-show-install-warnings="onShowInstallWarnings_"
on-load-error="onLoadError_">
</extensions-item-list>
<template id="details-view" is="cr-lazy-render">
<extensions-detail-view delegate="[[delegate]]" slot="view"
......
......@@ -176,7 +176,18 @@ cr.define('extensions', function() {
/** @override */
reloadItem(id) {
chrome.developerPrivate.reload(id, {failQuietly: false});
return new Promise(function(resolve, reject) {
chrome.developerPrivate.reload(
id, {failQuietly: true, populateErrorForUnpacked: true},
(loadError) => {
if (loadError) {
reject(loadError);
return;
}
resolve();
});
});
}
/** @override */
......
......@@ -300,6 +300,11 @@ namespace developerPrivate {
// If false, an alert dialog will show in the event of a reload error.
// Defaults to false.
boolean? failQuietly;
// If true, populates a LoadError for the response rather than setting
// lastError. Only relevant for unpacked extensions; it will be ignored for
// any other extension.
boolean? populateErrorForUnpacked;
};
dictionary LoadUnpackedOptions {
......@@ -533,7 +538,7 @@ namespace developerPrivate {
// |options| : Additional configuration parameters.
static void reload(DOMString extensionId,
optional ReloadOptions options,
optional VoidCallback callback);
optional LoadErrorCallback callback);
// Modifies an extension's current configuration.
// |update| : The parameters for updating the extension's configuration.
......
......@@ -161,6 +161,10 @@ TEST_F('CrExtensionsItemsTest', 'ClickableItems', function() {
this.runMochaTest(extension_item_tests.TestNames.ClickableItems);
});
TEST_F('CrExtensionsItemsTest', 'FailedReloadFiresLoadError', function() {
this.runMochaTest(extension_item_tests.TestNames.FailedReloadFiresLoadError);
});
TEST_F('CrExtensionsItemsTest', 'Warnings', function() {
this.runMochaTest(extension_item_tests.TestNames.Warnings);
});
......
......@@ -69,6 +69,7 @@ cr.define('extension_item_tests', function() {
ElementVisibilityDeveloperState:
'element visibility: after enabling developer mode',
ClickableItems: 'clickable items',
FailedReloadFiresLoadError: 'failed reload fires load error',
Warnings: 'warnings',
SourceIndicator: 'source indicator',
EnableToggle: 'toggle is disabled when necessary',
......@@ -167,13 +168,55 @@ cr.define('extension_item_tests', function() {
item.set('data.state', chrome.developerPrivate.ExtensionState.TERMINATED);
Polymer.dom.flush();
mockDelegate.testClickingCalls(
item.$$('#terminated-reload-button'), 'reloadItem', [item.data.id]);
item.$$('#terminated-reload-button'), 'reloadItem', [item.data.id],
Promise.resolve());
item.set('data.location', chrome.developerPrivate.Location.UNPACKED);
item.set('data.state', chrome.developerPrivate.ExtensionState.ENABLED);
Polymer.dom.flush();
mockDelegate.testClickingCalls(
item.$$('#dev-reload-button'), 'reloadItem', [item.data.id]);
});
/** Tests that the reload button properly fires the load-error event. */
test(assert(TestNames.FailedReloadFiresLoadError), function() {
item.set('inDevMode', true);
item.set('data.location', chrome.developerPrivate.Location.UNPACKED);
Polymer.dom.flush();
extension_test_util.testVisible(item, '#dev-reload-button', true);
// Check clicking the reload button. The reload button should fire a
// load-error event if and only if the reload fails (indicated by a
// rejected promise).
// This is a bit of a pain to verify because the promises finish
// asynchronously, so we have to use setTimeout()s.
var firedLoadError = false;
item.addEventListener('load-error', () => { firedLoadError = true; });
// This is easier to test with a TestBrowserProxy-style delegate.
var proxyDelegate = new extensions.TestService();
item.delegate = proxyDelegate;
var verifyEventPromise = function(expectCalled) {
return new Promise((resolve, reject) => {
setTimeout(() => {
expectEquals(expectCalled, firedLoadError);
resolve();
});
});
};
MockInteractions.tap(item.$$('#dev-reload-button'));
return proxyDelegate.whenCalled('reloadItem').then(function(id) {
expectEquals(item.data.id, id);
return verifyEventPromise(false);
}).then(function() {
proxyDelegate.resetResolver('reloadItem');
proxyDelegate.setForceReloadItemError(true);
MockInteractions.tap(item.$$('#dev-reload-button'));
return proxyDelegate.whenCalled('reloadItem');
}).then(function(id) {
expectEquals(item.data.id, id);
return verifyEventPromise(true);
});
});
test(assert(TestNames.Warnings), function() {
......
......@@ -17,10 +17,13 @@ cr.define('extension_test_util', function() {
* @param {string} callName The function expected to be called.
* @param {Array<*>=} opt_expectedArgs The arguments the function is
* expected to be called with.
* @param {*=} opt_returnValue The value to return from the function call.
*/
testClickingCalls: function(element, callName, opt_expectedArgs) {
testClickingCalls: function(element, callName, opt_expectedArgs,
opt_returnValue) {
var mock = new MockController();
var mockMethod = mock.createFunctionMock(this, callName);
mockMethod.returnValue = opt_returnValue;
MockMethod.prototype.addExpectation.apply(
mockMethod, opt_expectedArgs);
MockInteractions.tap(element);
......@@ -117,6 +120,9 @@ cr.define('extension_test_util', function() {
/** @override */
inspectItemView: function(id, view) {},
/** @override */
reloadItem: function(id) {},
/** @override */
repairItem: function(id) {},
......
......@@ -11,6 +11,7 @@ cr.define('extensions', function() {
'getProfileConfiguration',
'loadUnpacked',
'retryLoadUnpacked',
'reloadItem',
'setProfileInDevMode',
'setShortcutHandlingSuspended',
'updateAllExtensions',
......@@ -22,6 +23,9 @@ cr.define('extensions', function() {
/** @private {!chrome.developerPrivate.LoadError} */
this.retryLoadUnpackedError_;
/** @type {boolean} */
this.forceReloadItemError_ = false;
}
/**
......@@ -31,6 +35,13 @@ cr.define('extensions', function() {
this.retryLoadUnpackedError_ = error;
}
/**
* @param {boolean} force
*/
setForceReloadItemError(force) {
this.forceReloadItemError_ = force;
}
/** @override */
getProfileConfiguration() {
this.methodCalled('getProfileConfiguration');
......@@ -70,6 +81,12 @@ cr.define('extensions', function() {
return Promise.resolve();
}
/** @override */
reloadItem(id) {
this.methodCalled('reloadItem', id);
return this.forceReloadItemError_ ? Promise.reject() : Promise.resolve();
}
/** @override */
retryLoadUnpacked(guid) {
this.methodCalled('retryLoadUnpacked', guid);
......
......@@ -414,7 +414,8 @@ chrome.developerPrivate.ExtensionCommandUpdate;
/**
* @typedef {{
* failQuietly: (boolean|undefined)
* failQuietly: (boolean|undefined),
* populateErrorForUnpacked: (boolean|undefined)
* }}
* @see https://developer.chrome.com/extensions/developerPrivate#type-ReloadOptions
*/
......@@ -643,7 +644,8 @@ chrome.developerPrivate.showPermissionsDialog = function(extensionId, callback)
* @param {string} extensionId The id of the extension to reload.
* @param {!chrome.developerPrivate.ReloadOptions=} options Additional
* configuration parameters.
* @param {function():void=} callback
* @param {function((!chrome.developerPrivate.LoadError|undefined)):void=}
* callback
* @see https://developer.chrome.com/extensions/developerPrivate#method-reload
*/
chrome.developerPrivate.reload = function(extensionId, options, callback) {};
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment