From c8965289996843e686084e86036db4cfe5b1713f Mon Sep 17 00:00:00 2001 From: "finnur@chromium.org" Date: Mon, 29 Nov 2010 22:19:18 +0000 Subject: EnumerateModules: Address UI review comments. Make it a simple list with columns. Sort by status (conflicts at the top), then by location and module name. Also, convert the type of DLL to an enum so we can dedup the list (and for non-loaded DLLs show that they are not loaded yet, but are of type: Shell Extension / WinSock. Make sure lower bound version specified on the blacklist is inclusive (first version that broke) and the higher bound version is exclusive (first version that worked). This allows us to pointpoint exactly when the failure started and when the fix was introduced, instead of doing 0.9999 shenanigans. Specify an upper bound for the idmmbc.dll conflict, since Henry's outreach produced a fix in version 6.03 of the download manager. BUG=51105 TEST=Unit test Review URL: http://codereview.chromium.org/5278012 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@67598 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/app/generated_resources.grd | 18 ++-- .../enumerate_modules_model_unittest_win.cc | 12 ++- chrome/browser/enumerate_modules_model_win.cc | 55 +++++----- chrome/browser/enumerate_modules_model_win.h | 10 +- chrome/browser/resources/about_conflicts.html | 111 ++++++++++----------- 5 files changed, 111 insertions(+), 95 deletions(-) (limited to 'chrome') diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd index 7c3a6b2..c55248e 100644 --- a/chrome/app/generated_resources.grd +++ b/chrome/app/generated_resources.grd @@ -4217,10 +4217,7 @@ Keep your key file in a safe place. You will need it to create new versions of y Learn more - Warning: - - - Version $11.2.3.4. + Warning We are currently investigating this issue. @@ -4244,19 +4241,22 @@ Keep your key file in a safe place. You will need it to create new versions of y Unable to detect any modules loaded. - Software: + Software - Signed by: + Signed by - Location: + Location - Warning: + Warning - Help tip: + Help tip + + + Not loaded yet http://www.google.com/support/chrome/bin/answer.py?answer=666386&hl=[GRITLANGCODE]&n=$1Hash&l=$2Hash&d=$3Hash&s=$4Hash diff --git a/chrome/browser/enumerate_modules_model_unittest_win.cc b/chrome/browser/enumerate_modules_model_unittest_win.cc index 374ec42..841c420 100644 --- a/chrome/browser/enumerate_modules_model_unittest_win.cc +++ b/chrome/browser/enumerate_modules_model_unittest_win.cc @@ -160,9 +160,19 @@ const struct MatchingEntryList { kStandardModule, { kMatchName, kMatchLocation, kMatchSignature, kEmpty, kVersionHigh, ModuleEnumerator::SEE_LINK } + }, { // Matches: Name, Location, Version lower is inclusive => Confirmed. + ModuleEnumerator::CONFIRMED_BAD, + kStandardModule, + { kMatchName, kMatchLocation, kMatchSignature, + "1.0", "2.0", ModuleEnumerator::SEE_LINK } + }, { // Matches: Name, Location, Version higher is exclusive => No match. + ModuleEnumerator::NOT_MATCHED, + kStandardModule, + { kMatchName, kMatchLocation, kEmpty, + "0.0", "1.0", ModuleEnumerator::SEE_LINK } }, { // All empty fields doesn't produce a match. ModuleEnumerator::NOT_MATCHED, - {kType, kStatus, L"", L"", L"", L"", L""}, + { kType, kStatus, L"", L"", L"", L"", L""}, { "a.dll", "", "", "", "", ModuleEnumerator::SEE_LINK } }, }; diff --git a/chrome/browser/enumerate_modules_model_win.cc b/chrome/browser/enumerate_modules_model_win.cc index 26b0469..f4ed945 100644 --- a/chrome/browser/enumerate_modules_model_win.cc +++ b/chrome/browser/enumerate_modules_model_win.cc @@ -37,12 +37,13 @@ static const int kModuleCheckDelayMs = 60 * 1000; static const wchar_t kRegPath[] = L"Software\\Microsoft\\Windows\\CurrentVersion\\Shell Extensions\\Approved"; -// A sort method that sorts by ModuleType ordinal (loaded module at the top), -// then by full name (including path). +// A sort method that sorts by bad modules first, then by full name (including +// path). static bool ModuleSort(const ModuleEnumerator::Module& a, const ModuleEnumerator::Module& b) { - if (a.type != b.type) - return a.type < b.type; + if (a.status != b.status) + return a.status > b.status; + if (a.location == b.location) return a.name < b.name; @@ -61,10 +62,8 @@ struct FindModule { public: explicit FindModule(const ModuleEnumerator::Module& x) : module(x) {} - // Coverity issue 13896. bool operator()(const ModuleEnumerator::Module& module_in) const { - return (module.type == module_in.type) && - (module.location == module_in.location) && + return (module.location == module_in.location) && (module.name == module_in.name); } @@ -91,7 +90,7 @@ const ModuleEnumerator::BlacklistEntry ModuleEnumerator::kModuleBlacklist[] = { // idmmbc.dll, "%programfiles%\\internet download manager\\", "Tonec Inc.". // See: http://crbug.com/26892/. - {"b8dce5c3", "94541bf5", "d33ad640", "", "", NONE}, + {"b8dce5c3", "94541bf5", "d33ad640", "", "6.03", UPDATE}, // imon.dll. See: http://crbug.com/21715. {"8f42f22e", "", "", "", "", NONE}, @@ -194,7 +193,7 @@ ModuleEnumerator::ModuleStatus ModuleEnumerator::Match( module_version->CompareTo(*version_min.get()) < 0); bool too_high = version_max.get() && (!module_version.get() || - module_version->CompareTo(*version_max.get()) > 0); + module_version->CompareTo(*version_max.get()) >= 0); version_ok = !too_low && !too_high; } @@ -251,6 +250,9 @@ void ModuleEnumerator::ScanOnFileThread() { PreparePathMappings(); base::TimeTicks checkpoint = base::TimeTicks::Now(); + + // Enumerating loaded modules must happen first since the other types of + // modules check for duplication against the loaded modules. EnumerateLoadedModules(); HISTOGRAM_TIMES("Conflicts.EnumerateLoadedModules", base::TimeTicks::Now() - checkpoint); @@ -403,10 +405,12 @@ void ModuleEnumerator::AddToListWithoutDuplicating(const Module& module) { iter = std::find_if(enumerated_modules_->begin(), enumerated_modules_->end(), FindModule(module)); - if (iter != enumerated_modules_->end()) + if (iter != enumerated_modules_->end()) { iter->duplicate_count++; - else + iter->type = static_cast(iter->type | module.type); + } else { enumerated_modules_->push_back(module); + } } void ModuleEnumerator::PreparePathMappings() { @@ -613,25 +617,28 @@ ListValue* EnumerateModulesModel::GetModuleList() { module != enumerated_modules_.end(); ++module) { DictionaryValue* data = new DictionaryValue(); data->SetInteger("type", module->type); - switch (module->type) { - case ModuleEnumerator::SHELL_EXTENSION: - data->SetString("type_description", ASCIIToWide("Shell Extension")); - break; - case ModuleEnumerator::WINSOCK_MODULE_REGISTRATION: - data->SetString("type_description", ASCIIToWide("Winsock")); - break; - default: - data->SetString("type_description", ASCIIToWide("")); - break; + string16 type_string; + if ((module->type & ModuleEnumerator::LOADED_MODULE) == 0) { + // Module is not loaded, denote type of module. + if (module->type & ModuleEnumerator::SHELL_EXTENSION) + type_string = ASCIIToWide("Shell Extension"); + if (module->type & ModuleEnumerator::WINSOCK_MODULE_REGISTRATION) { + if (!type_string.empty()) + type_string += ASCIIToWide(", "); + type_string += ASCIIToWide("Winsock"); + } + // Must be one of the above type. + DCHECK(!type_string.empty()); + type_string += ASCIIToWide(" -- "); + type_string += l10n_util::GetStringUTF16(IDS_CONFLICTS_NOT_LOADED_YET); } + data->SetString("type_description", type_string); data->SetInteger("status", module->status); data->SetString("location", module->location); data->SetString("name", module->name); data->SetString("product_name", module->product_name); data->SetString("description", module->description); - data->SetString("version", module->version.empty() ? ASCIIToWide("") : - l10n_util::GetStringF(IDS_CONFLICTS_CHECK_VERSION_STRING, - module->version)); + data->SetString("version", module->version); data->SetString("digital_signer", module->digital_signer); // Figure out the possible resolution help string. diff --git a/chrome/browser/enumerate_modules_model_win.h b/chrome/browser/enumerate_modules_model_win.h index cf65eaf..9000a6c 100644 --- a/chrome/browser/enumerate_modules_model_win.h +++ b/chrome/browser/enumerate_modules_model_win.h @@ -29,9 +29,9 @@ class ModuleEnumerator : public base::RefCountedThreadSafe { // modules of interest and may or may not be loaded in the process at the // time of scan. enum ModuleType { - LOADED_MODULE, - SHELL_EXTENSION, - WINSOCK_MODULE_REGISTRATION, + LOADED_MODULE = 1 << 0, + SHELL_EXTENSION = 1 << 1, + WINSOCK_MODULE_REGISTRATION = 1 << 2, }; // The blacklist status of the module. Suspected Bad modules have been @@ -96,8 +96,8 @@ class ModuleEnumerator : public base::RefCountedThreadSafe { const char* filename; const char* location; const char* desc_or_signer; - const char* version_from; - const char* version_to; + const char* version_from; // Version where conflict started. + const char* version_to; // First version that works. RecommendedAction help_tip; }; diff --git a/chrome/browser/resources/about_conflicts.html b/chrome/browser/resources/about_conflicts.html index c4365ff..14274f49 100644 --- a/chrome/browser/resources/about_conflicts.html +++ b/chrome/browser/resources/about_conflicts.html @@ -128,12 +128,21 @@ html[dir=rtl] #top { } .suspected-bad { - color: orange; + color: #DD7700; } .confirmed-bad { color: red; } + +.nowrap { + white-space: nowrap; +} + +.extra-info-text { + margin-top: -1em; + margin-bottom: 1em; +}