Fix content verifier problem with content scripts (reland)
The problem is that we were creating the ContentVerifier too late
in ExtensionSystemImpl::Shared::Init, after the script loader had
already tried grabbing a reference to it for use on another
thead. Therefore at run time, for all content scripts we were was
seeing a null pointer for the content verifier and skipping
verification of those scripts.
Note: this is a reland of https://codereview.chromium.org/1226163010/
which was reverted in b9e8b50e4c45d8aefacac58e28c50d83bb257b7b due
to test failures on the Vista Tests bot.
BUG=509130
Review URL: https://codereview.chromium.org/1250473002
Cr-Commit-Position: refs/heads/master@{#341435}
diff --git a/chrome/browser/extensions/content_verifier_browsertest.cc b/chrome/browser/extensions/content_verifier_browsertest.cc
index a7b9ac8..91c6629 100644
--- a/chrome/browser/extensions/content_verifier_browsertest.cc
+++ b/chrome/browser/extensions/content_verifier_browsertest.cc
@@ -2,10 +2,15 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include <list>
+#include <set>
+#include <string>
+
#include "base/scoped_observer.h"
#include "chrome/browser/extensions/extension_browsertest.h"
#include "chrome/common/chrome_switches.h"
#include "content/public/test/test_utils.h"
+#include "extensions/browser/content_verifier.h"
#include "extensions/browser/content_verify_job.h"
#include "extensions/browser/extension_prefs.h"
#include "extensions/browser/extension_registry.h"
@@ -90,14 +95,15 @@
JobObserver();
virtual ~JobObserver();
+ enum class Result { SUCCESS, FAILURE };
+
// Call this to add an expected job result.
void ExpectJobResult(const std::string& extension_id,
const base::FilePath& relative_path,
- bool expected_to_fail);
+ Result expected_result);
- // Wait to see expected jobs. Returns true if we saw all jobs finish as
- // expected, or false if any job completed with non-expected success/failure
- // status.
+ // Wait to see expected jobs. Returns true when we've seen all expected jobs
+ // finish, or false if there was an error or timeout.
bool WaitForExpectedJobs();
// ContentVerifyJob::TestObserver interface
@@ -109,32 +115,47 @@
bool failed) override;
private:
- typedef std::pair<std::string, base::FilePath> ExtensionFile;
- typedef std::map<ExtensionFile, bool> ExpectedJobs;
- ExpectedJobs expected_jobs_;
+ struct ExpectedResult {
+ public:
+ std::string extension_id;
+ base::FilePath path;
+ Result result;
+
+ ExpectedResult(const std::string& extension_id, const base::FilePath& path,
+ Result result) {
+ this->extension_id = extension_id;
+ this->path = path;
+ this->result = result;
+ }
+ };
+ std::list<ExpectedResult> expectations_;
+ content::BrowserThread::ID creation_thread_;
scoped_refptr<content::MessageLoopRunner> loop_runner_;
- bool saw_expected_job_results_;
};
void JobObserver::ExpectJobResult(const std::string& extension_id,
const base::FilePath& relative_path,
- bool expected_to_fail) {
- expected_jobs_.insert(std::make_pair(
- ExtensionFile(extension_id, relative_path), expected_to_fail));
+ Result expected_result) {
+ expectations_.push_back(ExpectedResult(
+ extension_id, relative_path, expected_result));
}
-JobObserver::JobObserver() : saw_expected_job_results_(false) {
+JobObserver::JobObserver() {
+ EXPECT_TRUE(
+ content::BrowserThread::GetCurrentThreadIdentifier(&creation_thread_));
}
JobObserver::~JobObserver() {
}
bool JobObserver::WaitForExpectedJobs() {
- if (!expected_jobs_.empty()) {
+ EXPECT_TRUE(content::BrowserThread::CurrentlyOn(creation_thread_));
+ if (!expectations_.empty()) {
loop_runner_ = new content::MessageLoopRunner();
loop_runner_->Run();
+ loop_runner_ = nullptr;
}
- return saw_expected_job_results_;
+ return expectations_.empty();
}
void JobObserver::JobStarted(const std::string& extension_id,
@@ -144,21 +165,75 @@
void JobObserver::JobFinished(const std::string& extension_id,
const base::FilePath& relative_path,
bool failed) {
- ExpectedJobs::iterator i = expected_jobs_.find(ExtensionFile(
- extension_id, relative_path.NormalizePathSeparatorsTo('/')));
- if (i != expected_jobs_.end()) {
- if (failed != i->second) {
- saw_expected_job_results_ = false;
- if (loop_runner_.get())
- loop_runner_->Quit();
- }
- expected_jobs_.erase(i);
- if (expected_jobs_.empty()) {
- saw_expected_job_results_ = true;
- if (loop_runner_.get())
- loop_runner_->Quit();
+ if (!content::BrowserThread::CurrentlyOn(creation_thread_)) {
+ content::BrowserThread::PostTask(
+ creation_thread_, FROM_HERE,
+ base::Bind(&JobObserver::JobFinished, base::Unretained(this),
+ extension_id, relative_path, failed));
+ return;
+ }
+ Result result = failed ? Result::FAILURE : Result::SUCCESS;
+ bool found = false;
+ for (std::list<ExpectedResult>::iterator i = expectations_.begin();
+ i != expectations_.end(); ++i) {
+ if (i->extension_id == extension_id && i->path == relative_path &&
+ i->result == result) {
+ found = true;
+ expectations_.erase(i);
+ break;
}
}
+ if (found) {
+ if (expectations_.empty() && loop_runner_.get())
+ loop_runner_->Quit();
+ } else {
+ LOG(WARNING) << "Ignoring unexpected JobFinished " << extension_id << "/"
+ << relative_path.value() << " failed:" << failed;
+ }
+}
+
+class VerifierObserver : public ContentVerifier::TestObserver {
+ public:
+ VerifierObserver();
+ virtual ~VerifierObserver();
+
+ const std::set<std::string>& completed_fetches() {
+ return completed_fetches_;
+ }
+
+ // Returns when we've seen OnFetchComplete for |extension_id|.
+ void WaitForFetchComplete(const std::string& extension_id);
+
+ // ContentVerifier::TestObserver
+ void OnFetchComplete(const std::string& extension_id, bool success) override;
+
+ private:
+ std::set<std::string> completed_fetches_;
+ std::string id_to_wait_for_;
+ scoped_refptr<content::MessageLoopRunner> loop_runner_;
+};
+
+VerifierObserver::VerifierObserver() {
+}
+
+VerifierObserver::~VerifierObserver() {
+}
+
+void VerifierObserver::WaitForFetchComplete(const std::string& extension_id) {
+ EXPECT_TRUE(id_to_wait_for_.empty());
+ EXPECT_EQ(loop_runner_.get(), nullptr);
+ id_to_wait_for_ = extension_id;
+ loop_runner_ = new content::MessageLoopRunner();
+ loop_runner_->Run();
+ id_to_wait_for_.clear();
+ loop_runner_ = nullptr;
+}
+
+void VerifierObserver::OnFetchComplete(const std::string& extension_id,
+ bool success) {
+ completed_fetches_.insert(extension_id);
+ if (extension_id == id_to_wait_for_)
+ loop_runner_->Quit();
}
} // namespace
@@ -175,12 +250,12 @@
switches::kExtensionContentVerificationEnforce);
}
- // Setup our unload observer and JobDelegate, and install a test extension.
void SetUpOnMainThread() override {
ExtensionBrowserTest::SetUpOnMainThread();
}
void TearDownOnMainThread() override {
+ ContentVerifier::SetObserverForTests(NULL);
ContentVerifyJob::SetDelegateForTests(NULL);
ContentVerifyJob::SetObserverForTests(NULL);
ExtensionBrowserTest::TearDownOnMainThread();
@@ -237,15 +312,19 @@
std::string id = "hoipipabpcoomfapcecilckodldhmpgl";
job_observer.ExpectJobResult(
- id, base::FilePath(FILE_PATH_LITERAL("background.js")), false);
+ id, base::FilePath(FILE_PATH_LITERAL("background.js")),
+ JobObserver::Result::SUCCESS);
+ job_observer.ExpectJobResult(id,
+ base::FilePath(FILE_PATH_LITERAL("page.html")),
+ JobObserver::Result::SUCCESS);
+ job_observer.ExpectJobResult(id, base::FilePath(FILE_PATH_LITERAL("page.js")),
+ JobObserver::Result::SUCCESS);
job_observer.ExpectJobResult(
- id, base::FilePath(FILE_PATH_LITERAL("page.html")), false);
- job_observer.ExpectJobResult(
- id, base::FilePath(FILE_PATH_LITERAL("page.js")), false);
- job_observer.ExpectJobResult(
- id, base::FilePath(FILE_PATH_LITERAL("dir/page2.html")), false);
- job_observer.ExpectJobResult(
- id, base::FilePath(FILE_PATH_LITERAL("page2.js")), false);
+ id, base::FilePath(FILE_PATH_LITERAL("dir/page2.html")),
+ JobObserver::Result::SUCCESS);
+ job_observer.ExpectJobResult(id,
+ base::FilePath(FILE_PATH_LITERAL("page2.js")),
+ JobObserver::Result::SUCCESS);
// Install a test extension we copied from the webstore that has actual
// signatures, and contains image paths with leading "./".
@@ -260,4 +339,52 @@
ContentVerifyJob::SetObserverForTests(NULL);
}
+IN_PROC_BROWSER_TEST_F(ContentVerifierTest, ContentScripts) {
+ VerifierObserver verifier_observer;
+ ContentVerifier::SetObserverForTests(&verifier_observer);
+
+ // Install an extension with content scripts. The initial read of the content
+ // scripts will fail verification because they are read before the content
+ // verification system has completed a one-time processing of the expected
+ // hashes. (The extension only contains the root level hashes of the merkle
+ // tree, but the content verification system builds the entire tree and
+ // caches it in the extension install directory - see ContentHashFetcher for
+ // more details).
+ std::string id = "jmllhlobpjcnnomjlipadejplhmheiif";
+ const Extension* extension = InstallExtensionFromWebstore(
+ test_data_dir_.AppendASCII("content_verifier/content_script.crx"), 1);
+ ASSERT_TRUE(extension);
+ ASSERT_EQ(extension->id(), id);
+
+ // Wait for the content verification code to finish processing the hashes.
+ if (!ContainsKey(verifier_observer.completed_fetches(), id))
+ verifier_observer.WaitForFetchComplete(id);
+
+ // Now disable the extension, since content scripts are read at enable time,
+ // set up our job observer, and re-enable, expecting a success this time.
+ DisableExtension(id);
+ JobObserver job_observer;
+ ContentVerifyJob::SetObserverForTests(&job_observer);
+ job_observer.ExpectJobResult(id,
+ base::FilePath(FILE_PATH_LITERAL("script.js")),
+ JobObserver::Result::SUCCESS);
+ EnableExtension(id);
+ EXPECT_TRUE(job_observer.WaitForExpectedJobs());
+
+ // Now alter the contents of the content script, reload the extension, and
+ // expect to see a job failure due to the content script content hash not
+ // being what was signed by the webstore.
+ base::FilePath scriptfile = extension->path().AppendASCII("script.js");
+ std::string extra = "some_extra_function_call();";
+ ASSERT_TRUE(base::AppendToFile(scriptfile, extra.data(), extra.size()));
+ DisableExtension(id);
+ job_observer.ExpectJobResult(id,
+ base::FilePath(FILE_PATH_LITERAL("script.js")),
+ JobObserver::Result::FAILURE);
+ EnableExtension(id);
+ EXPECT_TRUE(job_observer.WaitForExpectedJobs());
+
+ ContentVerifyJob::SetObserverForTests(NULL);
+}
+
} // namespace extensions
