From 46488a59ea36f41de581dcdf094979982235a231 Mon Sep 17 00:00:00 2001 From: Krystian Zlomek Date: Thu, 19 Jul 2018 08:55:21 +0200 Subject: [PATCH 1/8] FilterEngine refactoring Created Updater class for handling application updates and moved respective API from FilterEngine to Updater component. For now Updater class does not care about asynchronicity (if any) in JS files which are used for update checks logic. TODO: Move Updater class to dedicated .h and .cpp files. Abstracted jsSources from FilterEngine. Now every component (like FilterEngine and Updater) created by Platfrom gets EvaluateCallback and evaluates own JS files needed for internal logic. EvaluateCallback keeps track of evaluated files to avoid duplicated evaluations. --- include/AdblockPlus/FilterEngine.h | 92 +++++++++------- include/AdblockPlus/JsEngine.h | 8 ++ include/AdblockPlus/Platform.h | 11 ++ src/FilterEngine.cpp | 167 +++++++++++++++++++++-------- src/Platform.cpp | 51 +++++++-- test/UpdateCheck.cpp | 6 +- 6 files changed, 242 insertions(+), 93 deletions(-) diff --git a/include/AdblockPlus/FilterEngine.h b/include/AdblockPlus/FilterEngine.h index dc533098..bbfc2c1f 100644 --- a/include/AdblockPlus/FilterEngine.h +++ b/include/AdblockPlus/FilterEngine.h @@ -187,7 +187,6 @@ namespace AdblockPlus * It handles: * - Filter management and matching. * - Subscription management and synchronization. - * - Update checks for the application. */ class FilterEngine { @@ -225,18 +224,6 @@ namespace AdblockPlus */ typedef int32_t ContentTypeMask; - /** - * Callback type invoked when an update becomes available. - * The parameter is the download URL of the update. - */ - typedef std::function UpdateAvailableCallback; - - /** - * Callback type invoked when a manually triggered update check finishes. - * The parameter is an optional error message. - */ - typedef std::function UpdateCheckDoneCallback; - /** * Callback type invoked when the filters change. * The first parameter is the action event code (see @@ -296,6 +283,7 @@ namespace AdblockPlus * @param parameters optional creation parameters. */ static void CreateAsync(const JsEnginePtr& jsEngine, + const JsEngine::EvaluateCallback& evaluateCallback, const OnCreatedCallback& onCreated, const CreationParameters& parameters = CreationParameters()); @@ -476,32 +464,6 @@ namespace AdblockPlus */ std::string GetHostFromURL(const std::string& url) const; - /** - * Sets the callback invoked when an application update becomes available. - * @param callback Callback to invoke. - */ - void SetUpdateAvailableCallback(const UpdateAvailableCallback& callback); - - /** - * Removes the callback invoked when an application update becomes - * available. - */ - void RemoveUpdateAvailableCallback(); - - /** - * Forces an immediate update check. - * `FilterEngine` will automatically check for updates in regular intervals, - * so applications should only call this when the user triggers an update - * check manually. - * @param callback Optional callback to invoke when the update check is - * finished. The string parameter will be empty when the update check - * succeeded, or contain an error message if it failed. - * Note that the callback will be invoked whether updates are - * available or not - to react to updates being available, use - * `FilterEngine::SetUpdateAvailableCallback()`. - */ - void ForceUpdateCheck(const UpdateCheckDoneCallback& callback = UpdateCheckDoneCallback()); - /** * Sets the callback invoked when the filters change. * @param callback Callback to invoke. @@ -558,7 +520,6 @@ namespace AdblockPlus private: JsEnginePtr jsEngine; bool firstRun; - int updateCheckId; static const std::map contentTypes; explicit FilterEngine(const JsEnginePtr& jsEngine); @@ -573,6 +534,57 @@ namespace AdblockPlus ContentTypeMask contentTypeMask, const std::vector& documentUrls) const; }; + + /** + * Component of libadblockplus responsible for Update checks for the application. + */ + class Updater + { + public: + explicit Updater(const JsEnginePtr& jsEngine, const JsEngine::EvaluateCallback& callback); + + /** + * Callback type invoked when an update becomes available. + * The parameter is the download URL of the update. + */ + typedef std::function UpdateAvailableCallback; + + /** + * Callback type invoked when a manually triggered update check finishes. + * The parameter is an optional error message. + */ + typedef std::function UpdateCheckDoneCallback; + + /** + * Sets the callback invoked when an application update becomes available. + * @param callback Callback to invoke. + */ + void SetUpdateAvailableCallback(const UpdateAvailableCallback& callback); + + /** + * Removes the callback invoked when an application update becomes + * available. + */ + void RemoveUpdateAvailableCallback(); + + /** + * Forces an immediate update check. + * `Updater` will automatically check for updates in regular intervals, + * so applications should only call this when the user triggers an update + * check manually. + * @param callback Optional callback to invoke when the update check is + * finished. The string parameter will be empty when the update check + * succeeded, or contain an error message if it failed. + * Note that the callback will be invoked whether updates are + * available or not - to react to updates being available, use + * `Updater::SetUpdateAvailableCallback()`. + */ + void ForceUpdateCheck(const UpdateCheckDoneCallback& callback = UpdateCheckDoneCallback()); + + private: + JsEnginePtr jsEngine; + int updateCheckId; + }; } #endif diff --git a/include/AdblockPlus/JsEngine.h b/include/AdblockPlus/JsEngine.h index 833e0d32..4c47a366 100644 --- a/include/AdblockPlus/JsEngine.h +++ b/include/AdblockPlus/JsEngine.h @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -143,6 +144,12 @@ namespace AdblockPlus JsValue Evaluate(const std::string& source, const std::string& filename = ""); + /** + * Callback type for evaluating JS expression. + * The parameter is the JS file name containing the expression. + */ + typedef std::function EvaluateCallback; + /** * Initiates a garbage collection. */ @@ -293,6 +300,7 @@ namespace AdblockPlus std::mutex eventCallbacksMutex; JsWeakValuesLists jsWeakValuesLists; std::mutex jsWeakValuesListsMutex; + std::set evaluatedJsSources; }; } diff --git a/include/AdblockPlus/Platform.h b/include/AdblockPlus/Platform.h index dd835471..6e9895e1 100644 --- a/include/AdblockPlus/Platform.h +++ b/include/AdblockPlus/Platform.h @@ -27,6 +27,8 @@ #include "FilterEngine.h" #include #include +#include +#include namespace AdblockPlus { @@ -108,6 +110,11 @@ namespace AdblockPlus */ FilterEngine& GetFilterEngine(); + /** + * Retrieves the Updater component instance. + */ + Updater& GetUpdater(); + typedef std::function WithTimerCallback; virtual void WithTimer(const WithTimerCallback&); @@ -125,11 +132,15 @@ namespace AdblockPlus TimerPtr timer; FileSystemPtr fileSystem; WebRequestPtr webRequest; + JsEngine::EvaluateCallback evaluateCallback; private: // used for creation and deletion of modules. std::mutex modulesMutex; std::shared_ptr jsEngine; std::shared_future filterEngine; + std::shared_ptr updater; + std::set evaluatedJsSources; + JsEngine::EvaluateCallback GetEvaluateCallback(); }; /** diff --git a/src/FilterEngine.cpp b/src/FilterEngine.cpp index 179a0dbe..f9eb9d9f 100644 --- a/src/FilterEngine.cpp +++ b/src/FilterEngine.cpp @@ -28,9 +28,82 @@ #include #include +#define ArraySize(a) (sizeof(a) / sizeof(a[0])) + using namespace AdblockPlus; -extern std::string jsSources[]; +namespace { + + /* + * TODO: Clarify JS dependencies for FilterEngine and Updater + * so both are using only required JS files. + * Now both tables are the same with full list of JS files. + */ + + static std::string filterEngineJsFiles[] = { + "compat.js", + "info.js", + "io.js", + "prefs.js", + "utils.js", + "elemHideHitRegistration.js", + "events.js", + "coreUtils.js", + "filterNotifier.js", + "init.js", + "common.js", + "filterClasses.js", + "subscriptionClasses.js", + "filterStorage.js", + "elemHide.js", + "elemHideEmulation.js", + "matcher.js", + "filterListener.js", + "downloader.js", + "notification.js", + "notificationShowRegistration.js", + "synchronizer.js", + "filterUpdateRegistration.js", + "subscriptions.xml", + "updater.js", + "api.js", + "publicSuffixList.js", + "punycode.js", + "basedomain.js" + }; + + static std::string updaterJsFiles[] = { + "compat.js", + "info.js", + "io.js", + "prefs.js", + "utils.js", + "elemHideHitRegistration.js", + "events.js", + "coreUtils.js", + "filterNotifier.js", + "init.js", + "common.js", + "filterClasses.js", + "subscriptionClasses.js", + "filterStorage.js", + "elemHide.js", + "elemHideEmulation.js", + "matcher.js", + "filterListener.js", + "downloader.js", + "notification.js", + "notificationShowRegistration.js", + "synchronizer.js", + "filterUpdateRegistration.js", + "subscriptions.xml", + "updater.js", + "api.js", + "publicSuffixList.js", + "punycode.js", + "basedomain.js" + }; +} Filter::Filter(JsValue&& value) : JsValue(std::move(value)) @@ -183,11 +256,13 @@ bool Subscription::operator==(const Subscription& subscription) const } FilterEngine::FilterEngine(const JsEnginePtr& jsEngine) - : jsEngine(jsEngine), firstRun(false), updateCheckId(0) + : jsEngine(jsEngine), firstRun(false) { + } void FilterEngine::CreateAsync(const JsEnginePtr& jsEngine, + const JsEngine::EvaluateCallback& evaluateCallback, const FilterEngine::OnCreatedCallback& onCreated, const FilterEngine::CreationParameters& params) { @@ -228,7 +303,7 @@ void FilterEngine::CreateAsync(const JsEnginePtr& jsEngine, isSubscriptionDownloadAllowedCallback(params[0].IsString() ? &allowedConnectionType : nullptr, callJsCallback); }); } - + jsEngine->SetEventCallback("_init", [jsEngine, filterEngine, onCreated](JsValueList&& params) { filterEngine->firstRun = params.size() && params[0].AsBool(); @@ -246,19 +321,16 @@ void FilterEngine::CreateAsync(const JsEnginePtr& jsEngine, filterEngine->GetJsEngine().NotifyLowMemory(); }); - // Lock the JS engine while we are loading scripts, no timeouts should fire - // until we are done. - const JsContext context(*jsEngine); // Set the preconfigured prefs auto preconfiguredPrefsObject = jsEngine->NewObject(); for (const auto& pref : params.preconfiguredPrefs) - { preconfiguredPrefsObject.SetProperty(pref.first, pref.second); - } jsEngine->SetGlobalProperty("_preconfiguredPrefs", preconfiguredPrefsObject); + // Load adblockplus scripts - for (int i = 0; !jsSources[i].empty(); i += 2) - jsEngine->Evaluate(jsSources[i + 1], jsSources[i]); + for(size_t i = 0; i < ArraySize(filterEngineJsFiles); ++i) + evaluateCallback(filterEngineJsFiles[i]); + } namespace @@ -491,40 +563,6 @@ std::string FilterEngine::GetHostFromURL(const std::string& url) const return func.Call(jsEngine->NewValue(url)).AsString(); } -void FilterEngine::SetUpdateAvailableCallback( - const FilterEngine::UpdateAvailableCallback& callback) -{ - jsEngine->SetEventCallback("updateAvailable", [this, callback](JsValueList&& params) - { - if (params.size() >= 1 && !params[0].IsNull()) - callback(params[0].AsString()); - }); -} - -void FilterEngine::RemoveUpdateAvailableCallback() -{ - jsEngine->RemoveEventCallback("updateAvailable"); -} - -void FilterEngine::ForceUpdateCheck( - const FilterEngine::UpdateCheckDoneCallback& callback) -{ - JsValue func = jsEngine->Evaluate("API.forceUpdateCheck"); - JsValueList params; - if (callback) - { - std::string eventName = "_updateCheckDone" + std::to_string(++updateCheckId); - jsEngine->SetEventCallback(eventName, [this, eventName, callback](JsValueList&& params) - { - std::string error(params.size() >= 1 && !params[0].IsNull() ? params[0].AsString() : ""); - callback(error); - jsEngine->RemoveEventCallback(eventName); - }); - params.push_back(jsEngine->NewValue(eventName)); - } - func.Call(params); -} - void FilterEngine::SetFilterChangeCallback(const FilterChangeCallback& callback) { jsEngine->SetEventCallback("filterChange", [this, callback](JsValueList&& params) @@ -602,3 +640,44 @@ FilterPtr FilterEngine::GetWhitelistingFilter(const std::string& url, while (urlIterator != documentUrls.end()); return FilterPtr(); } + + +Updater::Updater(const JsEnginePtr& jsEngine, const JsEngine::EvaluateCallback& evaluateCallback) + : jsEngine(jsEngine), updateCheckId(0) +{ + // Load adblockplus scripts + for(size_t i = 0; i < ArraySize(updaterJsFiles); ++i) + evaluateCallback(updaterJsFiles[i]); +} + +void Updater::SetUpdateAvailableCallback(const Updater::UpdateAvailableCallback& callback) +{ + jsEngine->SetEventCallback("updateAvailable", [this, callback](JsValueList&& params) + { + if (params.size() >= 1 && !params[0].IsNull()) + callback(params[0].AsString()); + }); +} + +void Updater::RemoveUpdateAvailableCallback() +{ + jsEngine->RemoveEventCallback("updateAvailable"); +} + +void Updater::ForceUpdateCheck(const Updater::UpdateCheckDoneCallback& callback) +{ + JsValue func = jsEngine->Evaluate("API.forceUpdateCheck"); + JsValueList params; + if (callback) + { + std::string eventName = "_updateCheckDone" + std::to_string(++updateCheckId); + jsEngine->SetEventCallback(eventName, [this, eventName, callback](JsValueList&& params) + { + std::string error(params.size() >= 1 && !params[0].IsNull() ? params[0].AsString() : ""); + callback(error); + jsEngine->RemoveEventCallback(eventName); + }); + params.push_back(jsEngine->NewValue(eventName)); + } + func.Call(params); +} diff --git a/src/Platform.cpp b/src/Platform.cpp index ffe6e690..e3ae9712 100644 --- a/src/Platform.cpp +++ b/src/Platform.cpp @@ -19,6 +19,7 @@ #include #include #include +#include "JsContext.h" #include "DefaultTimer.h" #include "DefaultWebRequest.h" #include "DefaultFileSystem.h" @@ -26,6 +27,9 @@ using namespace AdblockPlus; + +extern std::string jsSources[]; + namespace { template @@ -64,6 +68,26 @@ JsEngine& Platform::GetJsEngine() return *jsEngine; } +JsEngine::EvaluateCallback Platform::GetEvaluateCallback() +{ + // GetEvaluateCallback() method assumes that jsEngine is already created + return [this](const std::string& filename) + { + if (evaluatedJsSources.find(filename) != evaluatedJsSources.end()) + return; //NO-OP, file was already evaluated + + // Lock the JS engine while we are loading scripts + const JsContext context(*jsEngine); + for (int i = 0; !jsSources[i].empty(); i += 2) + if (jsSources[i] == filename) + { + jsEngine->Evaluate(jsSources[i + 1], jsSources[i]); + evaluatedJsSources.insert(filename); + break; + } + }; +} + void Platform::CreateFilterEngineAsync(const FilterEngine::CreationParameters& parameters, const OnFilterEngineCreatedCallback& onCreated) { @@ -77,12 +101,17 @@ void Platform::CreateFilterEngineAsync(const FilterEngine::CreationParameters& p } GetJsEngine(); // ensures that JsEngine is instantiated - FilterEngine::CreateAsync(jsEngine, [this, onCreated, filterEnginePromise](const FilterEnginePtr& filterEngine) - { - filterEnginePromise->set_value(filterEngine); - if (onCreated) - onCreated(*filterEngine); - }, parameters); + FilterEngine::CreateAsync( + jsEngine, + GetEvaluateCallback(), + [this, onCreated, filterEnginePromise](const FilterEnginePtr& filterEngine) + { + filterEnginePromise->set_value(filterEngine); + if (onCreated) + onCreated(*filterEngine); + }, + parameters + ); } FilterEngine& Platform::GetFilterEngine() @@ -91,6 +120,16 @@ FilterEngine& Platform::GetFilterEngine() return *std::shared_future(filterEngine).get(); } +Updater& Platform::GetUpdater() +{ + if (updater == nullptr) + { + GetJsEngine(); // ensures that JsEngine is instantiated + updater = std::make_shared(jsEngine, GetEvaluateCallback()); + } + return *updater; +} + void Platform::WithTimer(const WithTimerCallback& callback) { if (timer && callback) diff --git a/test/UpdateCheck.cpp b/test/UpdateCheck.cpp index 08d843fe..fc8b8b1a 100644 --- a/test/UpdateCheck.cpp +++ b/test/UpdateCheck.cpp @@ -87,7 +87,7 @@ namespace void ForceUpdateCheck() { - platform->GetFilterEngine().ForceUpdateCheck([this](const std::string& error) + platform->GetUpdater().ForceUpdateCheck([this](const std::string& error) { updateCallbackCalled = true; updateError = error; @@ -277,7 +277,7 @@ TEST_F(UpdateCheckTest, SetRemoveUpdateAvailableCallback) CreateFilterEngine(); int timesCalled = 0; - platform->GetFilterEngine().SetUpdateAvailableCallback([×Called](const std::string&)->void + platform->GetUpdater().SetUpdateAvailableCallback([×Called](const std::string&)->void { ++timesCalled; }); @@ -292,7 +292,7 @@ TEST_F(UpdateCheckTest, SetRemoveUpdateAvailableCallback) // handler for "updateAvailable" event. EXPECT_FALSE(eventCallbackCalled); - platform->GetFilterEngine().RemoveUpdateAvailableCallback(); + platform->GetUpdater().RemoveUpdateAvailableCallback(); ForceUpdateCheck(); // ensure that the was the corresponding request From 540ca04db9ec08905ca302d9c7ab5f53c411605d Mon Sep 17 00:00:00 2001 From: Krystian Zlomek Date: Thu, 19 Jul 2018 09:03:40 +0200 Subject: [PATCH 2/8] WIP: FilterEngine and Updater JS api separation Moved Updater class to dedicated header and source files. Created apiUpdater.js with a separate API for Updater component. Issues to fix: 1. Pass Prefs to Updater component - Now Updater sets just an empty Prefs object 2. Review content of apiUpdater.js and updaterJsFiles[] array - Check that all what's needed is set there and nothing is redundant - Does Updater component need to take care of asynchronicity in JS files? 3. Fix UpdateCheckTests - They are now passing just because FilterEngine object is created while those tests should just rely only on Updater object. --- include/AdblockPlus.h | 1 + include/AdblockPlus/FilterEngine.h | 51 ------------------ include/AdblockPlus/Platform.h | 1 + include/AdblockPlus/Updater.h | 79 ++++++++++++++++++++++++++++ lib/api.js | 7 +-- lib/apiUpdater.js | 52 ++++++++++++++++++ libadblockplus.gyp | 2 + src/FilterEngine.cpp | 84 +----------------------------- src/Updater.cpp | 81 ++++++++++++++++++++++++++++ src/Utils.h | 2 + 10 files changed, 220 insertions(+), 140 deletions(-) create mode 100644 include/AdblockPlus/Updater.h create mode 100644 lib/apiUpdater.js create mode 100644 src/Updater.cpp diff --git a/include/AdblockPlus.h b/include/AdblockPlus.h index 4ec66280..d80f71bd 100644 --- a/include/AdblockPlus.h +++ b/include/AdblockPlus.h @@ -32,6 +32,7 @@ #include #include +#include #include #include #include diff --git a/include/AdblockPlus/FilterEngine.h b/include/AdblockPlus/FilterEngine.h index bbfc2c1f..91156bcf 100644 --- a/include/AdblockPlus/FilterEngine.h +++ b/include/AdblockPlus/FilterEngine.h @@ -534,57 +534,6 @@ namespace AdblockPlus ContentTypeMask contentTypeMask, const std::vector& documentUrls) const; }; - - /** - * Component of libadblockplus responsible for Update checks for the application. - */ - class Updater - { - public: - explicit Updater(const JsEnginePtr& jsEngine, const JsEngine::EvaluateCallback& callback); - - /** - * Callback type invoked when an update becomes available. - * The parameter is the download URL of the update. - */ - typedef std::function UpdateAvailableCallback; - - /** - * Callback type invoked when a manually triggered update check finishes. - * The parameter is an optional error message. - */ - typedef std::function UpdateCheckDoneCallback; - - /** - * Sets the callback invoked when an application update becomes available. - * @param callback Callback to invoke. - */ - void SetUpdateAvailableCallback(const UpdateAvailableCallback& callback); - - /** - * Removes the callback invoked when an application update becomes - * available. - */ - void RemoveUpdateAvailableCallback(); - - /** - * Forces an immediate update check. - * `Updater` will automatically check for updates in regular intervals, - * so applications should only call this when the user triggers an update - * check manually. - * @param callback Optional callback to invoke when the update check is - * finished. The string parameter will be empty when the update check - * succeeded, or contain an error message if it failed. - * Note that the callback will be invoked whether updates are - * available or not - to react to updates being available, use - * `Updater::SetUpdateAvailableCallback()`. - */ - void ForceUpdateCheck(const UpdateCheckDoneCallback& callback = UpdateCheckDoneCallback()); - - private: - JsEnginePtr jsEngine; - int updateCheckId; - }; } #endif diff --git a/include/AdblockPlus/Platform.h b/include/AdblockPlus/Platform.h index 6e9895e1..c0718960 100644 --- a/include/AdblockPlus/Platform.h +++ b/include/AdblockPlus/Platform.h @@ -25,6 +25,7 @@ #include "AppInfo.h" #include "Scheduler.h" #include "FilterEngine.h" +#include "Updater.h" #include #include #include diff --git a/include/AdblockPlus/Updater.h b/include/AdblockPlus/Updater.h new file mode 100644 index 00000000..2f5a00a8 --- /dev/null +++ b/include/AdblockPlus/Updater.h @@ -0,0 +1,79 @@ +/* + * This file is part of Adblock Plus , + * Copyright (C) 2006-present eyeo GmbH + * + * Adblock Plus is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 3 as + * published by the Free Software Foundation. + * + * Adblock Plus is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Adblock Plus. If not, see . + */ + +#ifndef ADBLOCK_PLUS_UPDATER_H +#define ADBLOCK_PLUS_UPDATER_H + +#include +#include +#include + +namespace AdblockPlus +{ + /** + * Component of libadblockplus responsible for Update checks for the application. + */ + class Updater + { + public: + explicit Updater(const JsEnginePtr& jsEngine, const JsEngine::EvaluateCallback& callback); + + /** + * Callback type invoked when an update becomes available. + * The parameter is the download URL of the update. + */ + typedef std::function UpdateAvailableCallback; + + /** + * Callback type invoked when a manually triggered update check finishes. + * The parameter is an optional error message. + */ + typedef std::function UpdateCheckDoneCallback; + + /** + * Sets the callback invoked when an application update becomes available. + * @param callback Callback to invoke. + */ + void SetUpdateAvailableCallback(const UpdateAvailableCallback& callback); + + /** + * Removes the callback invoked when an application update becomes + * available. + */ + void RemoveUpdateAvailableCallback(); + + /** + * Forces an immediate update check. + * `Updater` will automatically check for updates in regular intervals, + * so applications should only call this when the user triggers an update + * check manually. + * @param callback Optional callback to invoke when the update check is + * finished. The string parameter will be empty when the update check + * succeeded, or contain an error message if it failed. + * Note that the callback will be invoked whether updates are + * available or not - to react to updates being available, use + * `Updater::SetUpdateAvailableCallback()`. + */ + void ForceUpdateCheck(const UpdateCheckDoneCallback& callback = UpdateCheckDoneCallback()); + + private: + JsEnginePtr jsEngine; + int updateCheckId; + }; +} + +#endif diff --git a/lib/api.js b/lib/api.js index bfcc6794..7092d5f0 100644 --- a/lib/api.js +++ b/lib/api.js @@ -28,7 +28,6 @@ let API = (() => const {ElemHide} = require("elemHide"); const {Synchronizer} = require("synchronizer"); const {Prefs} = require("prefs"); - const {checkForUpdates} = require("updater"); const {Notification} = require("notification"); return { @@ -188,6 +187,7 @@ let API = (() => { Notification.markAsShown(id); }, + checkFilterMatch(url, contentTypeMask, documentUrl) { let requestHost = extractHostFromURL(url); @@ -213,11 +213,6 @@ let API = (() => Prefs[pref] = value; }, - forceUpdateCheck(eventName) - { - checkForUpdates(eventName ? _triggerEvent.bind(null, eventName) : null); - }, - getHostFromUrl(url) { return extractHostFromURL(url); diff --git a/lib/apiUpdater.js b/lib/apiUpdater.js new file mode 100644 index 00000000..888f2284 --- /dev/null +++ b/lib/apiUpdater.js @@ -0,0 +1,52 @@ +/* + * This file is part of Adblock Plus , + * Copyright (C) 2006-present eyeo GmbH + * + * Adblock Plus is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 3 as + * published by the Free Software Foundation. + * + * Adblock Plus is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Adblock Plus. If not, see . + */ + +"use strict"; + +let API_UPDATER = (() => +{ + const {Services} = Cu.import("resource://gre/modules/Services.jsm", {}); + const {Prefs} = require("prefs"); + const {checkForUpdates} = require("updater"); + + return { + getPref(pref) + { + return Prefs[pref]; + }, + + setPref(pref, value) + { + Prefs[pref] = value; + }, + + forceUpdateCheck(eventName) + { + checkForUpdates(eventName ? _triggerEvent.bind(null, eventName) : null); + }, + + getHostFromUrl(url) + { + return extractHostFromURL(url); + }, + + compareVersions(v1, v2) + { + return Services.vc.compare(v1, v2); + } + }; +})(); diff --git a/libadblockplus.gyp b/libadblockplus.gyp index 45138cd9..d56d0101 100644 --- a/libadblockplus.gyp +++ b/libadblockplus.gyp @@ -44,6 +44,7 @@ 'src/DefaultWebRequest.cpp', 'src/FileSystemJsObject.cpp', 'src/FilterEngine.cpp', + 'src/Updater.cpp', 'src/GlobalJsObject.cpp', 'src/JsContext.cpp', 'src/JsEngine.cpp', @@ -159,6 +160,7 @@ ], 'load_after_files': [ 'lib/api.js', + 'lib/apiUpdater.js', 'lib/publicSuffixList.js', 'lib/punycode.js', 'lib/basedomain.js', diff --git a/src/FilterEngine.cpp b/src/FilterEngine.cpp index f9eb9d9f..5220e090 100644 --- a/src/FilterEngine.cpp +++ b/src/FilterEngine.cpp @@ -23,23 +23,15 @@ #include #include +#include "Utils.h" #include "JsContext.h" #include "Thread.h" #include #include -#define ArraySize(a) (sizeof(a) / sizeof(a[0])) - using namespace AdblockPlus; namespace { - - /* - * TODO: Clarify JS dependencies for FilterEngine and Updater - * so both are using only required JS files. - * Now both tables are the same with full list of JS files. - */ - static std::string filterEngineJsFiles[] = { "compat.js", "info.js", @@ -65,39 +57,6 @@ namespace { "synchronizer.js", "filterUpdateRegistration.js", "subscriptions.xml", - "updater.js", - "api.js", - "publicSuffixList.js", - "punycode.js", - "basedomain.js" - }; - - static std::string updaterJsFiles[] = { - "compat.js", - "info.js", - "io.js", - "prefs.js", - "utils.js", - "elemHideHitRegistration.js", - "events.js", - "coreUtils.js", - "filterNotifier.js", - "init.js", - "common.js", - "filterClasses.js", - "subscriptionClasses.js", - "filterStorage.js", - "elemHide.js", - "elemHideEmulation.js", - "matcher.js", - "filterListener.js", - "downloader.js", - "notification.js", - "notificationShowRegistration.js", - "synchronizer.js", - "filterUpdateRegistration.js", - "subscriptions.xml", - "updater.js", "api.js", "publicSuffixList.js", "punycode.js", @@ -640,44 +599,3 @@ FilterPtr FilterEngine::GetWhitelistingFilter(const std::string& url, while (urlIterator != documentUrls.end()); return FilterPtr(); } - - -Updater::Updater(const JsEnginePtr& jsEngine, const JsEngine::EvaluateCallback& evaluateCallback) - : jsEngine(jsEngine), updateCheckId(0) -{ - // Load adblockplus scripts - for(size_t i = 0; i < ArraySize(updaterJsFiles); ++i) - evaluateCallback(updaterJsFiles[i]); -} - -void Updater::SetUpdateAvailableCallback(const Updater::UpdateAvailableCallback& callback) -{ - jsEngine->SetEventCallback("updateAvailable", [this, callback](JsValueList&& params) - { - if (params.size() >= 1 && !params[0].IsNull()) - callback(params[0].AsString()); - }); -} - -void Updater::RemoveUpdateAvailableCallback() -{ - jsEngine->RemoveEventCallback("updateAvailable"); -} - -void Updater::ForceUpdateCheck(const Updater::UpdateCheckDoneCallback& callback) -{ - JsValue func = jsEngine->Evaluate("API.forceUpdateCheck"); - JsValueList params; - if (callback) - { - std::string eventName = "_updateCheckDone" + std::to_string(++updateCheckId); - jsEngine->SetEventCallback(eventName, [this, eventName, callback](JsValueList&& params) - { - std::string error(params.size() >= 1 && !params[0].IsNull() ? params[0].AsString() : ""); - callback(error); - jsEngine->RemoveEventCallback(eventName); - }); - params.push_back(jsEngine->NewValue(eventName)); - } - func.Call(params); -} diff --git a/src/Updater.cpp b/src/Updater.cpp new file mode 100644 index 00000000..0bc8324a --- /dev/null +++ b/src/Updater.cpp @@ -0,0 +1,81 @@ +/* + * This file is part of Adblock Plus , + * Copyright (C) 2006-present eyeo GmbH + * + * Adblock Plus is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 3 as + * published by the Free Software Foundation. + * + * Adblock Plus is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Adblock Plus. If not, see . + */ + +#include +#include "Utils.h" + +using namespace AdblockPlus; + +namespace { + static std::string updaterJsFiles[] = { + "compat.js", + "info.js", + "prefs.js", + "utils.js", + "events.js", + "coreUtils.js", + "common.js", + "downloader.js", + "updater.js", + "apiUpdater.js", + "basedomain.js" + }; +} + +Updater::Updater(const JsEnginePtr& jsEngine, const JsEngine::EvaluateCallback& evaluateCallback) + : jsEngine(jsEngine), updateCheckId(0) +{ + // Set empty preconfigured prefs + auto preconfiguredPrefsObject = jsEngine->NewObject(); + jsEngine->SetGlobalProperty("_preconfiguredPrefs", preconfiguredPrefsObject); + + // Load adblockplus scripts + for(size_t i = 0; i < ArraySize(updaterJsFiles); ++i) + evaluateCallback(updaterJsFiles[i]); +} + +void Updater::SetUpdateAvailableCallback(const Updater::UpdateAvailableCallback& callback) +{ + jsEngine->SetEventCallback("updateAvailable", [this, callback](JsValueList&& params) + { + if (params.size() >= 1 && !params[0].IsNull()) + callback(params[0].AsString()); + }); +} + +void Updater::RemoveUpdateAvailableCallback() +{ + jsEngine->RemoveEventCallback("updateAvailable"); +} + +void Updater::ForceUpdateCheck(const Updater::UpdateCheckDoneCallback& callback) +{ + JsValue func = jsEngine->Evaluate("API_UPDATER.forceUpdateCheck"); + JsValueList params; + if (callback) + { + std::string eventName = "_updateCheckDone" + std::to_string(++updateCheckId); + jsEngine->SetEventCallback(eventName, [this, eventName, callback](JsValueList&& params) + { + std::string error(params.size() >= 1 && !params[0].IsNull() ? params[0].AsString() : ""); + callback(error); + jsEngine->RemoveEventCallback(eventName); + }); + params.push_back(jsEngine->NewValue(eventName)); + } + func.Call(params); +} diff --git a/src/Utils.h b/src/Utils.h index 5cd71a09..a1fa90dc 100644 --- a/src/Utils.h +++ b/src/Utils.h @@ -29,6 +29,8 @@ #include "JsError.h" +#define ArraySize(a) (sizeof(a) / sizeof(a[0])) + namespace AdblockPlus { namespace Utils From 5d210811b499ebb389e3e11fdb6b53866cc3c732 Mon Sep 17 00:00:00 2001 From: Krystian Zlomek Date: Fri, 20 Jul 2018 16:43:37 +0200 Subject: [PATCH 3/8] Updates in Updater class Added cpp methods for Prefs JS API. Removed not needed JS API. Added _isSubscriptionDownloadAllowed event callback to unblock downloads in JS code. Cleaned up UpdateCheckTest a bit removing FilterEngine because with the change above those tests don't require anymore FilterEnigne object to pass. --- include/AdblockPlus/Updater.h | 15 +++++++++++++++ lib/apiUpdater.js | 10 ---------- src/Updater.cpp | 31 +++++++++++++++++++++++++++++-- test/UpdateCheck.cpp | 24 ++++++++++++------------ 4 files changed, 56 insertions(+), 24 deletions(-) diff --git a/include/AdblockPlus/Updater.h b/include/AdblockPlus/Updater.h index 2f5a00a8..d8f0f17a 100644 --- a/include/AdblockPlus/Updater.h +++ b/include/AdblockPlus/Updater.h @@ -21,6 +21,7 @@ #include #include #include +#include namespace AdblockPlus { @@ -70,6 +71,20 @@ namespace AdblockPlus */ void ForceUpdateCheck(const UpdateCheckDoneCallback& callback = UpdateCheckDoneCallback()); + /** + * Retrieves a preference value. + * @param pref Preference name. + * @return Preference value, or `null` if it doesn't exist. + */ + JsValue GetPref(const std::string& pref) const; + + /** + * Sets a preference value. + * @param pref Preference name. + * @param value New value of the preference. + */ + void SetPref(const std::string& pref, const JsValue& value); + private: JsEnginePtr jsEngine; int updateCheckId; diff --git a/lib/apiUpdater.js b/lib/apiUpdater.js index 888f2284..c329af75 100644 --- a/lib/apiUpdater.js +++ b/lib/apiUpdater.js @@ -37,16 +37,6 @@ let API_UPDATER = (() => forceUpdateCheck(eventName) { checkForUpdates(eventName ? _triggerEvent.bind(null, eventName) : null); - }, - - getHostFromUrl(url) - { - return extractHostFromURL(url); - }, - - compareVersions(v1, v2) - { - return Services.vc.compare(v1, v2); } }; })(); diff --git a/src/Updater.cpp b/src/Updater.cpp index 0bc8324a..9d96188e 100644 --- a/src/Updater.cpp +++ b/src/Updater.cpp @@ -17,6 +17,7 @@ #include #include "Utils.h" +#include using namespace AdblockPlus; @@ -31,14 +32,25 @@ namespace { "common.js", "downloader.js", "updater.js", - "apiUpdater.js", - "basedomain.js" + "apiUpdater.js" }; } Updater::Updater(const JsEnginePtr& jsEngine, const JsEngine::EvaluateCallback& evaluateCallback) : jsEngine(jsEngine), updateCheckId(0) { + // Hack to enable downloads from JS (see compat.js) + jsEngine->SetEventCallback("_isSubscriptionDownloadAllowed", [this](JsValueList&& params) + { + // param[0] - nullable string Prefs.allowed_connection_type + // param[1] - function(Boolean) + bool areArgumentsValid = params.size() == 2 && (params[0].IsNull() || params[0].IsString()) && params[1].IsFunction(); + assert(areArgumentsValid && "Invalid argument: there should be two args and the second one should be a function"); + if (!areArgumentsValid) + return; + params[1].Call(this->jsEngine->NewValue(true)); + }); + // Set empty preconfigured prefs auto preconfiguredPrefsObject = jsEngine->NewObject(); jsEngine->SetGlobalProperty("_preconfiguredPrefs", preconfiguredPrefsObject); @@ -79,3 +91,18 @@ void Updater::ForceUpdateCheck(const Updater::UpdateCheckDoneCallback& callback) } func.Call(params); } + +JsValue Updater::GetPref(const std::string& pref) const +{ + JsValue func = jsEngine->Evaluate("API_UPDATER.getPref"); + return func.Call(jsEngine->NewValue(pref)); +} + +void Updater::SetPref(const std::string& pref, const JsValue& value) +{ + JsValue func = jsEngine->Evaluate("API_UPDATER.setPref"); + JsValueList params; + params.push_back(jsEngine->NewValue(pref)); + params.push_back(value); + func.Call(params); +} diff --git a/test/UpdateCheck.cpp b/test/UpdateCheck.cpp index fc8b8b1a..7c57675d 100644 --- a/test/UpdateCheck.cpp +++ b/test/UpdateCheck.cpp @@ -48,7 +48,7 @@ namespace updateCallbackCalled = false; } - void CreateFilterEngine() + void CreateUpdater() { LazyFileSystem* fileSystem; ThrowingPlatformCreationParameters platformParams; @@ -64,7 +64,7 @@ namespace eventCallbackParams = std::move(params); }); - ::CreateFilterEngine(*fileSystem, *platform); + platform->GetUpdater(); } // Returns a URL or the empty string if there is no such request. @@ -107,7 +107,7 @@ TEST_F(UpdateCheckTest, RequestFailure) appInfo.applicationVersion = "2"; appInfo.developmentBuild = false; - CreateFilterEngine(); + CreateUpdater(); ForceUpdateCheck(); auto requestUrl = ProcessPendingUpdateWebRequest(); @@ -116,7 +116,7 @@ TEST_F(UpdateCheckTest, RequestFailure) ASSERT_TRUE(updateCallbackCalled); ASSERT_FALSE(updateError.empty()); - std::string expectedUrl(platform->GetFilterEngine().GetPref("update_url_release").AsString()); + std::string expectedUrl(platform->GetUpdater().GetPref("update_url_release").AsString()); std::string platform = GetJsEngine().Evaluate("require('info').platform").AsString(); std::string platformVersion = GetJsEngine().Evaluate("require('info').platformVersion").AsString(); @@ -144,7 +144,7 @@ TEST_F(UpdateCheckTest, UpdateAvailable) appInfo.applicationVersion = "2"; appInfo.developmentBuild = true; - CreateFilterEngine(); + CreateUpdater(); ForceUpdateCheck(); auto requestUrl = ProcessPendingUpdateWebRequest(); @@ -155,7 +155,7 @@ TEST_F(UpdateCheckTest, UpdateAvailable) ASSERT_TRUE(updateCallbackCalled); ASSERT_TRUE(updateError.empty()); - std::string expectedUrl(platform->GetFilterEngine().GetPref("update_url_devbuild").AsString()); + std::string expectedUrl(platform->GetUpdater().GetPref("update_url_devbuild").AsString()); std::string platform = GetJsEngine().Evaluate("require('info').platform").AsString(); std::string platformVersion = GetJsEngine().Evaluate("require('info').platformVersion").AsString(); @@ -183,7 +183,7 @@ TEST_F(UpdateCheckTest, ApplicationUpdateAvailable) appInfo.applicationVersion = "2"; appInfo.developmentBuild = true; - CreateFilterEngine(); + CreateUpdater(); ForceUpdateCheck(); ProcessPendingUpdateWebRequest(); @@ -206,7 +206,7 @@ TEST_F(UpdateCheckTest, WrongApplication) appInfo.applicationVersion = "2"; appInfo.developmentBuild = true; - CreateFilterEngine(); + CreateUpdater(); ForceUpdateCheck(); ProcessPendingUpdateWebRequest(); @@ -228,7 +228,7 @@ TEST_F(UpdateCheckTest, WrongVersion) appInfo.applicationVersion = "2"; appInfo.developmentBuild = true; - CreateFilterEngine(); + CreateUpdater(); ForceUpdateCheck(); ProcessPendingUpdateWebRequest(); @@ -250,7 +250,7 @@ TEST_F(UpdateCheckTest, WrongURL) appInfo.applicationVersion = "2"; appInfo.developmentBuild = true; - CreateFilterEngine(); + CreateUpdater(); ForceUpdateCheck(); ProcessPendingUpdateWebRequest(); @@ -274,7 +274,7 @@ TEST_F(UpdateCheckTest, SetRemoveUpdateAvailableCallback) appInfo.name = "test"; appInfo.version = "1.0.1"; - CreateFilterEngine(); + CreateUpdater(); int timesCalled = 0; platform->GetUpdater().SetUpdateAvailableCallback([×Called](const std::string&)->void @@ -288,7 +288,7 @@ TEST_F(UpdateCheckTest, SetRemoveUpdateAvailableCallback) EXPECT_EQ(1, timesCalled); - // FilterEngine::SetUpdateAvailableCallback overriddes previously installed on JsEngine + // Updater::SetUpdateAvailableCallback overriddes previously installed on JsEngine // handler for "updateAvailable" event. EXPECT_FALSE(eventCallbackCalled); From 7d6bdda352825fbc69737b5032575b0fe064b7dd Mon Sep 17 00:00:00 2001 From: Krystian Zlomek Date: Sun, 22 Jul 2018 12:32:37 +0200 Subject: [PATCH 4/8] Code cleanup --- include/AdblockPlus/JsEngine.h | 2 -- src/Platform.cpp | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/include/AdblockPlus/JsEngine.h b/include/AdblockPlus/JsEngine.h index 4c47a366..cab146b1 100644 --- a/include/AdblockPlus/JsEngine.h +++ b/include/AdblockPlus/JsEngine.h @@ -21,7 +21,6 @@ #include #include #include -#include #include #include #include @@ -300,7 +299,6 @@ namespace AdblockPlus std::mutex eventCallbacksMutex; JsWeakValuesLists jsWeakValuesLists; std::mutex jsWeakValuesListsMutex; - std::set evaluatedJsSources; }; } diff --git a/src/Platform.cpp b/src/Platform.cpp index e3ae9712..9c2dcbe3 100644 --- a/src/Platform.cpp +++ b/src/Platform.cpp @@ -84,7 +84,7 @@ JsEngine::EvaluateCallback Platform::GetEvaluateCallback() jsEngine->Evaluate(jsSources[i + 1], jsSources[i]); evaluatedJsSources.insert(filename); break; - } + } }; } From 49f017119996ea894a8e08c5be3a2b1f2d4d9272 Mon Sep 17 00:00:00 2001 From: Krystian Zlomek Date: Sat, 4 Aug 2018 15:41:53 +0200 Subject: [PATCH 5/8] Addressed review comments Apart from changes in new code, this also withdraws changes in existing code not related with this feature. --- include/AdblockPlus/FilterEngine.h | 8 ++++++- include/AdblockPlus/JsEngine.h | 6 ----- include/AdblockPlus/Platform.h | 4 ++-- include/AdblockPlus/Updater.h | 10 +++++++-- lib/apiUpdater.js | 1 - lib/compat.js | 7 ++++++ lib/prefs.js | 9 ++++---- src/FilterEngine.cpp | 20 +++++++++++------ src/Platform.cpp | 5 +---- src/Updater.cpp | 35 ++++++++++-------------------- src/Utils.h | 2 -- 11 files changed, 54 insertions(+), 53 deletions(-) diff --git a/include/AdblockPlus/FilterEngine.h b/include/AdblockPlus/FilterEngine.h index 91156bcf..c7d4af48 100644 --- a/include/AdblockPlus/FilterEngine.h +++ b/include/AdblockPlus/FilterEngine.h @@ -274,6 +274,12 @@ namespace AdblockPlus */ typedef std::function OnCreatedCallback; + /** + * Callback type for evaluating JS expression. + * The parameter is the JS file name containing the expression. + */ + typedef std::function EvaluateCallback; + /** * Asynchronously constructs FilterEngine. * @param jsEngine `JsEngine` instance used to run JavaScript code @@ -283,7 +289,7 @@ namespace AdblockPlus * @param parameters optional creation parameters. */ static void CreateAsync(const JsEnginePtr& jsEngine, - const JsEngine::EvaluateCallback& evaluateCallback, + const EvaluateCallback& evaluateCallback, const OnCreatedCallback& onCreated, const CreationParameters& parameters = CreationParameters()); diff --git a/include/AdblockPlus/JsEngine.h b/include/AdblockPlus/JsEngine.h index cab146b1..833e0d32 100644 --- a/include/AdblockPlus/JsEngine.h +++ b/include/AdblockPlus/JsEngine.h @@ -143,12 +143,6 @@ namespace AdblockPlus JsValue Evaluate(const std::string& source, const std::string& filename = ""); - /** - * Callback type for evaluating JS expression. - * The parameter is the JS file name containing the expression. - */ - typedef std::function EvaluateCallback; - /** * Initiates a garbage collection. */ diff --git a/include/AdblockPlus/Platform.h b/include/AdblockPlus/Platform.h index c0718960..977ba24e 100644 --- a/include/AdblockPlus/Platform.h +++ b/include/AdblockPlus/Platform.h @@ -30,6 +30,7 @@ #include #include #include +#include namespace AdblockPlus { @@ -133,7 +134,6 @@ namespace AdblockPlus TimerPtr timer; FileSystemPtr fileSystem; WebRequestPtr webRequest; - JsEngine::EvaluateCallback evaluateCallback; private: // used for creation and deletion of modules. std::mutex modulesMutex; @@ -141,7 +141,7 @@ namespace AdblockPlus std::shared_future filterEngine; std::shared_ptr updater; std::set evaluatedJsSources; - JsEngine::EvaluateCallback GetEvaluateCallback(); + std::function GetEvaluateCallback(); }; /** diff --git a/include/AdblockPlus/Updater.h b/include/AdblockPlus/Updater.h index d8f0f17a..6b9593ad 100644 --- a/include/AdblockPlus/Updater.h +++ b/include/AdblockPlus/Updater.h @@ -31,8 +31,6 @@ namespace AdblockPlus class Updater { public: - explicit Updater(const JsEnginePtr& jsEngine, const JsEngine::EvaluateCallback& callback); - /** * Callback type invoked when an update becomes available. * The parameter is the download URL of the update. @@ -57,6 +55,12 @@ namespace AdblockPlus */ void RemoveUpdateAvailableCallback(); + /** + * Callback type for evaluating JS expression. + * The parameter is the JS file name containing the expression. + */ + typedef std::function EvaluateCallback; + /** * Forces an immediate update check. * `Updater` will automatically check for updates in regular intervals, @@ -85,6 +89,8 @@ namespace AdblockPlus */ void SetPref(const std::string& pref, const JsValue& value); + explicit Updater(const JsEnginePtr& jsEngine, const EvaluateCallback& callback); + private: JsEnginePtr jsEngine; int updateCheckId; diff --git a/lib/apiUpdater.js b/lib/apiUpdater.js index c329af75..ba2c3d47 100644 --- a/lib/apiUpdater.js +++ b/lib/apiUpdater.js @@ -19,7 +19,6 @@ let API_UPDATER = (() => { - const {Services} = Cu.import("resource://gre/modules/Services.jsm", {}); const {Prefs} = require("prefs"); const {checkForUpdates} = require("updater"); diff --git a/lib/compat.js b/lib/compat.js index a433b3b5..4c6f0606 100644 --- a/lib/compat.js +++ b/lib/compat.js @@ -333,6 +333,13 @@ XMLHttpRequest.prototype = for (let i = 0; i < list.length; i++) list[i].call(this, event); }; + + if (this._url.includes("update.json")) + { + window._webRequest.GET(this._url, this._requestHeaders, onGetDone); + return; + } + // HACK (#5066): the code checking whether the connection is // allowed is temporary, the actual check should be in the core // when we make a decision whether to update a subscription with diff --git a/lib/prefs.js b/lib/prefs.js index b9ac2c46..fa6fd584 100644 --- a/lib/prefs.js +++ b/lib/prefs.js @@ -146,10 +146,11 @@ let Prefs = exports.Prefs = { } }; -// Update the default prefs with what was preconfigured -for (let key in _preconfiguredPrefs) - if (preconfigurable.indexOf(key) != -1) - defaults[key] = _preconfiguredPrefs[key]; +if (typeof _preconfiguredPrefs !== "undefined") + // Update the default prefs with what was preconfigured + for (let key in _preconfiguredPrefs) + if (preconfigurable.indexOf(key) != -1) + defaults[key] = _preconfiguredPrefs[key]; // Define defaults for (let key in defaults) diff --git a/src/FilterEngine.cpp b/src/FilterEngine.cpp index 5220e090..ca3522ca 100644 --- a/src/FilterEngine.cpp +++ b/src/FilterEngine.cpp @@ -23,7 +23,6 @@ #include #include -#include "Utils.h" #include "JsContext.h" #include "Thread.h" #include @@ -31,8 +30,10 @@ using namespace AdblockPlus; -namespace { - static std::string filterEngineJsFiles[] = { +namespace +{ + const std::string filterEngineJsFiles[] = + { "compat.js", "info.js", "io.js", @@ -221,7 +222,7 @@ FilterEngine::FilterEngine(const JsEnginePtr& jsEngine) } void FilterEngine::CreateAsync(const JsEnginePtr& jsEngine, - const JsEngine::EvaluateCallback& evaluateCallback, + const EvaluateCallback& evaluateCallback, const FilterEngine::OnCreatedCallback& onCreated, const FilterEngine::CreationParameters& params) { @@ -262,7 +263,7 @@ void FilterEngine::CreateAsync(const JsEnginePtr& jsEngine, isSubscriptionDownloadAllowedCallback(params[0].IsString() ? &allowedConnectionType : nullptr, callJsCallback); }); } - + jsEngine->SetEventCallback("_init", [jsEngine, filterEngine, onCreated](JsValueList&& params) { filterEngine->firstRun = params.size() && params[0].AsBool(); @@ -280,15 +281,20 @@ void FilterEngine::CreateAsync(const JsEnginePtr& jsEngine, filterEngine->GetJsEngine().NotifyLowMemory(); }); + // Lock the JS engine while we are loading scripts, no timeouts should fire + // until we are done. + const JsContext context(*jsEngine); // Set the preconfigured prefs auto preconfiguredPrefsObject = jsEngine->NewObject(); for (const auto& pref : params.preconfiguredPrefs) + { preconfiguredPrefsObject.SetProperty(pref.first, pref.second); + } jsEngine->SetGlobalProperty("_preconfiguredPrefs", preconfiguredPrefsObject); // Load adblockplus scripts - for(size_t i = 0; i < ArraySize(filterEngineJsFiles); ++i) - evaluateCallback(filterEngineJsFiles[i]); + for (const auto& filterEngineJsFile: filterEngineJsFiles) + evaluateCallback(filterEngineJsFile); } diff --git a/src/Platform.cpp b/src/Platform.cpp index 9c2dcbe3..f24af79f 100644 --- a/src/Platform.cpp +++ b/src/Platform.cpp @@ -19,7 +19,6 @@ #include #include #include -#include "JsContext.h" #include "DefaultTimer.h" #include "DefaultWebRequest.h" #include "DefaultFileSystem.h" @@ -68,7 +67,7 @@ JsEngine& Platform::GetJsEngine() return *jsEngine; } -JsEngine::EvaluateCallback Platform::GetEvaluateCallback() +std::function Platform::GetEvaluateCallback() { // GetEvaluateCallback() method assumes that jsEngine is already created return [this](const std::string& filename) @@ -76,8 +75,6 @@ JsEngine::EvaluateCallback Platform::GetEvaluateCallback() if (evaluatedJsSources.find(filename) != evaluatedJsSources.end()) return; //NO-OP, file was already evaluated - // Lock the JS engine while we are loading scripts - const JsContext context(*jsEngine); for (int i = 0; !jsSources[i].empty(); i += 2) if (jsSources[i] == filename) { diff --git a/src/Updater.cpp b/src/Updater.cpp index 9d96188e..c64b0881 100644 --- a/src/Updater.cpp +++ b/src/Updater.cpp @@ -16,13 +16,14 @@ */ #include -#include "Utils.h" -#include +#include "JsContext.h" using namespace AdblockPlus; -namespace { - static std::string updaterJsFiles[] = { +namespace +{ + const std::string updaterJsFiles[] = + { "compat.js", "info.js", "prefs.js", @@ -36,28 +37,14 @@ namespace { }; } -Updater::Updater(const JsEnginePtr& jsEngine, const JsEngine::EvaluateCallback& evaluateCallback) +Updater::Updater(const JsEnginePtr& jsEngine, const EvaluateCallback& evaluateCallback) : jsEngine(jsEngine), updateCheckId(0) { - // Hack to enable downloads from JS (see compat.js) - jsEngine->SetEventCallback("_isSubscriptionDownloadAllowed", [this](JsValueList&& params) - { - // param[0] - nullable string Prefs.allowed_connection_type - // param[1] - function(Boolean) - bool areArgumentsValid = params.size() == 2 && (params[0].IsNull() || params[0].IsString()) && params[1].IsFunction(); - assert(areArgumentsValid && "Invalid argument: there should be two args and the second one should be a function"); - if (!areArgumentsValid) - return; - params[1].Call(this->jsEngine->NewValue(true)); - }); - - // Set empty preconfigured prefs - auto preconfiguredPrefsObject = jsEngine->NewObject(); - jsEngine->SetGlobalProperty("_preconfiguredPrefs", preconfiguredPrefsObject); - - // Load adblockplus scripts - for(size_t i = 0; i < ArraySize(updaterJsFiles); ++i) - evaluateCallback(updaterJsFiles[i]); + // Lock the JS engine while we are loading scripts, no timeouts should fire + // until we are done. + const JsContext context(*jsEngine); + for (const auto& updaterJsFile: updaterJsFiles) + evaluateCallback(updaterJsFile); } void Updater::SetUpdateAvailableCallback(const Updater::UpdateAvailableCallback& callback) diff --git a/src/Utils.h b/src/Utils.h index a1fa90dc..5cd71a09 100644 --- a/src/Utils.h +++ b/src/Utils.h @@ -29,8 +29,6 @@ #include "JsError.h" -#define ArraySize(a) (sizeof(a) / sizeof(a[0])) - namespace AdblockPlus { namespace Utils From 511c94960dd16501aaec4f24dd8937db3eee04b5 Mon Sep 17 00:00:00 2001 From: Krystian Zlomek Date: Tue, 7 Aug 2018 20:09:12 +0200 Subject: [PATCH 6/8] Fixed thread safety and added more tests Now creation of Updater is thread safe. Added synchronization for evaluatedJsSources set. Added more unit tests. --- include/AdblockPlus/Platform.h | 1 + libadblockplus.gyp | 3 +- src/Platform.cpp | 11 +- test/UpdaterAndFilterEngineCreation.cpp | 172 ++++++++++++++++++++++++ 4 files changed, 183 insertions(+), 4 deletions(-) create mode 100644 test/UpdaterAndFilterEngineCreation.cpp diff --git a/include/AdblockPlus/Platform.h b/include/AdblockPlus/Platform.h index 977ba24e..c7272150 100644 --- a/include/AdblockPlus/Platform.h +++ b/include/AdblockPlus/Platform.h @@ -141,6 +141,7 @@ namespace AdblockPlus std::shared_future filterEngine; std::shared_ptr updater; std::set evaluatedJsSources; + std::mutex evaluatedJsSourcesMutex; std::function GetEvaluateCallback(); }; diff --git a/libadblockplus.gyp b/libadblockplus.gyp index d56d0101..b9192474 100644 --- a/libadblockplus.gyp +++ b/libadblockplus.gyp @@ -209,7 +209,8 @@ 'test/Prefs.cpp', 'test/ReferrerMapping.cpp', 'test/UpdateCheck.cpp', - 'test/WebRequest.cpp' + 'test/WebRequest.cpp', + 'test/UpdaterAndFilterEngineCreation.cpp' ], 'msvs_settings': { 'VCLinkerTool': { diff --git a/src/Platform.cpp b/src/Platform.cpp index f24af79f..79122c44 100644 --- a/src/Platform.cpp +++ b/src/Platform.cpp @@ -72,6 +72,7 @@ std::function Platform::GetEvaluateCallback() // GetEvaluateCallback() method assumes that jsEngine is already created return [this](const std::string& filename) { + std::lock_guard lock(evaluatedJsSourcesMutex); if (evaluatedJsSources.find(filename) != evaluatedJsSources.end()) return; //NO-OP, file was already evaluated @@ -119,11 +120,15 @@ FilterEngine& Platform::GetFilterEngine() Updater& Platform::GetUpdater() { - if (updater == nullptr) { - GetJsEngine(); // ensures that JsEngine is instantiated - updater = std::make_shared(jsEngine, GetEvaluateCallback()); + std::lock_guard lock(modulesMutex); + if (updater) + return *updater; } + GetJsEngine(); // ensures that JsEngine is instantiated + std::lock_guard lock(modulesMutex); + if (!updater) + updater = std::make_shared(jsEngine, GetEvaluateCallback()); return *updater; } diff --git a/test/UpdaterAndFilterEngineCreation.cpp b/test/UpdaterAndFilterEngineCreation.cpp new file mode 100644 index 00000000..c2e3dcde --- /dev/null +++ b/test/UpdaterAndFilterEngineCreation.cpp @@ -0,0 +1,172 @@ +/* + * This file is part of Adblock Plus , + * Copyright (C) 2006-present eyeo GmbH + * + * Adblock Plus is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 3 as + * published by the Free Software Foundation. + * + * Adblock Plus is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Adblock Plus. If not, see . + */ + + +#include +#include +#include +#include +#include + +#include "BaseJsTest.h" + +using namespace AdblockPlus; + +namespace +{ + class UpdaterAndFilterEngineCreationTest : public BaseJsTest + { + protected: + + static const size_t COUNT = 100; + const std::string PROP_NAME = "patternsbackupinterval"; + std::vector threadsVector; + Updater* updaterAddrArray[COUNT]; + FilterEngine* filterAddrArray[COUNT]; + DelayedWebRequest::SharedTasks webRequestTasks; + DelayedTimer::SharedTasks timerTasks; + + void SetUp() + { + LazyFileSystem* fileSystem; + ThrowingPlatformCreationParameters platformParams; + platformParams.logSystem.reset(new LazyLogSystem()); + platformParams.timer = DelayedTimer::New(timerTasks); + platformParams.fileSystem.reset(fileSystem = new LazyFileSystem()); + platformParams.webRequest = DelayedWebRequest::New(webRequestTasks); + platform.reset(new Platform(std::move(platformParams))); + threadsVector.reserve(COUNT); + std::uninitialized_fill(updaterAddrArray, updaterAddrArray + COUNT, nullptr); + std::uninitialized_fill(filterAddrArray, filterAddrArray + COUNT, nullptr); + } + + void CallGetUpdaterSimultaneously() + { + CallGetSimultaneously(true, false); + } + + void CallGetFilterEngineSimultaneously() + { + CallGetSimultaneously(false, true); + } + + void CallGetUpdaterAndGetFilterEngineSimultaneously() + { + CallGetSimultaneously(true, true); + } + + private: + + void CallGetSimultaneously(bool isUpdater, bool isFilterEngine) + { + for (size_t idx = 0; idx < COUNT; ++idx) + threadsVector.push_back( + std::thread([this, idx, isUpdater, isFilterEngine]() -> void { + Updater* updaterAddr = nullptr; + FilterEngine* filterEngineAddr = nullptr; + if (isUpdater && isFilterEngine) + { + // some randomization in order of calling gets + if (idx % 2) + { + updaterAddr = &(platform->GetUpdater()); + filterEngineAddr = &(platform->GetFilterEngine()); + } + else + { + filterEngineAddr = &(platform->GetFilterEngine()); + updaterAddr = &(platform->GetUpdater()); + } + } + else if (isUpdater) + updaterAddr = &(platform->GetUpdater()); + else if (isFilterEngine) + filterEngineAddr = &(platform->GetFilterEngine()); + + if (updaterAddr != nullptr) + updaterAddrArray[idx] = updaterAddr; + + if (filterEngineAddr != nullptr) + filterAddrArray[idx] = filterEngineAddr; + })); + + // join the threads + for (auto& th: threadsVector) + if (th.joinable()) + th.join(); + } + }; +} + +TEST_F(UpdaterAndFilterEngineCreationTest, TestFilterEngineSingleInstance) +{ + CallGetFilterEngineSimultaneously(); + FilterEngine* filterEngineAddr = filterAddrArray[0]; + for (size_t i = 1; i < COUNT; ++i) + ASSERT_EQ(filterEngineAddr, filterAddrArray[i]); +} + +TEST_F(UpdaterAndFilterEngineCreationTest, TestUpdaterSingleInstance) +{ + CallGetUpdaterSimultaneously(); + Updater* updaterAddr = updaterAddrArray[0]; + for (size_t i = 1; i < COUNT; ++i) + ASSERT_EQ(updaterAddr, updaterAddrArray[i]); +} + +TEST_F(UpdaterAndFilterEngineCreationTest, TestUpdaterAndFilterEngineCreationsDontCollide) +{ + CallGetUpdaterAndGetFilterEngineSimultaneously(); + ASSERT_EQ(filterAddrArray[0], filterAddrArray[COUNT - 1]); + ASSERT_EQ(updaterAddrArray[0], updaterAddrArray[COUNT - 1]); +} + +TEST_F(UpdaterAndFilterEngineCreationTest, TestUpdaterAndFilterEngineCreationOrder1) +{ + Updater& updater = platform->GetUpdater(); + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + FilterEngine& filterEngine = platform->GetFilterEngine(); + + int propFromUpdater = updater.GetPref(PROP_NAME).AsInt(); + int propFromFilterEngine = filterEngine.GetPref(PROP_NAME).AsInt(); + ASSERT_EQ(propFromUpdater, propFromFilterEngine); + + int newPropValue = 8; + updater.SetPref(PROP_NAME, GetJsEngine().NewValue(newPropValue)); + propFromUpdater = updater.GetPref(PROP_NAME).AsInt(); + propFromFilterEngine = filterEngine.GetPref(PROP_NAME).AsInt(); + ASSERT_EQ(propFromUpdater, newPropValue); + ASSERT_EQ(propFromFilterEngine, newPropValue); +} + +TEST_F(UpdaterAndFilterEngineCreationTest, TestUpdaterAndFilterEngineCreationOrder2) +{ + FilterEngine& filterEngine = platform->GetFilterEngine(); + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + Updater& updater = platform->GetUpdater(); + + int propFromFilterEngine = filterEngine.GetPref(PROP_NAME).AsInt(); + int propFromUpdater = updater.GetPref(PROP_NAME).AsInt(); + ASSERT_EQ(propFromUpdater, propFromFilterEngine); + + int newPropValue = 18; + filterEngine.SetPref(PROP_NAME, GetJsEngine().NewValue(newPropValue)); + propFromFilterEngine = filterEngine.GetPref(PROP_NAME).AsInt(); + propFromUpdater = updater.GetPref(PROP_NAME).AsInt(); + ASSERT_EQ(propFromUpdater, newPropValue); + ASSERT_EQ(propFromFilterEngine, newPropValue); +} From 1ae9556fa49df06cea43358f992773dbdc5ed860 Mon Sep 17 00:00:00 2001 From: Krystian Zlomek Date: Thu, 9 Aug 2018 16:00:41 +0200 Subject: [PATCH 7/8] Reworked last commit applying review comments --- test/UpdaterAndFilterEngineCreation.cpp | 74 ++++++++++--------------- 1 file changed, 30 insertions(+), 44 deletions(-) diff --git a/test/UpdaterAndFilterEngineCreation.cpp b/test/UpdaterAndFilterEngineCreation.cpp index c2e3dcde..38816ecc 100644 --- a/test/UpdaterAndFilterEngineCreation.cpp +++ b/test/UpdaterAndFilterEngineCreation.cpp @@ -15,12 +15,11 @@ * along with Adblock Plus. If not, see . */ - -#include -#include -#include #include #include +#include + +#include #include "BaseJsTest.h" @@ -34,9 +33,8 @@ namespace static const size_t COUNT = 100; const std::string PROP_NAME = "patternsbackupinterval"; - std::vector threadsVector; - Updater* updaterAddrArray[COUNT]; - FilterEngine* filterAddrArray[COUNT]; + std::array updaterAddrArray; + std::array filterAddrArray; DelayedWebRequest::SharedTasks webRequestTasks; DelayedTimer::SharedTasks timerTasks; @@ -49,9 +47,8 @@ namespace platformParams.fileSystem.reset(fileSystem = new LazyFileSystem()); platformParams.webRequest = DelayedWebRequest::New(webRequestTasks); platform.reset(new Platform(std::move(platformParams))); - threadsVector.reserve(COUNT); - std::uninitialized_fill(updaterAddrArray, updaterAddrArray + COUNT, nullptr); - std::uninitialized_fill(filterAddrArray, filterAddrArray + COUNT, nullptr); + std::uninitialized_fill(updaterAddrArray.begin(), updaterAddrArray.end(), nullptr); + std::uninitialized_fill(filterAddrArray.begin(), filterAddrArray.end(), nullptr); } void CallGetUpdaterSimultaneously() @@ -73,41 +70,28 @@ namespace void CallGetSimultaneously(bool isUpdater, bool isFilterEngine) { + AsyncExecutor asyncExecutor; for (size_t idx = 0; idx < COUNT; ++idx) - threadsVector.push_back( - std::thread([this, idx, isUpdater, isFilterEngine]() -> void { - Updater* updaterAddr = nullptr; - FilterEngine* filterEngineAddr = nullptr; - if (isUpdater && isFilterEngine) + asyncExecutor.Dispatch(([this, idx, isUpdater, isFilterEngine]() -> void { + if (isUpdater && isFilterEngine) + { + // some randomization in order of calling gets + if (idx % 2) + { + updaterAddrArray[idx] = &(platform->GetUpdater()); + filterAddrArray[idx] = &(platform->GetFilterEngine()); + } + else { - // some randomization in order of calling gets - if (idx % 2) - { - updaterAddr = &(platform->GetUpdater()); - filterEngineAddr = &(platform->GetFilterEngine()); - } - else - { - filterEngineAddr = &(platform->GetFilterEngine()); - updaterAddr = &(platform->GetUpdater()); - } + filterAddrArray[idx] = &(platform->GetFilterEngine()); + updaterAddrArray[idx] = &(platform->GetUpdater()); } - else if (isUpdater) - updaterAddr = &(platform->GetUpdater()); - else if (isFilterEngine) - filterEngineAddr = &(platform->GetFilterEngine()); - - if (updaterAddr != nullptr) - updaterAddrArray[idx] = updaterAddr; - - if (filterEngineAddr != nullptr) - filterAddrArray[idx] = filterEngineAddr; - })); - - // join the threads - for (auto& th: threadsVector) - if (th.joinable()) - th.join(); + } + else if (isUpdater) + updaterAddrArray[idx] = &(platform->GetUpdater()); + else if (isFilterEngine) + filterAddrArray[idx] = &(platform->GetFilterEngine()); + })); } }; } @@ -116,6 +100,7 @@ TEST_F(UpdaterAndFilterEngineCreationTest, TestFilterEngineSingleInstance) { CallGetFilterEngineSimultaneously(); FilterEngine* filterEngineAddr = filterAddrArray[0]; + ASSERT_NE(filterEngineAddr, nullptr); for (size_t i = 1; i < COUNT; ++i) ASSERT_EQ(filterEngineAddr, filterAddrArray[i]); } @@ -124,6 +109,7 @@ TEST_F(UpdaterAndFilterEngineCreationTest, TestUpdaterSingleInstance) { CallGetUpdaterSimultaneously(); Updater* updaterAddr = updaterAddrArray[0]; + ASSERT_NE(updaterAddr, nullptr); for (size_t i = 1; i < COUNT; ++i) ASSERT_EQ(updaterAddr, updaterAddrArray[i]); } @@ -131,14 +117,15 @@ TEST_F(UpdaterAndFilterEngineCreationTest, TestUpdaterSingleInstance) TEST_F(UpdaterAndFilterEngineCreationTest, TestUpdaterAndFilterEngineCreationsDontCollide) { CallGetUpdaterAndGetFilterEngineSimultaneously(); + ASSERT_NE(filterAddrArray[0], nullptr); ASSERT_EQ(filterAddrArray[0], filterAddrArray[COUNT - 1]); + ASSERT_NE(updaterAddrArray[0], nullptr); ASSERT_EQ(updaterAddrArray[0], updaterAddrArray[COUNT - 1]); } TEST_F(UpdaterAndFilterEngineCreationTest, TestUpdaterAndFilterEngineCreationOrder1) { Updater& updater = platform->GetUpdater(); - std::this_thread::sleep_for(std::chrono::milliseconds(100)); FilterEngine& filterEngine = platform->GetFilterEngine(); int propFromUpdater = updater.GetPref(PROP_NAME).AsInt(); @@ -156,7 +143,6 @@ TEST_F(UpdaterAndFilterEngineCreationTest, TestUpdaterAndFilterEngineCreationOrd TEST_F(UpdaterAndFilterEngineCreationTest, TestUpdaterAndFilterEngineCreationOrder2) { FilterEngine& filterEngine = platform->GetFilterEngine(); - std::this_thread::sleep_for(std::chrono::milliseconds(100)); Updater& updater = platform->GetUpdater(); int propFromFilterEngine = filterEngine.GetPref(PROP_NAME).AsInt(); From 956d6202e106427b4aacb74b73999f02d10f06b5 Mon Sep 17 00:00:00 2001 From: Krystian Zlomek Date: Fri, 10 Aug 2018 15:40:28 +0200 Subject: [PATCH 8/8] Updated UpdaterAndFilterEngineCreationTest to match coding convention --- test/UpdaterAndFilterEngineCreation.cpp | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/test/UpdaterAndFilterEngineCreation.cpp b/test/UpdaterAndFilterEngineCreation.cpp index 38816ecc..cb42543b 100644 --- a/test/UpdaterAndFilterEngineCreation.cpp +++ b/test/UpdaterAndFilterEngineCreation.cpp @@ -100,7 +100,7 @@ TEST_F(UpdaterAndFilterEngineCreationTest, TestFilterEngineSingleInstance) { CallGetFilterEngineSimultaneously(); FilterEngine* filterEngineAddr = filterAddrArray[0]; - ASSERT_NE(filterEngineAddr, nullptr); + EXPECT_NE(nullptr, filterEngineAddr); for (size_t i = 1; i < COUNT; ++i) ASSERT_EQ(filterEngineAddr, filterAddrArray[i]); } @@ -109,7 +109,7 @@ TEST_F(UpdaterAndFilterEngineCreationTest, TestUpdaterSingleInstance) { CallGetUpdaterSimultaneously(); Updater* updaterAddr = updaterAddrArray[0]; - ASSERT_NE(updaterAddr, nullptr); + EXPECT_NE(nullptr, updaterAddr); for (size_t i = 1; i < COUNT; ++i) ASSERT_EQ(updaterAddr, updaterAddrArray[i]); } @@ -117,10 +117,10 @@ TEST_F(UpdaterAndFilterEngineCreationTest, TestUpdaterSingleInstance) TEST_F(UpdaterAndFilterEngineCreationTest, TestUpdaterAndFilterEngineCreationsDontCollide) { CallGetUpdaterAndGetFilterEngineSimultaneously(); - ASSERT_NE(filterAddrArray[0], nullptr); - ASSERT_EQ(filterAddrArray[0], filterAddrArray[COUNT - 1]); - ASSERT_NE(updaterAddrArray[0], nullptr); - ASSERT_EQ(updaterAddrArray[0], updaterAddrArray[COUNT - 1]); + EXPECT_NE(nullptr, filterAddrArray[0]); + EXPECT_NE(nullptr, updaterAddrArray[0]); + EXPECT_EQ(filterAddrArray[0], filterAddrArray[COUNT - 1]); + EXPECT_EQ(updaterAddrArray[0], updaterAddrArray[COUNT - 1]); } TEST_F(UpdaterAndFilterEngineCreationTest, TestUpdaterAndFilterEngineCreationOrder1) @@ -130,14 +130,14 @@ TEST_F(UpdaterAndFilterEngineCreationTest, TestUpdaterAndFilterEngineCreationOrd int propFromUpdater = updater.GetPref(PROP_NAME).AsInt(); int propFromFilterEngine = filterEngine.GetPref(PROP_NAME).AsInt(); - ASSERT_EQ(propFromUpdater, propFromFilterEngine); + EXPECT_EQ(propFromUpdater, propFromFilterEngine); int newPropValue = 8; updater.SetPref(PROP_NAME, GetJsEngine().NewValue(newPropValue)); propFromUpdater = updater.GetPref(PROP_NAME).AsInt(); propFromFilterEngine = filterEngine.GetPref(PROP_NAME).AsInt(); - ASSERT_EQ(propFromUpdater, newPropValue); - ASSERT_EQ(propFromFilterEngine, newPropValue); + EXPECT_EQ(newPropValue, propFromUpdater); + EXPECT_EQ(newPropValue, propFromFilterEngine); } TEST_F(UpdaterAndFilterEngineCreationTest, TestUpdaterAndFilterEngineCreationOrder2) @@ -147,12 +147,12 @@ TEST_F(UpdaterAndFilterEngineCreationTest, TestUpdaterAndFilterEngineCreationOrd int propFromFilterEngine = filterEngine.GetPref(PROP_NAME).AsInt(); int propFromUpdater = updater.GetPref(PROP_NAME).AsInt(); - ASSERT_EQ(propFromUpdater, propFromFilterEngine); + EXPECT_EQ(propFromUpdater, propFromFilterEngine); int newPropValue = 18; filterEngine.SetPref(PROP_NAME, GetJsEngine().NewValue(newPropValue)); propFromFilterEngine = filterEngine.GetPref(PROP_NAME).AsInt(); propFromUpdater = updater.GetPref(PROP_NAME).AsInt(); - ASSERT_EQ(propFromUpdater, newPropValue); - ASSERT_EQ(propFromFilterEngine, newPropValue); + EXPECT_EQ(newPropValue, propFromUpdater); + EXPECT_EQ(newPropValue, propFromFilterEngine); }