diff options
Diffstat (limited to 'chromium/extensions/browser/api')
100 files changed, 1727 insertions, 1043 deletions
diff --git a/chromium/extensions/browser/api/DEPS b/chromium/extensions/browser/api/DEPS index a8608a8aa09..83b09650b55 100644 --- a/chromium/extensions/browser/api/DEPS +++ b/chromium/extensions/browser/api/DEPS @@ -1,6 +1,8 @@ include_rules = [ "+components/device_event_log", + "+components/ukm", "+services/device/public", + "+services/metrics/public/cpp", "+storage/browser/file_system", "+storage/common/file_system", "+third_party/openscreen/src/cast/common/certificate/proto", diff --git a/chromium/extensions/browser/api/alarms/BUILD.gn b/chromium/extensions/browser/api/alarms/BUILD.gn index e6c7666c9e2..190d1a2baa5 100644 --- a/chromium/extensions/browser/api/alarms/BUILD.gn +++ b/chromium/extensions/browser/api/alarms/BUILD.gn @@ -17,7 +17,10 @@ source_set("alarms") { "alarms_api_constants.h", ] - deps = [ "//extensions/common/api" ] + deps = [ + "//base/util/values:values_util", + "//extensions/common/api", + ] public_deps = [ "//extensions/browser:browser_sources" ] } diff --git a/chromium/extensions/browser/api/alarms/alarm_manager.cc b/chromium/extensions/browser/api/alarms/alarm_manager.cc index 2b3e1ac4afe..07edcf02925 100644 --- a/chromium/extensions/browser/api/alarms/alarm_manager.cc +++ b/chromium/extensions/browser/api/alarms/alarm_manager.cc @@ -15,7 +15,7 @@ #include "base/time/clock.h" #include "base/time/default_clock.h" #include "base/time/time.h" -#include "base/value_conversions.h" +#include "base/util/values/values_util.h" #include "base/values.h" #include "extensions/browser/api/alarms/alarms_api_constants.h" #include "extensions/browser/event_router.h" @@ -76,11 +76,12 @@ AlarmManager::AlarmList AlarmsFromValue(const std::string extension_id, std::unique_ptr<Alarm> alarm(new Alarm()); if (list->GetDictionary(i, &alarm_dict) && alarms::Alarm::Populate(*alarm_dict, alarm->js_alarm.get())) { - const base::Value* time_value = nullptr; - if (alarm_dict->Get(kAlarmGranularity, &time_value)) { - // It's okay to ignore the failure since we have minimum granularity. - ignore_result( - base::GetValueAsTimeDelta(*time_value, &alarm->granularity)); + base::Optional<base::TimeDelta> delta = + util::ValueToTimeDelta(alarm_dict->FindKey(kAlarmGranularity)); + if (delta) { + alarm->granularity = *delta; + // No else branch. It's okay to ignore the failure since we have + // minimum granularity. } alarm->minimum_granularity = base::TimeDelta::FromSecondsD( (is_unpacked ? alarms_api_constants::kDevDelayMinimum @@ -101,7 +102,7 @@ std::unique_ptr<base::ListValue> AlarmsToValue( std::unique_ptr<base::DictionaryValue> alarm = alarms[i]->js_alarm->ToValue(); alarm->SetKey(kAlarmGranularity, - base::CreateTimeDeltaValue(alarms[i]->granularity)); + util::TimeDeltaToValue(alarms[i]->granularity)); list->Append(std::move(alarm)); } return list; diff --git a/chromium/extensions/browser/api/async_api_function.cc b/chromium/extensions/browser/api/async_api_function.cc index 5e5441fa6b0..975673903b8 100644 --- a/chromium/extensions/browser/api/async_api_function.cc +++ b/chromium/extensions/browser/api/async_api_function.cc @@ -5,7 +5,6 @@ #include "extensions/browser/api/async_api_function.h" #include "base/bind.h" -#include "base/task/post_task.h" #include "content/public/browser/browser_task_traits.h" #include "content/public/browser/browser_thread.h" #include "extensions/browser/extension_system.h" @@ -16,8 +15,7 @@ namespace extensions { // AsyncApiFunction AsyncApiFunction::AsyncApiFunction() - : work_task_runner_( - base::CreateSingleThreadTaskRunner({BrowserThread::IO})) {} + : work_task_runner_(content::GetIOThreadTaskRunner({})) {} AsyncApiFunction::~AsyncApiFunction() {} @@ -58,9 +56,8 @@ ExtensionFunction::ResponseAction AsyncApiFunction::Run() { void AsyncApiFunction::AsyncWorkCompleted() { if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) { - bool rv = base::PostTask( - FROM_HERE, {BrowserThread::UI}, - base::BindOnce(&AsyncApiFunction::RespondOnUIThread, this)); + bool rv = content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&AsyncApiFunction::RespondOnUIThread, this)); DCHECK(rv); } else { SendResponse(Respond()); diff --git a/chromium/extensions/browser/api/audio/audio_apitest_chromeos.cc b/chromium/extensions/browser/api/audio/audio_apitest_chromeos.cc index 5b0d7ba30f5..45900d38efb 100644 --- a/chromium/extensions/browser/api/audio/audio_apitest_chromeos.cc +++ b/chromium/extensions/browser/api/audio/audio_apitest_chromeos.cc @@ -49,6 +49,9 @@ struct AudioNodeInfo { const char* const name; }; +const uint32_t kInputMaxSupportedChannels = 1; +const uint32_t kOutputMaxSupportedChannels = 2; + const AudioNodeInfo kJabraSpeaker1 = { false, kJabraSpeaker1Id, kJabraSpeaker1StableDeviceId, "Jabra Speaker", "USB", "Jabra Speaker 1"}; @@ -74,12 +77,14 @@ const AudioNodeInfo kUSBCameraMic = { "Webcam Mic", "USB", "Logitech Webcam"}; AudioNode CreateAudioNode(const AudioNodeInfo& info, int version) { - return AudioNode(info.is_input, info.id, version == 2, - // stable_device_id_v1: - info.stable_id, - // stable_device_id_v2: - version == 2 ? info.stable_id ^ 0xFFFF : 0, info.device_name, - info.type, info.name, false, 0); + return AudioNode( + info.is_input, info.id, version == 2, + // stable_device_id_v1: + info.stable_id, + // stable_device_id_v2: + version == 2 ? info.stable_id ^ 0xFFFF : 0, info.device_name, info.type, + info.name, false, 0, + info.is_input ? kInputMaxSupportedChannels : kOutputMaxSupportedChannels); } class AudioApiTest : public ShellApiTest { diff --git a/chromium/extensions/browser/api/audio/audio_service_chromeos.cc b/chromium/extensions/browser/api/audio/audio_service_chromeos.cc index 40d0a4cb457..b292104a6cb 100644 --- a/chromium/extensions/browser/api/audio/audio_service_chromeos.cc +++ b/chromium/extensions/browser/api/audio/audio_service_chromeos.cc @@ -362,8 +362,8 @@ AudioDeviceInfo AudioServiceImpl::ToAudioDeviceInfo( info.is_active = device.active; info.level = device.is_input - ? cras_audio_handler_->GetOutputVolumePercentForDevice(device.id) - : cras_audio_handler_->GetInputGainPercentForDevice(device.id); + ? cras_audio_handler_->GetInputGainPercentForDevice(device.id) + : cras_audio_handler_->GetOutputVolumePercentForDevice(device.id); info.stable_device_id = std::make_unique<std::string>( id_calculator_->GetStableDeviceId(device.stable_device_id)); diff --git a/chromium/extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc b/chromium/extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc index 95f22356ef9..31a5b61c141 100644 --- a/chromium/extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc +++ b/chromium/extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc @@ -669,7 +669,7 @@ void BluetoothLowEnergyEventRouter::WriteCharacteristicValue( return; } - characteristic->WriteRemoteCharacteristic( + characteristic->DeprecatedWriteRemoteCharacteristic( value, callback, base::BindOnce(&BluetoothLowEnergyEventRouter::OnError, weak_ptr_factory_.GetWeakPtr(), error_callback)); diff --git a/chromium/extensions/browser/api/bluetooth_socket/bluetooth_socket_event_dispatcher.cc b/chromium/extensions/browser/api/bluetooth_socket/bluetooth_socket_event_dispatcher.cc index c8df92dfbcf..48b796db4ef 100644 --- a/chromium/extensions/browser/api/bluetooth_socket/bluetooth_socket_event_dispatcher.cc +++ b/chromium/extensions/browser/api/bluetooth_socket/bluetooth_socket_event_dispatcher.cc @@ -351,8 +351,8 @@ void BluetoothSocketEventDispatcher::PostEvent(const SocketParams& params, std::unique_ptr<Event> event) { DCHECK_CURRENTLY_ON(params.thread_id); - base::PostTask(FROM_HERE, {BrowserThread::UI}, - base::BindOnce(&DispatchEvent, params.browser_context_id, + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&DispatchEvent, params.browser_context_id, params.extension_id, std::move(event))); } diff --git a/chromium/extensions/browser/api/cast_channel/cast_channel_api.cc b/chromium/extensions/browser/api/cast_channel/cast_channel_api.cc index dc1f5bb8c59..5bd2b5daa9f 100644 --- a/chromium/extensions/browser/api/cast_channel/cast_channel_api.cc +++ b/chromium/extensions/browser/api/cast_channel/cast_channel_api.cc @@ -17,7 +17,6 @@ #include "base/json/json_writer.h" #include "base/lazy_instance.h" #include "base/strings/string_number_conversions.h" -#include "base/task/post_task.h" #include "base/values.h" #include "components/cast_channel/cast_channel_enum.h" #include "components/cast_channel/cast_message_util.h" @@ -515,8 +514,8 @@ void CastChannelAPI::CastMessageHandler::OnError( OnError::Create(channel_info, error_info); std::unique_ptr<Event> event(new Event( events::CAST_CHANNEL_ON_ERROR, OnError::kEventName, std::move(results))); - base::PostTask(FROM_HERE, {BrowserThread::UI}, - base::BindOnce(ui_dispatch_cb_, std::move(event))); + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(ui_dispatch_cb_, std::move(event))); } void CastChannelAPI::CastMessageHandler::OnMessage( @@ -536,8 +535,8 @@ void CastChannelAPI::CastMessageHandler::OnMessage( std::unique_ptr<Event> event(new Event(events::CAST_CHANNEL_ON_MESSAGE, OnMessage::kEventName, std::move(results))); - base::PostTask(FROM_HERE, {BrowserThread::UI}, - base::BindOnce(ui_dispatch_cb_, std::move(event))); + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(ui_dispatch_cb_, std::move(event))); } } // namespace extensions diff --git a/chromium/extensions/browser/api/cast_channel/cast_channel_apitest.cc b/chromium/extensions/browser/api/cast_channel/cast_channel_apitest.cc index bcf1c80d54f..8b4acb1cca4 100644 --- a/chromium/extensions/browser/api/cast_channel/cast_channel_apitest.cc +++ b/chromium/extensions/browser/api/cast_channel/cast_channel_apitest.cc @@ -8,7 +8,6 @@ #include "base/command_line.h" #include "base/files/file_path.h" #include "base/memory/ptr_util.h" -#include "base/task/post_task.h" #include "base/test/scoped_feature_list.h" #include "base/timer/mock_timer.h" #include "build/build_config.h" @@ -100,7 +99,11 @@ class CastChannelAPITest : public extensions::ExtensionApiTest { // Stub out DualMediaSinkService so it does not interfere with the test. media_router::DualMediaSinkService::SetInstanceForTest( new media_router::NoopDualMediaSinkService()); - feature_list_.InitAndDisableFeature(media_router::kDialMediaRouteProvider); + // The Media Route Providers must be disabled because they rely on the + // presence of a valid DualMediaSinkService. + feature_list_.InitWithFeatures( + {}, /* disabled_features */ {media_router::kDialMediaRouteProvider, + media_router::kCastMediaRouteProvider}); extensions::ExtensionApiTest::SetUp(); } @@ -204,8 +207,8 @@ class CastChannelAPITest : public extensions::ExtensionApiTest { protected: void CallOnMessage(const std::string& message) { - base::PostTask(FROM_HERE, {content::BrowserThread::IO}, - base::BindOnce(&CastChannelAPITest::DoCallOnMessage, + content::GetIOThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&CastChannelAPITest::DoCallOnMessage, base::Unretained(this), GetApi(), mock_cast_socket_, message)); } @@ -220,8 +223,8 @@ class CastChannelAPITest : public extensions::ExtensionApiTest { // Fires a timer on the IO thread. void FireTimeout() { - base::PostTask(FROM_HERE, {content::BrowserThread::IO}, - base::BindOnce(&CastChannelAPITest::DoFireTimeout, + content::GetIOThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&CastChannelAPITest::DoFireTimeout, base::Unretained(this), mock_cast_socket_)); } @@ -255,8 +258,8 @@ class CastChannelAPITest : public extensions::ExtensionApiTest { }; ACTION_P2(InvokeObserverOnError, api_test, cast_socket_service) { - base::PostTask(FROM_HERE, {content::BrowserThread::IO}, - base::BindOnce(&CastChannelAPITest::DoCallOnError, + content::GetIOThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&CastChannelAPITest::DoCallOnError, base::Unretained(api_test), base::Unretained(cast_socket_service))); } diff --git a/chromium/extensions/browser/api/crash_report_private/crash_report_private_api.cc b/chromium/extensions/browser/api/crash_report_private/crash_report_private_api.cc index ed996bc1345..112afa3535f 100644 --- a/chromium/extensions/browser/api/crash_report_private/crash_report_private_api.cc +++ b/chromium/extensions/browser/api/crash_report_private/crash_report_private_api.cc @@ -7,7 +7,6 @@ #include "base/strings/strcat.h" #include "base/strings/stringprintf.h" #include "base/system/sys_info.h" -#include "base/task/post_task.h" #include "base/task/task_traits.h" #include "base/task/thread_pool.h" #include "base/time/default_clock.h" @@ -216,8 +215,8 @@ ExtensionFunction::ResponseAction CrashReportPrivateReportErrorFunction::Run() { // Consent checking may be blocking, so do it on a separate thread to avoid // blocking the UI thread. - PostTaskAndReplyWithResult( - FROM_HERE, {base::ThreadPool(), base::MayBlock()}, + base::ThreadPool::PostTaskAndReplyWithResult( + FROM_HERE, {base::MayBlock()}, base::BindOnce(&crash_reporter::GetClientCollectStatsConsent), base::BindOnce( &CrashReportPrivateReportErrorFunction::OnConsentCheckCompleted, this, @@ -240,7 +239,7 @@ void CrashReportPrivateReportErrorFunction::OnConsentCheckCompleted( ->GetURLLoaderFactoryForBrowserProcess(); // Don't anonymize the report on the UI thread as it can take some time. - PostTaskAndReplyWithResult( + base::ThreadPool::PostTaskAndReplyWithResult( FROM_HERE, base::BindOnce(&AnonymizeErrorMessage, info.message), base::BindOnce( &ReportJavaScriptError, std::move(loader_factory), std::move(info), diff --git a/chromium/extensions/browser/api/declarative/deduping_factory.h b/chromium/extensions/browser/api/declarative/deduping_factory.h index cb96e0f75fb..67c7033353a 100644 --- a/chromium/extensions/browser/api/declarative/deduping_factory.h +++ b/chromium/extensions/browser/api/declarative/deduping_factory.h @@ -12,8 +12,8 @@ #include <unordered_map> #include <unordered_set> +#include "base/check.h" #include "base/compiler_specific.h" -#include "base/logging.h" #include "base/macros.h" #include "base/memory/ref_counted.h" #include "base/stl_util.h" diff --git a/chromium/extensions/browser/api/declarative/rules_registry.cc b/chromium/extensions/browser/api/declarative/rules_registry.cc index 30b767cdb39..fd6feb20da6 100644 --- a/chromium/extensions/browser/api/declarative/rules_registry.cc +++ b/chromium/extensions/browser/api/declarative/rules_registry.cc @@ -12,7 +12,6 @@ #include "base/stl_util.h" #include "base/strings/stringprintf.h" #include "base/strings/utf_string_conversions.h" -#include "base/task/post_task.h" #include "base/time/time.h" #include "base/values.h" #include "content/public/browser/browser_task_traits.h" @@ -348,8 +347,8 @@ void RulesRegistry::ProcessChangedRules(const std::string& extension_id) { std::vector<const api::events::Rule*> new_rules; GetRules(extension_id, &rules_, &new_rules); - base::PostTask( - FROM_HERE, {content::BrowserThread::UI}, + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&RulesCacheDelegate::UpdateRules, cache_delegate_, extension_id, RulesToValue(new_rules))); } diff --git a/chromium/extensions/browser/api/declarative/rules_registry_service.cc b/chromium/extensions/browser/api/declarative/rules_registry_service.cc index 9e76ebf145c..19f4de09c21 100644 --- a/chromium/extensions/browser/api/declarative/rules_registry_service.cc +++ b/chromium/extensions/browser/api/declarative/rules_registry_service.cc @@ -42,7 +42,7 @@ const int RulesRegistryService::kInvalidRulesRegistryID = -1; RulesRegistryService::RulesRegistryService(content::BrowserContext* context) : current_rules_registry_id_(kDefaultRulesRegistryID), - content_rules_registry_(NULL), + content_rules_registry_(nullptr), browser_context_(context) { if (browser_context_) { extension_registry_observer_.Add(ExtensionRegistry::Get(browser_context_)); diff --git a/chromium/extensions/browser/api/declarative_net_request/composite_matcher.cc b/chromium/extensions/browser/api/declarative_net_request/composite_matcher.cc index 8240f453bb0..70dae8c8409 100644 --- a/chromium/extensions/browser/api/declarative_net_request/composite_matcher.cc +++ b/chromium/extensions/browser/api/declarative_net_request/composite_matcher.cc @@ -5,6 +5,7 @@ #include "extensions/browser/api/declarative_net_request/composite_matcher.h" #include <algorithm> +#include <functional> #include <iterator> #include <set> #include <utility> @@ -73,29 +74,32 @@ CompositeMatcher::CompositeMatcher(MatcherList matchers) CompositeMatcher::~CompositeMatcher() = default; -CompositeMatcher::MatcherList CompositeMatcher::GetAndResetMatchers() { - MatcherList result; - std::swap(result, matchers_); - OnMatchersModified(); - return result; +void CompositeMatcher::AddOrUpdateRuleset( + std::unique_ptr<RulesetMatcher> matcher) { + MatcherList matchers; + matchers.push_back(std::move(matcher)); + AddOrUpdateRulesets(std::move(matchers)); } -void CompositeMatcher::SetMatchers(MatcherList matchers) { - matchers_ = std::move(matchers); +void CompositeMatcher::AddOrUpdateRulesets(MatcherList matchers) { + std::set<RulesetID> ids_to_remove; + for (const auto& matcher : matchers) + ids_to_remove.insert(matcher->id()); + + RemoveRulesetsWithIDs(ids_to_remove); + matchers_.insert(matchers_.end(), std::make_move_iterator(matchers.begin()), + std::make_move_iterator(matchers.end())); OnMatchersModified(); } -void CompositeMatcher::AddOrUpdateRuleset( - std::unique_ptr<RulesetMatcher> new_matcher) { - // A linear search is ok since the number of rulesets per extension is - // expected to be quite small. - base::EraseIf(matchers_, - [&new_matcher](const std::unique_ptr<RulesetMatcher>& matcher) { - return matcher->id() == new_matcher->id(); - }); - matchers_.push_back(std::move(new_matcher)); +void CompositeMatcher::RemoveRulesetsWithIDs(const std::set<RulesetID>& ids) { + size_t erased_count = base::EraseIf( + matchers_, [&ids](const std::unique_ptr<RulesetMatcher>& matcher) { + return base::Contains(ids, matcher->id()); + }); - OnMatchersModified(); + if (erased_count > 0) + OnMatchersModified(); } std::set<RulesetID> CompositeMatcher::ComputeStaticRulesetIDs() const { @@ -117,11 +121,21 @@ ActionInfo CompositeMatcher::GetBeforeRequestAction( bool notify_request_withheld = false; base::Optional<RequestAction> final_action; + + // The priority of the highest priority matching allow or allowAllRequests + // rule within this matcher, or base::nullopt otherwise. + base::Optional<uint64_t> max_allow_rule_priority; + for (const auto& matcher : matchers_) { base::Optional<RequestAction> action = matcher->GetBeforeRequestAction(params); - params.allow_rule_cache[matcher.get()] = - action && action->IsAllowOrAllowAllRequests(); + + if (action && action->IsAllowOrAllowAllRequests()) { + max_allow_rule_priority = + max_allow_rule_priority + ? std::max(*max_allow_rule_priority, action->index_priority) + : action->index_priority; + } if (action && action->type == RequestAction::Type::REDIRECT) { // Redirecting requires host permissions. @@ -139,6 +153,8 @@ ActionInfo CompositeMatcher::GetBeforeRequestAction( GetMaxPriorityAction(std::move(final_action), std::move(action)); } + params.allow_rule_max_priority[this] = max_allow_rule_priority; + if (final_action) return ActionInfo(std::move(final_action), false); return ActionInfo(base::nullopt, notify_request_withheld); @@ -147,16 +163,19 @@ ActionInfo CompositeMatcher::GetBeforeRequestAction( std::vector<RequestAction> CompositeMatcher::GetModifyHeadersActions( const RequestParams& params) const { std::vector<RequestAction> modify_headers_actions; + DCHECK(params.allow_rule_max_priority.contains(this)); - for (const auto& matcher : matchers_) { - // TODO(crbug.com/947591): An allow or allowAllRequests rule should override - // all equal or lower priority modifyHeaders rules specified by |matcher|. - DCHECK(params.allow_rule_cache.contains(matcher.get())); - if (params.allow_rule_cache[matcher.get()]) - return std::vector<RequestAction>(); + // The priority of the highest priority matching allow or allowAllRequests + // rule within this matcher, or base::nullopt if no such rule exists. + base::Optional<uint64_t> max_allow_rule_priority = + params.allow_rule_max_priority[this]; + for (const auto& matcher : matchers_) { + // Plumb |max_allow_rule_priority| into GetModifyHeadersActions so that + // modifyHeaders rules with priorities less than or equal to the highest + // priority matching allow/allowAllRequests rule are ignored. std::vector<RequestAction> actions_for_matcher = - matcher->GetModifyHeadersActions(params); + matcher->GetModifyHeadersActions(params, max_allow_rule_priority); modify_headers_actions.insert( modify_headers_actions.end(), @@ -166,9 +185,7 @@ std::vector<RequestAction> CompositeMatcher::GetModifyHeadersActions( // Sort |modify_headers_actions| in descending order of priority. std::sort(modify_headers_actions.begin(), modify_headers_actions.end(), - [](const RequestAction& lhs, const RequestAction& rhs) { - return lhs.index_priority > rhs.index_priority; - }); + std::greater<>()); return modify_headers_actions; } diff --git a/chromium/extensions/browser/api/declarative_net_request/composite_matcher.h b/chromium/extensions/browser/api/declarative_net_request/composite_matcher.h index 0fae10217a6..85f66bc49eb 100644 --- a/chromium/extensions/browser/api/declarative_net_request/composite_matcher.h +++ b/chromium/extensions/browser/api/declarative_net_request/composite_matcher.h @@ -25,8 +25,7 @@ namespace declarative_net_request { struct RequestAction; -// Per extension instance which manages the different rulesets for an extension -// while respecting their priorities. +// Per extension instance which manages the different rulesets for an extension. class CompositeMatcher { public: struct ActionInfo { @@ -48,21 +47,21 @@ class CompositeMatcher { using MatcherList = std::vector<std::unique_ptr<RulesetMatcher>>; - // Each RulesetMatcher should have a distinct ID and priority. + // Each RulesetMatcher should have a distinct RulesetID. explicit CompositeMatcher(MatcherList matchers); ~CompositeMatcher(); const MatcherList& matchers() const { return matchers_; } - // Returns the set of matchers and resets |matchers_| to an empty vector. - MatcherList GetAndResetMatchers(); + // Inserts |matcher|, overwriting any existing RulesetMatcher with the same + // RulesetID. + void AddOrUpdateRuleset(std::unique_ptr<RulesetMatcher> matcher); - // Updates the set of matchers. IDs for all the |matchers| must be unique. - void SetMatchers(MatcherList matchers); + // Inserts |matchers| overwriting any matchers with the same RulesetID. + void AddOrUpdateRulesets(CompositeMatcher::MatcherList matchers); - // Adds the |new_matcher| to the list of matchers. If a matcher with the - // corresponding ID is already present, updates the matcher. - void AddOrUpdateRuleset(std::unique_ptr<RulesetMatcher> new_matcher); + // Erases RulesetMatchers with the given RulesetIDs. + void RemoveRulesetsWithIDs(const std::set<RulesetID>& ids); // Computes and returns the set of static RulesetIDs corresponding to // |matchers_|. diff --git a/chromium/extensions/browser/api/declarative_net_request/composite_matcher_unittest.cc b/chromium/extensions/browser/api/declarative_net_request/composite_matcher_unittest.cc index 3b183413e5e..25ca0642d16 100644 --- a/chromium/extensions/browser/api/declarative_net_request/composite_matcher_unittest.cc +++ b/chromium/extensions/browser/api/declarative_net_request/composite_matcher_unittest.cc @@ -9,7 +9,6 @@ #include <vector> #include "base/strings/stringprintf.h" -#include "components/version_info/channel.h" #include "extensions/browser/api/declarative_net_request/constants.h" #include "extensions/browser/api/declarative_net_request/request_action.h" #include "extensions/browser/api/declarative_net_request/request_params.h" @@ -19,7 +18,6 @@ #include "extensions/common/api/declarative_net_request.h" #include "extensions/common/api/declarative_net_request/constants.h" #include "extensions/common/api/declarative_net_request/test_utils.h" -#include "extensions/common/features/feature_channel.h" #include "extensions/common/permissions/permissions_data.h" #include "net/http/http_request_headers.h" #include "testing/gmock/include/gmock/gmock.h" @@ -28,6 +26,7 @@ namespace extensions { namespace declarative_net_request { +namespace { using PageAccess = PermissionsData::PageAccess; using ActionInfo = CompositeMatcher::ActionInfo; @@ -36,6 +35,32 @@ namespace dnr_api = api::declarative_net_request; using CompositeMatcherTest = ::testing::Test; +TestRule CreateModifyHeadersRule( + int id, + int priority, + base::Optional<std::string> url_filter, + base::Optional<std::string> regex_filter, + base::Optional<std::vector<TestHeaderInfo>> request_headers_list, + base::Optional<std::vector<TestHeaderInfo>> response_headers_list) { + TestRule rule = CreateGenericRule(); + rule.id = id; + rule.priority = priority; + + if (url_filter) + rule.condition->url_filter = url_filter; + else if (regex_filter) { + rule.condition->url_filter.reset(); + rule.condition->regex_filter = regex_filter; + } + + rule.action->type = std::string("modifyHeaders"); + if (request_headers_list) + rule.action->request_headers = std::move(request_headers_list); + if (response_headers_list) + rule.action->response_headers = std::move(response_headers_list); + return rule; +} + // Ensure that the rules in a CompositeMatcher are in the same priority space. TEST_F(CompositeMatcherTest, SamePrioritySpace) { // Create the first ruleset matcher. It allows requests to google.com. @@ -98,31 +123,19 @@ TEST_F(CompositeMatcherTest, SamePrioritySpace) { // Tests the GetModifyHeadersActions method. TEST_F(CompositeMatcherTest, GetModifyHeadersActions) { - // TODO(crbug.com/947591): Remove the channel override once implementation of - // modifyHeaders action is complete. - ScopedCurrentChannel channel(::version_info::Channel::UNKNOWN); - - auto create_modify_headers_rule = - [](int id, int priority, const std::string& url_filter, - std::vector<TestHeaderInfo> request_headers_list) { - TestRule rule = CreateGenericRule(); - rule.id = id; - rule.priority = priority; - rule.condition->url_filter = url_filter; - rule.action->type = std::string("modifyHeaders"); - rule.action->request_headers = std::move(request_headers_list); - return rule; - }; - - TestRule rule_1 = create_modify_headers_rule( - kMinValidID, kMinValidPriority, "google.com", - std::vector<TestHeaderInfo>({TestHeaderInfo("header1", "remove"), - TestHeaderInfo("header2", "remove")})); - - TestRule rule_2 = create_modify_headers_rule( - kMinValidID, kMinValidPriority + 1, "/path", - std::vector<TestHeaderInfo>({TestHeaderInfo("header1", "remove"), - TestHeaderInfo("header3", "remove")})); + TestRule rule_1 = CreateModifyHeadersRule( + kMinValidID, kMinValidPriority, "google.com", base::nullopt, + std::vector<TestHeaderInfo>( + {TestHeaderInfo("header1", "remove", base::nullopt), + TestHeaderInfo("header2", "set", "value2")}), + base::nullopt); + + TestRule rule_2 = CreateModifyHeadersRule( + kMinValidID, kMinValidPriority + 1, "/path", base::nullopt, base::nullopt, + std::vector<TestHeaderInfo>( + {TestHeaderInfo("header1", "remove", base::nullopt), + TestHeaderInfo("header2", "append", "VALUE2"), + TestHeaderInfo("header3", "set", "VALUE3")})); // Create the first ruleset matcher, which matches all requests from // |google.com|. @@ -164,15 +177,21 @@ TEST_F(CompositeMatcherTest, GetModifyHeadersActions) { CreateRequestActionForTesting(RequestAction::Type::MODIFY_HEADERS, *rule_1.id, *rule_1.priority, kSource1ID); action_1.request_headers_to_modify = { - RequestAction::HeaderInfo("header1", dnr_api::HEADER_OPERATION_REMOVE), - RequestAction::HeaderInfo("header2", dnr_api::HEADER_OPERATION_REMOVE)}; + RequestAction::HeaderInfo("header1", dnr_api::HEADER_OPERATION_REMOVE, + base::nullopt), + RequestAction::HeaderInfo("header2", dnr_api::HEADER_OPERATION_SET, + "value2")}; RequestAction action_2 = CreateRequestActionForTesting(RequestAction::Type::MODIFY_HEADERS, *rule_2.id, *rule_2.priority, kSource2ID); - action_2.request_headers_to_modify = { - RequestAction::HeaderInfo("header1", dnr_api::HEADER_OPERATION_REMOVE), - RequestAction::HeaderInfo("header3", dnr_api::HEADER_OPERATION_REMOVE)}; + action_2.response_headers_to_modify = { + RequestAction::HeaderInfo("header1", dnr_api::HEADER_OPERATION_REMOVE, + base::nullopt), + RequestAction::HeaderInfo("header2", dnr_api::HEADER_OPERATION_APPEND, + "VALUE2"), + RequestAction::HeaderInfo("header3", dnr_api::HEADER_OPERATION_SET, + "VALUE3")}; // |action_2| should be before |action_1| because |rule_2| // has a higher priority. @@ -200,21 +219,27 @@ TEST_F(CompositeMatcherTest, GetModifyHeadersActions) { PageAccess::kAllowed); // Re-create |action_1| and |action_2| with the updated rule - // priorities. The headers removed by each action should not change. + // priorities. The headers modified by each action should not change. actions = composite_matcher->GetModifyHeadersActions(google_params); action_1 = CreateRequestActionForTesting(RequestAction::Type::MODIFY_HEADERS, *rule_1.id, *rule_1.priority, kSource1ID); action_1.request_headers_to_modify = { - RequestAction::HeaderInfo("header1", dnr_api::HEADER_OPERATION_REMOVE), - RequestAction::HeaderInfo("header2", dnr_api::HEADER_OPERATION_REMOVE)}; + RequestAction::HeaderInfo("header1", dnr_api::HEADER_OPERATION_REMOVE, + base::nullopt), + RequestAction::HeaderInfo("header2", dnr_api::HEADER_OPERATION_SET, + "value2")}; action_2 = CreateRequestActionForTesting(RequestAction::Type::MODIFY_HEADERS, *rule_2.id, *rule_2.priority, kSource2ID); - action_2.request_headers_to_modify = { - RequestAction::HeaderInfo("header1", dnr_api::HEADER_OPERATION_REMOVE), - RequestAction::HeaderInfo("header3", dnr_api::HEADER_OPERATION_REMOVE)}; + action_2.response_headers_to_modify = { + RequestAction::HeaderInfo("header1", dnr_api::HEADER_OPERATION_REMOVE, + base::nullopt), + RequestAction::HeaderInfo("header2", dnr_api::HEADER_OPERATION_APPEND, + "VALUE2"), + RequestAction::HeaderInfo("header3", dnr_api::HEADER_OPERATION_SET, + "VALUE3")}; // |action_1| should now be before |action_2| after their // priorities have been reversed. @@ -223,6 +248,150 @@ TEST_F(CompositeMatcherTest, GetModifyHeadersActions) { ::testing::Eq(::testing::ByRef(action_2)))); } +// Tests that GetModifyHeadersActions method omits rules with an equal or lower +// priority than a matched allow or allowAllRequests rule. +TEST_F(CompositeMatcherTest, GetModifyHeadersActions_Priority) { + using HeaderInfo = RequestAction::HeaderInfo; + int allow_rule_priority = kMinValidPriority + 1; + + TestRule allow_rule = CreateGenericRule(); + allow_rule.id = kMinValidID; + allow_rule.condition->url_filter = std::string("google.com/1"); + allow_rule.action->type = std::string("allow"); + allow_rule.priority = allow_rule_priority; + + TestRule url_rule_1 = CreateModifyHeadersRule( + kMinValidID + 1, allow_rule_priority - 1, "google.com", base::nullopt, + std::vector<TestHeaderInfo>( + {TestHeaderInfo("header1", "remove", base::nullopt)}), + base::nullopt); + + TestRule url_rule_2 = CreateModifyHeadersRule( + kMinValidID + 2, allow_rule_priority, "google.com", base::nullopt, + std::vector<TestHeaderInfo>( + {TestHeaderInfo("header2", "remove", base::nullopt)}), + base::nullopt); + + TestRule url_rule_3 = CreateModifyHeadersRule( + kMinValidID + 3, allow_rule_priority + 1, "google.com", base::nullopt, + std::vector<TestHeaderInfo>( + {TestHeaderInfo("header3", "remove", base::nullopt)}), + base::nullopt); + + TestRule regex_rule_1 = CreateModifyHeadersRule( + kMinValidID + 4, allow_rule_priority - 1, base::nullopt, R"(google\.com)", + std::vector<TestHeaderInfo>( + {TestHeaderInfo("header4", "remove", base::nullopt)}), + base::nullopt); + + TestRule regex_rule_2 = CreateModifyHeadersRule( + kMinValidID + 5, allow_rule_priority, base::nullopt, R"(google\.com)", + std::vector<TestHeaderInfo>( + {TestHeaderInfo("header5", "remove", base::nullopt)}), + base::nullopt); + + TestRule regex_rule_3 = CreateModifyHeadersRule( + kMinValidID + 6, allow_rule_priority + 1, base::nullopt, R"(google\.com)", + std::vector<TestHeaderInfo>( + {TestHeaderInfo("header6", "remove", base::nullopt)}), + base::nullopt); + + const RulesetID kSource1ID(1); + std::unique_ptr<RulesetMatcher> matcher_1; + ASSERT_TRUE( + CreateVerifiedMatcher({allow_rule, url_rule_1, url_rule_2, url_rule_3}, + CreateTemporarySource(kSource1ID), &matcher_1)); + + const RulesetID kSource2ID(2); + std::unique_ptr<RulesetMatcher> matcher_2; + ASSERT_TRUE(CreateVerifiedMatcher({regex_rule_1, regex_rule_2, regex_rule_3}, + CreateTemporarySource(kSource2ID), + &matcher_2)); + + // Create a CompositeMatcher with the rulesets. + std::vector<std::unique_ptr<RulesetMatcher>> matchers; + matchers.push_back(std::move(matcher_1)); + matchers.push_back(std::move(matcher_2)); + auto composite_matcher = + std::make_unique<CompositeMatcher>(std::move(matchers)); + + // Make a request to "http://google.com/1" which matches with all + // modifyHeaders rules and |allow_rule|. + GURL google_url = GURL("http://google.com/1"); + RequestParams google_params; + google_params.url = &google_url; + google_params.element_type = url_pattern_index::flat::ElementType_SUBDOCUMENT; + google_params.is_third_party = false; + + // Call GetBeforeRequestAction first to ensure that test and production code + // paths are consistent. + composite_matcher->GetBeforeRequestAction(google_params, + PageAccess::kAllowed); + + std::vector<RequestAction> actions = + composite_matcher->GetModifyHeadersActions(google_params); + + auto create_action_for_rule = + [](const TestRule& rule, const RulesetID& ruleset_id, + const std::vector<HeaderInfo>& request_headers) { + RequestAction action = + CreateRequestActionForTesting(RequestAction::Type::MODIFY_HEADERS, + *rule.id, *rule.priority, ruleset_id); + + action.request_headers_to_modify = request_headers; + return action; + }; + + RequestAction header_3_action = create_action_for_rule( + url_rule_3, kSource1ID, + {HeaderInfo("header3", dnr_api::HEADER_OPERATION_REMOVE, base::nullopt)}); + RequestAction header_6_action = create_action_for_rule( + regex_rule_3, kSource2ID, + {HeaderInfo("header6", dnr_api::HEADER_OPERATION_REMOVE, base::nullopt)}); + + // For the request to "http://google.com/1", since |url_rule_3| and + // |regex_rule_3| are the only rules with a greater priority than + // |allow_rule|, "header3" and "header4" should be removed. + EXPECT_THAT(actions, ::testing::UnorderedElementsAre( + ::testing::Eq(::testing::ByRef(header_3_action)), + ::testing::Eq(::testing::ByRef(header_6_action)))); + + // Make a request to "http://google.com/2" which should match with all + // modifyHeaders rules but not |allow_rule|. + google_url = GURL("http://google.com/2"); + google_params.url = &google_url; + + // Call GetBeforeRequestAction first to ensure that test and production code + // paths are consistent. + composite_matcher->GetBeforeRequestAction(google_params, + PageAccess::kAllowed); + actions = composite_matcher->GetModifyHeadersActions(google_params); + + RequestAction header_1_action = create_action_for_rule( + url_rule_1, kSource1ID, + {HeaderInfo("header1", dnr_api::HEADER_OPERATION_REMOVE, base::nullopt)}); + RequestAction header_2_action = create_action_for_rule( + url_rule_2, kSource1ID, + {HeaderInfo("header2", dnr_api::HEADER_OPERATION_REMOVE, base::nullopt)}); + RequestAction header_4_action = create_action_for_rule( + regex_rule_1, kSource2ID, + {HeaderInfo("header4", dnr_api::HEADER_OPERATION_REMOVE, base::nullopt)}); + RequestAction header_5_action = create_action_for_rule( + regex_rule_2, kSource2ID, + {HeaderInfo("header5", dnr_api::HEADER_OPERATION_REMOVE, base::nullopt)}); + + // For the request to "http://google.com/2", "header1" to "header6" should be + // removed since all modifyHeaders rules are matched and there is no matching + // allow/allowAllRequests rule. + EXPECT_THAT(actions, ::testing::UnorderedElementsAre( + ::testing::Eq(::testing::ByRef(header_1_action)), + ::testing::Eq(::testing::ByRef(header_2_action)), + ::testing::Eq(::testing::ByRef(header_3_action)), + ::testing::Eq(::testing::ByRef(header_4_action)), + ::testing::Eq(::testing::ByRef(header_5_action)), + ::testing::Eq(::testing::ByRef(header_6_action)))); +} + // Ensure CompositeMatcher detects requests to be notified based on the rule // matched and whether the extenion has access to the request. TEST_F(CompositeMatcherTest, NotifyWithholdFromPageAccess) { @@ -393,5 +562,6 @@ TEST_F(CompositeMatcherTest, GetRedirectUrlFromPriority) { } } +} // namespace } // namespace declarative_net_request } // namespace extensions diff --git a/chromium/extensions/browser/api/declarative_net_request/constants.cc b/chromium/extensions/browser/api/declarative_net_request/constants.cc index deb70eb42f2..692e1c23c2e 100644 --- a/chromium/extensions/browser/api/declarative_net_request/constants.cc +++ b/chromium/extensions/browser/api/declarative_net_request/constants.cc @@ -65,6 +65,15 @@ const char kErrorNoHeaderListsSpecified[] = "one of these keys must be specified with a non-empty list."; const char kErrorInvalidHeaderName[] = "Rule with id * must specify a valid header name to be modified."; +const char kErrorInvalidHeaderValue[] = + "Rule with id * specifies an invalid header value."; +const char kErrorNoHeaderValueSpecified[] = + "Rule with id * must provide a value for a header to be appended/set."; +const char kErrorHeaderValuePresent[] = + "Rule with id * must not provide a header value for a header to be " + "removed."; +const char kErrorCannotAppendRequestHeader[] = + "Rule with id * must not specify a request header to be appended."; const char kErrorListNotPassed[] = "Rules file must contain a list."; const char kRuleCountExceeded[] = @@ -109,6 +118,8 @@ const char kReadDynamicRulesJSONStatusHistogram[] = "Extensions.DeclarativeNetRequest.ReadDynamicRulesJSONStatus"; const char kIsLargeRegexHistogram[] = "Extensions.DeclarativeNetRequest.IsLargeRegexRule"; +const char kLoadRulesetResultHistogram[] = + "Extensions.DeclarativeNetRequest.LoadRulesetResult"; const char kActionCountPlaceholderBadgeText[] = "<<declarativeNetRequestActionCount>>"; diff --git a/chromium/extensions/browser/api/declarative_net_request/constants.h b/chromium/extensions/browser/api/declarative_net_request/constants.h index 7e4cdcfe7c8..8b54d660b1e 100644 --- a/chromium/extensions/browser/api/declarative_net_request/constants.h +++ b/chromium/extensions/browser/api/declarative_net_request/constants.h @@ -56,7 +56,11 @@ enum class ParseResult { ERROR_NO_HEADERS_SPECIFIED, ERROR_EMPTY_REQUEST_HEADERS_LIST, ERROR_EMPTY_RESPONSE_HEADERS_LIST, - ERROR_INVALID_HEADER_NAME + ERROR_INVALID_HEADER_NAME, + ERROR_INVALID_HEADER_VALUE, + ERROR_HEADER_VALUE_NOT_SPECIFIED, + ERROR_HEADER_VALUE_PRESENT, + ERROR_APPEND_REQUEST_HEADER_UNSUPPORTED }; // Describes the ways in which updating dynamic rules can fail. @@ -85,6 +89,39 @@ enum class UpdateDynamicRulesStatus { kMaxValue = kErrorRegexRuleCountExceeded, }; +// Describes the result of loading a single JSON Ruleset. +// This is logged as part of UMA. Hence existing values should not be re- +// numbered or deleted. +enum class LoadRulesetResult { + // Ruleset loading succeeded. + kSuccess = 0, + + // Ruleset loading failed since the provided path did not exist. + kErrorInvalidPath = 1, + + // Ruleset loading failed due to a file read error. + kErrorCannotReadFile = 2, + + // Ruleset loading failed due to a checksum mismatch. + kErrorChecksumMismatch = 3, + + // Ruleset loading failed due to version header mismatch. + // TODO(karandeepb): This should be split into two cases: + // - When the indexed ruleset doesn't have the version header in the + // correct format. + // - When the indexed ruleset's version is not the same as that used by + // Chrome. + kErrorVersionMismatch = 4, + + // Ruleset loading failed since the checksum for the ruleset wasn't found in + // prefs. + kErrorChecksumNotFound = 5, + + // Magic constant used by histograms code. Should be equal to the largest enum + // value. + kMaxValue = kErrorChecksumNotFound, +}; + // Schemes which can be used as part of url transforms. extern const char* const kAllowedTransformSchemes[4]; @@ -110,6 +147,10 @@ extern const char kErrorRegexTooLarge[]; extern const char kErrorRegexesTooLarge[]; extern const char kErrorNoHeaderListsSpecified[]; extern const char kErrorInvalidHeaderName[]; +extern const char kErrorInvalidHeaderValue[]; +extern const char kErrorNoHeaderValueSpecified[]; +extern const char kErrorHeaderValuePresent[]; +extern const char kErrorCannotAppendRequestHeader[]; extern const char kErrorListNotPassed[]; @@ -140,6 +181,7 @@ extern const char kManifestEnabledRulesCountHistogram[]; extern const char kUpdateDynamicRulesStatusHistogram[]; extern const char kReadDynamicRulesJSONStatusHistogram[]; extern const char kIsLargeRegexHistogram[]; +extern const char kLoadRulesetResultHistogram[]; // Placeholder text to use for getBadgeText extension function call, when the // badge text is set to the DNR action count. diff --git a/chromium/extensions/browser/api/declarative_net_request/extension_url_pattern_index_matcher.cc b/chromium/extensions/browser/api/declarative_net_request/extension_url_pattern_index_matcher.cc index 3c2370d818b..71aa3281796 100644 --- a/chromium/extensions/browser/api/declarative_net_request/extension_url_pattern_index_matcher.cc +++ b/chromium/extensions/browser/api/declarative_net_request/extension_url_pattern_index_matcher.cc @@ -84,10 +84,19 @@ ExtensionUrlPatternIndexMatcher::GetAllowAllRequestsAction( std::vector<RequestAction> ExtensionUrlPatternIndexMatcher::GetModifyHeadersActions( - const RequestParams& params) const { + const RequestParams& params, + base::Optional<uint64_t> min_priority) const { + // TODO(crbug.com/1083178): Plumb |min_priority| into UrlPatternIndexMatcher + // to prune more rules before matching on url filters. std::vector<const flat_rule::UrlRule*> rules = GetAllMatchingRules(params, flat::IndexType_modify_headers); + if (min_priority) { + base::EraseIf(rules, [&min_priority](const flat_rule::UrlRule* rule) { + return rule->priority() <= *min_priority; + }); + } + return GetModifyHeadersActionsFromMetadata(params, rules, *metadata_list_); } diff --git a/chromium/extensions/browser/api/declarative_net_request/extension_url_pattern_index_matcher.h b/chromium/extensions/browser/api/declarative_net_request/extension_url_pattern_index_matcher.h index ccf520ac914..a886abb681f 100644 --- a/chromium/extensions/browser/api/declarative_net_request/extension_url_pattern_index_matcher.h +++ b/chromium/extensions/browser/api/declarative_net_request/extension_url_pattern_index_matcher.h @@ -29,7 +29,8 @@ class ExtensionUrlPatternIndexMatcher final : public RulesetMatcherBase { // RulesetMatcherBase override: ~ExtensionUrlPatternIndexMatcher() override; std::vector<RequestAction> GetModifyHeadersActions( - const RequestParams& params) const override; + const RequestParams& params, + base::Optional<uint64_t> min_priority) const override; bool IsExtraHeadersMatcher() const override { return is_extra_headers_matcher_; } diff --git a/chromium/extensions/browser/api/declarative_net_request/file_sequence_helper.cc b/chromium/extensions/browser/api/declarative_net_request/file_sequence_helper.cc index f8018d903e7..9afe17d2dc4 100644 --- a/chromium/extensions/browser/api/declarative_net_request/file_sequence_helper.cc +++ b/chromium/extensions/browser/api/declarative_net_request/file_sequence_helper.cc @@ -18,7 +18,6 @@ #include "base/notreached.h" #include "base/stl_util.h" #include "base/strings/string_number_conversions.h" -#include "base/task/post_task.h" #include "content/public/browser/browser_task_traits.h" #include "content/public/browser/browser_thread.h" #include "extensions/browser/api/declarative_net_request/constants.h" @@ -101,9 +100,8 @@ class ReindexHelper : public base::RefCountedThreadSafe<ReindexHelper> { // In case of updates to the ruleset version, the change of ruleset checksum // is expected. - if (result.success && - ruleset->load_ruleset_result() == - RulesetMatcher::LoadRulesetResult::kLoadErrorVersionMismatch) { + if (result.success && ruleset->load_ruleset_result() == + LoadRulesetResult::kErrorVersionMismatch) { ruleset->set_new_checksum(result.ruleset_checksum); // Also change the |expected_checksum| so that any subsequent load @@ -131,21 +129,21 @@ class ReindexHelper : public base::RefCountedThreadSafe<ReindexHelper> { DISALLOW_COPY_AND_ASSIGN(ReindexHelper); }; -UpdateDynamicRulesStatus GetStatusForLoadRulesetError( - RulesetMatcher::LoadRulesetResult result) { - using Result = RulesetMatcher::LoadRulesetResult; +UpdateDynamicRulesStatus GetUpdateDynamicRuleStatus(LoadRulesetResult result) { switch (result) { - case Result::kLoadSuccess: + case LoadRulesetResult::kSuccess: break; - case Result::kLoadErrorInvalidPath: + case LoadRulesetResult::kErrorInvalidPath: return UpdateDynamicRulesStatus::kErrorCreateMatcher_InvalidPath; - case Result::kLoadErrorFileRead: + case LoadRulesetResult::kErrorCannotReadFile: return UpdateDynamicRulesStatus::kErrorCreateMatcher_FileReadError; - case Result::kLoadErrorChecksumMismatch: + case LoadRulesetResult::kErrorChecksumMismatch: return UpdateDynamicRulesStatus::kErrorCreateMatcher_ChecksumMismatch; - case Result::kLoadErrorVersionMismatch: + case LoadRulesetResult::kErrorVersionMismatch: return UpdateDynamicRulesStatus::kErrorCreateMatcher_VersionMismatch; - case Result::kLoadResultMax: + case LoadRulesetResult::kErrorChecksumNotFound: + // Updating dynamic rules shouldn't require looking up checksum from + // prefs. break; } @@ -207,7 +205,7 @@ bool GetNewDynamicRules(const RulesetSource& source, int regex_rule_count = std::count_if( new_rules->begin(), new_rules->end(), [](const dnr_api::Rule& rule) { return !!rule.condition.regex_filter; }); - if (regex_rule_count > dnr_api::MAX_NUMBER_OF_REGEX_RULES) { + if (regex_rule_count > GetRegexRuleLimit()) { *status = UpdateDynamicRulesStatus::kErrorRegexRuleCountExceeded; *error = kDynamicRegexRuleCountExceeded; return false; @@ -336,23 +334,23 @@ std::unique_ptr<RulesetMatcher> RulesetInfo::TakeMatcher() { return std::move(matcher_); } -RulesetMatcher::LoadRulesetResult RulesetInfo::load_ruleset_result() const { - DCHECK(load_ruleset_result_); +const base::Optional<LoadRulesetResult>& RulesetInfo::load_ruleset_result() + const { // |matcher_| is valid only on success. - DCHECK_EQ(load_ruleset_result_ == RulesetMatcher::kLoadSuccess, !!matcher_); - return *load_ruleset_result_; + DCHECK_EQ(load_ruleset_result_ == LoadRulesetResult::kSuccess, !!matcher_); + return load_ruleset_result_; } void RulesetInfo::CreateVerifiedMatcher() { DCHECK(expected_checksum_); DCHECK(GetExtensionFileTaskRunner()->RunsTasksInCurrentSequence()); + // Ensure we aren't calling this redundantly. If did_load_successfully() + // returns true, we should already have a valid RulesetMatcher. + DCHECK(!did_load_successfully()); + load_ruleset_result_ = RulesetMatcher::CreateVerifiedMatcher( source_, *expected_checksum_, &matcher_); - - UMA_HISTOGRAM_ENUMERATION( - "Extensions.DeclarativeNetRequest.LoadRulesetResult", - load_ruleset_result(), RulesetMatcher::kLoadResultMax); } LoadRequestData::LoadRequestData(ExtensionId extension_id) @@ -371,7 +369,6 @@ void FileSequenceHelper::LoadRulesets( LoadRequestData load_data, LoadRulesetsUICallback ui_callback) const { DCHECK(GetExtensionFileTaskRunner()->RunsTasksInCurrentSequence()); - DCHECK(!load_data.rulesets.empty()); bool success = true; for (auto& ruleset : load_data.rulesets) { @@ -381,10 +378,9 @@ void FileSequenceHelper::LoadRulesets( if (success) { // Set priority explicitly to avoid unwanted task priority inheritance. - base::PostTask( - FROM_HERE, - {content::BrowserThread::UI, base::TaskPriority::USER_BLOCKING}, - base::BindOnce(std::move(ui_callback), std::move(load_data))); + content::GetUIThreadTaskRunner({base::TaskPriority::USER_BLOCKING}) + ->PostTask(FROM_HERE, base::BindOnce(std::move(ui_callback), + std::move(load_data))); return; } @@ -418,11 +414,10 @@ void FileSequenceHelper::UpdateDynamicRules( base::UmaHistogramEnumeration(kUpdateDynamicRulesStatusHistogram, status); // Set priority explicitly to avoid unwanted task priority inheritance. - base::PostTask( - FROM_HERE, - {content::BrowserThread::UI, base::TaskPriority::USER_BLOCKING}, - base::BindOnce(std::move(ui_callback), std::move(load_data), - std::move(error))); + content::GetUIThreadTaskRunner({base::TaskPriority::USER_BLOCKING}) + ->PostTask(FROM_HERE, + base::BindOnce(std::move(ui_callback), std::move(load_data), + std::move(error))); }; int new_ruleset_checksum = -1; @@ -440,10 +435,10 @@ void FileSequenceHelper::UpdateDynamicRules( dynamic_ruleset.set_expected_checksum(new_ruleset_checksum); dynamic_ruleset.set_new_checksum(new_ruleset_checksum); dynamic_ruleset.CreateVerifiedMatcher(); + DCHECK(dynamic_ruleset.load_ruleset_result()); if (!dynamic_ruleset.did_load_successfully()) { - status = - GetStatusForLoadRulesetError(dynamic_ruleset.load_ruleset_result()); + status = GetUpdateDynamicRuleStatus(*dynamic_ruleset.load_ruleset_result()); log_status_and_dispatch_callback(kInternalErrorUpdatingDynamicRules, status); return; @@ -467,10 +462,9 @@ void FileSequenceHelper::OnRulesetsReindexed(LoadRulesetsUICallback ui_callback, } // The UI thread will handle success or failure. - base::PostTask( - FROM_HERE, - {content::BrowserThread::UI, base::TaskPriority::USER_BLOCKING}, - base::BindOnce(std::move(ui_callback), std::move(load_data))); + content::GetUIThreadTaskRunner({base::TaskPriority::USER_BLOCKING}) + ->PostTask(FROM_HERE, + base::BindOnce(std::move(ui_callback), std::move(load_data))); } } // namespace declarative_net_request diff --git a/chromium/extensions/browser/api/declarative_net_request/file_sequence_helper.h b/chromium/extensions/browser/api/declarative_net_request/file_sequence_helper.h index 4a9e77cb26f..c5e640d97c0 100644 --- a/chromium/extensions/browser/api/declarative_net_request/file_sequence_helper.h +++ b/chromium/extensions/browser/api/declarative_net_request/file_sequence_helper.h @@ -13,6 +13,7 @@ #include "base/macros.h" #include "base/memory/weak_ptr.h" #include "base/optional.h" +#include "extensions/browser/api/declarative_net_request/constants.h" #include "extensions/browser/api/declarative_net_request/ruleset_matcher.h" #include "extensions/browser/api/declarative_net_request/ruleset_source.h" #include "extensions/common/extension_id.h" @@ -57,13 +58,13 @@ class RulesetInfo { return reindexing_successful_; } - // Must be called after CreateVerifiedMatcher. - RulesetMatcher::LoadRulesetResult load_ruleset_result() const; + // Returns the result of loading the ruleset. The return value is valid (not + // equal to base::nullopt) iff CreateVerifiedMatcher() has been called. + const base::Optional<LoadRulesetResult>& load_ruleset_result() const; // Whether the ruleset loaded successfully. bool did_load_successfully() const { - return load_ruleset_result_ && - *load_ruleset_result_ == RulesetMatcher::kLoadSuccess; + return load_ruleset_result() == LoadRulesetResult::kSuccess; } // Must be invoked on the extension file task runner. Must only be called @@ -78,7 +79,7 @@ class RulesetInfo { // Stores the result of creating a verified matcher from the |source_|. std::unique_ptr<RulesetMatcher> matcher_; - base::Optional<RulesetMatcher::LoadRulesetResult> load_ruleset_result_; + base::Optional<LoadRulesetResult> load_ruleset_result_; // The new checksum to be persisted to prefs. A new checksum should only be // set in case of flatbuffer version mismatch. @@ -114,7 +115,7 @@ class FileSequenceHelper { // Loads rulesets for |load_data|. Invokes |ui_callback| on the UI thread once // loading is done. Also tries to reindex the rulesets on failure. - // |load_data.rulesets| must not be empty. + // This is a no-op if |load_data.rulesets| is empty. using LoadRulesetsUICallback = base::OnceCallback<void(LoadRequestData)>; void LoadRulesets(LoadRequestData load_data, LoadRulesetsUICallback ui_callback) const; diff --git a/chromium/extensions/browser/api/declarative_net_request/file_sequence_helper_unittest.cc b/chromium/extensions/browser/api/declarative_net_request/file_sequence_helper_unittest.cc index 4f112cf8361..d8b18623c71 100644 --- a/chromium/extensions/browser/api/declarative_net_request/file_sequence_helper_unittest.cc +++ b/chromium/extensions/browser/api/declarative_net_request/file_sequence_helper_unittest.cc @@ -44,20 +44,23 @@ api::declarative_net_request::Rule GetAPIRule(const TestRule& rule) { return result; } -struct LoadRulesetResult { +struct TestLoadRulesetInfo { bool has_new_checksum = false; base::Optional<bool> reindexing_successful; - RulesetMatcher::LoadRulesetResult load_result = - RulesetMatcher::kLoadResultMax; + base::Optional<LoadRulesetResult> load_result; }; struct TestCase { explicit TestCase(RulesetSource source) : source(std::move(source)) {} int checksum; RulesetSource source; - LoadRulesetResult expected_result; + TestLoadRulesetInfo expected_result; }; +ExtensionId GenerateDummyExtensionID() { + return crx_file::id_util::GenerateId("dummy_extension"); +} + class FileSequenceHelperTest : public ExtensionsTest { public: FileSequenceHelperTest() = default; @@ -115,9 +118,7 @@ class FileSequenceHelperTest : public ExtensionsTest { } void TestLoadRulesets(const std::vector<TestCase>& test_cases) { - ExtensionId extension_id = crx_file::id_util::GenerateId("dummy_extension"); - - LoadRequestData data(extension_id); + LoadRequestData data(GenerateDummyExtensionID()); for (const auto& test_case : test_cases) { data.rulesets.emplace_back(test_case.source.Clone()); data.rulesets.back().set_expected_checksum(test_case.checksum); @@ -133,16 +134,17 @@ class FileSequenceHelperTest : public ExtensionsTest { for (size_t i = 0; i < data.rulesets.size(); i++) { SCOPED_TRACE(base::StringPrintf("Testing ruleset %" PRIuS, i)); const RulesetInfo& ruleset = data.rulesets[i]; - const LoadRulesetResult& expected_result = + const TestLoadRulesetInfo& expected_result = test_cases[i].expected_result; EXPECT_EQ(expected_result.has_new_checksum, ruleset.new_checksum().has_value()); EXPECT_EQ(expected_result.reindexing_successful, ruleset.reindexing_successful()); + ASSERT_TRUE(ruleset.load_ruleset_result()); EXPECT_EQ(expected_result.load_result, ruleset.load_ruleset_result()) - << ruleset.load_ruleset_result(); + << *ruleset.load_ruleset_result(); } run_loop->Quit(); @@ -158,6 +160,22 @@ class FileSequenceHelperTest : public ExtensionsTest { run_loop.Run(); } + void TestNoRulesetsToLoad() { + LoadRequestData data(GenerateDummyExtensionID()); + + base::RunLoop run_loop; + auto load_ruleset_callback = base::BindOnce( + [](base::RunLoop* run_loop, LoadRequestData data) { run_loop->Quit(); }, + &run_loop); + + auto load_ruleset_task = base::BindOnce( + &FileSequenceHelper::LoadRulesets, base::Unretained(helper_.get()), + std::move(data), std::move(load_ruleset_callback)); + GetExtensionFileTaskRunner()->PostTask(FROM_HERE, + std::move(load_ruleset_task)); + run_loop.Run(); + } + // Initialize |num_rulesets| rulesets and returns the corresponding test // cases. std::vector<TestCase> InitializeRulesets(size_t num_rulesets) const { @@ -174,7 +192,7 @@ class FileSequenceHelperTest : public ExtensionsTest { &matcher, &test_case.checksum)); // Initially loading all the rulesets should succeed. - test_case.expected_result.load_result = RulesetMatcher::kLoadSuccess; + test_case.expected_result.load_result = LoadRulesetResult::kSuccess; } return test_cases; } @@ -188,6 +206,10 @@ class FileSequenceHelperTest : public ExtensionsTest { DISALLOW_COPY_AND_ASSIGN(FileSequenceHelperTest); }; +TEST_F(FileSequenceHelperTest, NoRulesetsToLoad) { + TestNoRulesetsToLoad(); +} + TEST_F(FileSequenceHelperTest, IndexedRulesetDeleted) { const size_t kNumRulesets = 3; std::vector<TestCase> test_cases = InitializeRulesets(kNumRulesets); @@ -219,9 +241,9 @@ TEST_F(FileSequenceHelperTest, ChecksumMismatch) { test_cases[1].checksum--; test_cases[2].checksum--; test_cases[1].expected_result.load_result = - RulesetMatcher::kLoadErrorChecksumMismatch; + LoadRulesetResult::kErrorChecksumMismatch; test_cases[2].expected_result.load_result = - RulesetMatcher::kLoadErrorChecksumMismatch; + LoadRulesetResult::kErrorChecksumMismatch; test_cases[1].expected_result.reindexing_successful = false; test_cases[2].expected_result.reindexing_successful = false; @@ -242,7 +264,7 @@ TEST_F(FileSequenceHelperTest, RulesetFormatVersionMismatch) { for (auto& test_case : test_cases) { test_case.expected_result.reindexing_successful = true; test_case.expected_result.has_new_checksum = true; - test_case.expected_result.load_result = RulesetMatcher::kLoadSuccess; + test_case.expected_result.load_result = LoadRulesetResult::kSuccess; } TestLoadRulesets(test_cases); @@ -264,10 +286,10 @@ TEST_F(FileSequenceHelperTest, JSONAndIndexedRulesetDeleted) { test_cases[1].expected_result.reindexing_successful = false; test_cases[0].expected_result.load_result = - RulesetMatcher::kLoadErrorInvalidPath; + LoadRulesetResult::kErrorInvalidPath; test_cases[1].expected_result.load_result = - RulesetMatcher::kLoadErrorInvalidPath; - test_cases[2].expected_result.load_result = RulesetMatcher::kLoadSuccess; + LoadRulesetResult::kErrorInvalidPath; + test_cases[2].expected_result.load_result = LoadRulesetResult::kSuccess; TestLoadRulesets(test_cases); } diff --git a/chromium/extensions/browser/api/declarative_net_request/flat/extension_ruleset.fbs b/chromium/extensions/browser/api/declarative_net_request/flat/extension_ruleset.fbs index 046cef8022d..f8dc1aac9f0 100644 --- a/chromium/extensions/browser/api/declarative_net_request/flat/extension_ruleset.fbs +++ b/chromium/extensions/browser/api/declarative_net_request/flat/extension_ruleset.fbs @@ -101,6 +101,8 @@ enum IndexType : ubyte { /// The type of header operation for modifyHeaders rules. Corresponds to /// extensions::api::declarative_net_request::HeaderOperation. enum HeaderOperation : ubyte { + append, + set, remove } @@ -112,6 +114,9 @@ table ModifyHeaderInfo { /// case-insensitive, the header name is normalized by converting it to /// lowercase. header: string; + /// The value of the header to be set/appended. Should be specified only if + /// |operation| is append or set. + value: string; } /// Completely represents a rule with a regex filter. diff --git a/chromium/extensions/browser/api/declarative_net_request/flat_ruleset_indexer.cc b/chromium/extensions/browser/api/declarative_net_request/flat_ruleset_indexer.cc index ff9430d05f1..511e8c3f231 100644 --- a/chromium/extensions/browser/api/declarative_net_request/flat_ruleset_indexer.cc +++ b/chromium/extensions/browser/api/declarative_net_request/flat_ruleset_indexer.cc @@ -162,10 +162,17 @@ FlatVectorOffset<flat::ModifyHeaderInfo> BuildModifyHeaderInfoOffset( for (const dnr_api::ModifyHeaderInfo& header_info : modify_header_list) { flat::HeaderOperation operation = flat::HeaderOperation_remove; + FlatStringOffset header_value; switch (header_info.operation) { case dnr_api::HeaderOperation::HEADER_OPERATION_NONE: - NOTREACHED(); + case dnr_api::HEADER_OPERATION_APPEND: + operation = flat::HeaderOperation_append; + header_value = builder->CreateSharedString(*header_info.value); + break; + case dnr_api::HEADER_OPERATION_SET: + operation = flat::HeaderOperation_set; + header_value = builder->CreateSharedString(*header_info.value); break; case dnr_api::HEADER_OPERATION_REMOVE: operation = flat::HeaderOperation_remove; @@ -174,8 +181,8 @@ FlatVectorOffset<flat::ModifyHeaderInfo> BuildModifyHeaderInfoOffset( FlatStringOffset header_name = builder->CreateSharedString(base::ToLowerASCII(header_info.header)); - flat_modify_header_list.push_back( - flat::CreateModifyHeaderInfo(*builder, operation, header_name)); + flat_modify_header_list.push_back(flat::CreateModifyHeaderInfo( + *builder, operation, header_name, header_value)); } return builder->CreateVector(flat_modify_header_list); diff --git a/chromium/extensions/browser/api/declarative_net_request/flat_ruleset_indexer_unittest.cc b/chromium/extensions/browser/api/declarative_net_request/flat_ruleset_indexer_unittest.cc index 24c08e49319..2769692c88f 100644 --- a/chromium/extensions/browser/api/declarative_net_request/flat_ruleset_indexer_unittest.cc +++ b/chromium/extensions/browser/api/declarative_net_request/flat_ruleset_indexer_unittest.cc @@ -60,7 +60,18 @@ std::vector<dnr_api::ModifyHeaderInfo> ToVector( dnr_api::ModifyHeaderInfo header_info; const flat::HeaderOperation flat_operation = flat_header_info->operation(); + const flatbuffers::String* flat_value = flat_header_info->value(); switch (flat_operation) { + case flat::HeaderOperation_append: + header_info.operation = dnr_api::HEADER_OPERATION_APPEND; + DCHECK(flat_value); + header_info.value = std::make_unique<std::string>(ToString(flat_value)); + break; + case flat::HeaderOperation_set: + header_info.operation = dnr_api::HEADER_OPERATION_SET; + DCHECK(flat_value); + header_info.value = std::make_unique<std::string>(ToString(flat_value)); + break; case flat::HeaderOperation_remove: header_info.operation = dnr_api::HEADER_OPERATION_REMOVE; break; @@ -466,12 +477,18 @@ TEST_F(FlatRulesetIndexerTest, MultipleRules) { // Modify headers rules. std::vector<dnr_api::ModifyHeaderInfo> request_headers_1; - request_headers_1.push_back( - CreateModifyHeaderInfo(dnr_api::HEADER_OPERATION_REMOVE, "cookie")); + request_headers_1.push_back(CreateModifyHeaderInfo( + dnr_api::HEADER_OPERATION_SET, "cookie", "sample-cookie")); std::vector<dnr_api::ModifyHeaderInfo> response_headers_1; - response_headers_1.push_back( - CreateModifyHeaderInfo(dnr_api::HEADER_OPERATION_REMOVE, "set-cookie")); + response_headers_1.push_back(CreateModifyHeaderInfo( + dnr_api::HEADER_OPERATION_REMOVE, "set-cookie", base::nullopt)); + + response_headers_1.push_back(CreateModifyHeaderInfo( + dnr_api::HEADER_OPERATION_APPEND, "custom-1", "value-1")); + + response_headers_1.push_back(CreateModifyHeaderInfo( + dnr_api::HEADER_OPERATION_SET, "custom-2", "value-2")); rules_to_index.push_back(CreateIndexedRule( 23, kMinValidPriority, flat_rule::OptionFlag_IS_CASE_INSENSITIVE, @@ -482,8 +499,8 @@ TEST_F(FlatRulesetIndexerTest, MultipleRules) { std::move(request_headers_1), std::move(response_headers_1))); std::vector<dnr_api::ModifyHeaderInfo> request_headers_2; - request_headers_2.push_back( - CreateModifyHeaderInfo(dnr_api::HEADER_OPERATION_REMOVE, "referer")); + request_headers_2.push_back(CreateModifyHeaderInfo( + dnr_api::HEADER_OPERATION_REMOVE, "referer", base::nullopt)); rules_to_index.push_back(CreateIndexedRule( 24, kMinValidPriority, flat_rule::OptionFlag_IS_CASE_INSENSITIVE, @@ -544,8 +561,10 @@ TEST_F(FlatRulesetIndexerTest, RegexRules) { // Modify headers rule. std::vector<dnr_api::ModifyHeaderInfo> request_headers; - request_headers.push_back( - CreateModifyHeaderInfo(dnr_api::HEADER_OPERATION_REMOVE, "referer")); + request_headers.push_back(CreateModifyHeaderInfo( + dnr_api::HEADER_OPERATION_REMOVE, "referer", base::nullopt)); + request_headers.push_back(CreateModifyHeaderInfo( + dnr_api::HEADER_OPERATION_SET, "cookie", "sample-cookie")); rules_to_index.push_back(CreateIndexedRule( 21, kMinValidPriority, flat_rule::OptionFlag_IS_CASE_INSENSITIVE, flat_rule::ElementType_SUBDOCUMENT, flat_rule::ActivationType_NONE, diff --git a/chromium/extensions/browser/api/declarative_net_request/index_helper.cc b/chromium/extensions/browser/api/declarative_net_request/index_helper.cc index e573e2a0364..cfe9d0f36f6 100644 --- a/chromium/extensions/browser/api/declarative_net_request/index_helper.cc +++ b/chromium/extensions/browser/api/declarative_net_request/index_helper.cc @@ -11,6 +11,7 @@ #include "base/metrics/histogram_macros.h" #include "base/time/time.h" #include "extensions/browser/api/declarative_net_request/constants.h" +#include "extensions/browser/api/declarative_net_request/utils.h" #include "extensions/common/api/declarative_net_request.h" #include "extensions/common/extension.h" #include "extensions/common/manifest.h" @@ -45,7 +46,7 @@ IndexHelper::Result CombineResults( // Per-ruleset limits should have been enforced during ruleset indexing. DCHECK_LE(index_result.regex_rules_count, - static_cast<size_t>(dnr_api::MAX_NUMBER_OF_REGEX_RULES)); + static_cast<size_t>(GetRegexRuleLimit())); DCHECK_LE(index_result.rules_count, source->rule_count_limit()); if (!index_result.success) { @@ -72,12 +73,12 @@ IndexHelper::Result CombineResults( // Raise an install warning if the enabled rule count exceeds the API limits. // We don't raise a hard error to maintain forwards compatibility. - if (enabled_rules_count > static_cast<size_t>(dnr_api::MAX_NUMBER_OF_RULES)) { + if (enabled_rules_count > static_cast<size_t>(GetStaticRuleLimit())) { total_result.warnings.emplace_back( kEnabledRuleCountExceeded, manifest_keys::kDeclarativeNetRequestKey, manifest_keys::kDeclarativeRuleResourcesKey); } else if (enabled_regex_rules_count > - static_cast<size_t>(dnr_api::MAX_NUMBER_OF_REGEX_RULES)) { + static_cast<size_t>(GetRegexRuleLimit())) { total_result.warnings.emplace_back( kEnabledRegexRuleCountExceeded, manifest_keys::kDeclarativeNetRequestKey, diff --git a/chromium/extensions/browser/api/declarative_net_request/indexed_rule.cc b/chromium/extensions/browser/api/declarative_net_request/indexed_rule.cc index 2181175b7e5..330459e4708 100644 --- a/chromium/extensions/browser/api/declarative_net_request/indexed_rule.cc +++ b/chromium/extensions/browser/api/declarative_net_request/indexed_rule.cc @@ -436,6 +436,25 @@ ParseResult ValidateHeaders( for (const auto& header_info : headers) { if (!net::HttpUtil::IsValidHeaderName(header_info.header)) return ParseResult::ERROR_INVALID_HEADER_NAME; + + // Ensure that request headers cannot be appended. + if (are_request_headers && + header_info.operation == dnr_api::HEADER_OPERATION_APPEND) { + return ParseResult::ERROR_APPEND_REQUEST_HEADER_UNSUPPORTED; + } + + if (header_info.value) { + if (!net::HttpUtil::IsValidHeaderValue(*header_info.value)) + return ParseResult::ERROR_INVALID_HEADER_VALUE; + + // Check that a remove operation must not specify a value. + if (header_info.operation == dnr_api::HEADER_OPERATION_REMOVE) + return ParseResult::ERROR_HEADER_VALUE_PRESENT; + } else if (header_info.operation == dnr_api::HEADER_OPERATION_APPEND || + header_info.operation == dnr_api::HEADER_OPERATION_SET) { + // Check that an append or set operation must specify a value. + return ParseResult::ERROR_HEADER_VALUE_NOT_SPECIFIED; + } } return ParseResult::SUCCESS; diff --git a/chromium/extensions/browser/api/declarative_net_request/indexed_rule_unittest.cc b/chromium/extensions/browser/api/declarative_net_request/indexed_rule_unittest.cc index 1821cdd4e5a..04a1ff6a796 100644 --- a/chromium/extensions/browser/api/declarative_net_request/indexed_rule_unittest.cc +++ b/chromium/extensions/browser/api/declarative_net_request/indexed_rule_unittest.cc @@ -766,6 +766,7 @@ TEST_F(IndexedRuleTest, ModifyHeadersParsing) { struct RawHeaderInfo { dnr_api::HeaderOperation operation; std::string header; + base::Optional<std::string> value; }; using RawHeaderInfoList = std::vector<RawHeaderInfo>; @@ -785,7 +786,8 @@ TEST_F(IndexedRuleTest, ModifyHeadersParsing) { // Raise an error if the request or response headers list is specified, // but empty. {RawHeaderInfoList(), - RawHeaderInfoList({{dnr_api::HEADER_OPERATION_REMOVE, "set-cookie"}}), + RawHeaderInfoList( + {{dnr_api::HEADER_OPERATION_REMOVE, "set-cookie", base::nullopt}}), ParseResult::ERROR_EMPTY_REQUEST_HEADERS_LIST}, {base::nullopt, RawHeaderInfoList(), @@ -794,21 +796,51 @@ TEST_F(IndexedRuleTest, ModifyHeadersParsing) { // Raise an error if a header list contains an empty or invalid header // name. {base::nullopt, - RawHeaderInfoList({{dnr_api::HEADER_OPERATION_REMOVE, ""}}), + RawHeaderInfoList( + {{dnr_api::HEADER_OPERATION_REMOVE, "", base::nullopt}}), ParseResult::ERROR_INVALID_HEADER_NAME}, {base::nullopt, - RawHeaderInfoList({{dnr_api::HEADER_OPERATION_REMOVE, "<<invalid>>"}}), + RawHeaderInfoList( + {{dnr_api::HEADER_OPERATION_REMOVE, "<<invalid>>", base::nullopt}}), ParseResult::ERROR_INVALID_HEADER_NAME}, - // Parsing should succeed if only one non-empty header list is specified. - {RawHeaderInfoList({{dnr_api::HEADER_OPERATION_REMOVE, "cookie"}}), + // Raise an error if a header list contains an invalid header value. + {base::nullopt, + RawHeaderInfoList({{dnr_api::HEADER_OPERATION_APPEND, "set-cookie", + "invalid\nvalue"}}), + ParseResult::ERROR_INVALID_HEADER_VALUE}, + + // Raise an error if a header value is specified for a remove rule. + {RawHeaderInfoList( + {{dnr_api::HEADER_OPERATION_REMOVE, "cookie", "remove"}}), + base::nullopt, ParseResult::ERROR_HEADER_VALUE_PRESENT}, + + // Raise an error if no header value is specified for an append or set + // rule. + {RawHeaderInfoList( + {{dnr_api::HEADER_OPERATION_SET, "cookie", base::nullopt}}), + base::nullopt, ParseResult::ERROR_HEADER_VALUE_NOT_SPECIFIED}, + + {base::nullopt, + RawHeaderInfoList( + {{dnr_api::HEADER_OPERATION_APPEND, "set-cookie", base::nullopt}}), + ParseResult::ERROR_HEADER_VALUE_NOT_SPECIFIED}, + + // Raise an error if a rule specifies a request header to be appended. + {RawHeaderInfoList( + {{dnr_api::HEADER_OPERATION_APPEND, "cookie", "cookie-value"}}), + base::nullopt, ParseResult::ERROR_APPEND_REQUEST_HEADER_UNSUPPORTED}, + + {RawHeaderInfoList( + {{dnr_api::HEADER_OPERATION_REMOVE, "cookie", base::nullopt}, + {dnr_api::HEADER_OPERATION_SET, "referer", ""}}), base::nullopt, ParseResult::SUCCESS}, - // Parsing should succeed if both header lists are specified and - // non-empty. - {RawHeaderInfoList({{dnr_api::HEADER_OPERATION_REMOVE, "referer"}}), - RawHeaderInfoList({{dnr_api::HEADER_OPERATION_REMOVE, "set-cookie"}}), + {RawHeaderInfoList( + {{dnr_api::HEADER_OPERATION_REMOVE, "referer", base::nullopt}}), + RawHeaderInfoList( + {{dnr_api::HEADER_OPERATION_APPEND, "set-cookie", "abcd"}}), ParseResult::SUCCESS}, }; @@ -821,11 +853,11 @@ TEST_F(IndexedRuleTest, ModifyHeadersParsing) { if (cases[i].request_headers) { rule.action.request_headers = std::make_unique<ModifyHeaderInfoList>(); for (auto header : *cases[i].request_headers) { - rule.action.request_headers->push_back( - CreateModifyHeaderInfo(header.operation, header.header)); + rule.action.request_headers->push_back(CreateModifyHeaderInfo( + header.operation, header.header, header.value)); - expected_request_headers.push_back( - CreateModifyHeaderInfo(header.operation, header.header)); + expected_request_headers.push_back(CreateModifyHeaderInfo( + header.operation, header.header, header.value)); } } @@ -833,11 +865,11 @@ TEST_F(IndexedRuleTest, ModifyHeadersParsing) { if (cases[i].response_headers) { rule.action.response_headers = std::make_unique<ModifyHeaderInfoList>(); for (auto header : *cases[i].response_headers) { - rule.action.response_headers->push_back( - CreateModifyHeaderInfo(header.operation, header.header)); + rule.action.response_headers->push_back(CreateModifyHeaderInfo( + header.operation, header.header, header.value)); - expected_response_headers.push_back( - CreateModifyHeaderInfo(header.operation, header.header)); + expected_response_headers.push_back(CreateModifyHeaderInfo( + header.operation, header.header, header.value)); } } diff --git a/chromium/extensions/browser/api/declarative_net_request/indexed_ruleset_format_version_unittest.cc b/chromium/extensions/browser/api/declarative_net_request/indexed_ruleset_format_version_unittest.cc index ea29e068875..bf3f591949b 100644 --- a/chromium/extensions/browser/api/declarative_net_request/indexed_ruleset_format_version_unittest.cc +++ b/chromium/extensions/browser/api/declarative_net_request/indexed_ruleset_format_version_unittest.cc @@ -64,11 +64,14 @@ enum IndexType : ubyte { count } enum HeaderOperation : ubyte { + append, + set, remove } table ModifyHeaderInfo { operation: HeaderOperation; header: string; + value: string; } table RegexRule { url_rule: url_pattern_index.flat.UrlRule; @@ -146,7 +149,7 @@ TEST_F(IndexedRulesetFormatVersionTest, CheckVersionUpdated) { EXPECT_EQ(StripCommentsAndWhitespace(kFlatbufferSchemaExpected), StripCommentsAndWhitespace(flatbuffer_schema)) << "Schema change detected; update this test and the schema version."; - EXPECT_EQ(17, GetIndexedRulesetFormatVersionForTesting()) + EXPECT_EQ(18, GetIndexedRulesetFormatVersionForTesting()) << "Update this test if you update the schema version."; } diff --git a/chromium/extensions/browser/api/declarative_net_request/parse_info.cc b/chromium/extensions/browser/api/declarative_net_request/parse_info.cc index 3069695505b..7c0c1474502 100644 --- a/chromium/extensions/browser/api/declarative_net_request/parse_info.cc +++ b/chromium/extensions/browser/api/declarative_net_request/parse_info.cc @@ -137,6 +137,18 @@ std::string GetError(ParseResult error_reason, const int* rule_id) { case ParseResult::ERROR_INVALID_HEADER_NAME: return ErrorUtils::FormatErrorMessage(kErrorInvalidHeaderName, base::NumberToString(*rule_id)); + case ParseResult::ERROR_INVALID_HEADER_VALUE: + return ErrorUtils::FormatErrorMessage(kErrorInvalidHeaderValue, + base::NumberToString(*rule_id)); + case ParseResult::ERROR_HEADER_VALUE_NOT_SPECIFIED: + return ErrorUtils::FormatErrorMessage(kErrorNoHeaderValueSpecified, + base::NumberToString(*rule_id)); + case ParseResult::ERROR_HEADER_VALUE_PRESENT: + return ErrorUtils::FormatErrorMessage(kErrorHeaderValuePresent, + base::NumberToString(*rule_id)); + case ParseResult::ERROR_APPEND_REQUEST_HEADER_UNSUPPORTED: + return ErrorUtils::FormatErrorMessage(kErrorCannotAppendRequestHeader, + base::NumberToString(*rule_id)); case ParseResult::ERROR_REGEX_TOO_LARGE: // These rules are ignored while indexing and so won't cause an error. break; diff --git a/chromium/extensions/browser/api/declarative_net_request/parse_info.h b/chromium/extensions/browser/api/declarative_net_request/parse_info.h index 1bad05c35e3..2d2124a5ae8 100644 --- a/chromium/extensions/browser/api/declarative_net_request/parse_info.h +++ b/chromium/extensions/browser/api/declarative_net_request/parse_info.h @@ -9,7 +9,7 @@ #include <string> #include <vector> -#include "base/logging.h" +#include "base/check.h" #include "base/optional.h" #include "extensions/browser/api/declarative_net_request/constants.h" diff --git a/chromium/extensions/browser/api/declarative_net_request/regex_rules_matcher.cc b/chromium/extensions/browser/api/declarative_net_request/regex_rules_matcher.cc index f8907712101..18c1cdc4581 100644 --- a/chromium/extensions/browser/api/declarative_net_request/regex_rules_matcher.cc +++ b/chromium/extensions/browser/api/declarative_net_request/regex_rules_matcher.cc @@ -6,6 +6,7 @@ #include <algorithm> +#include "base/logging.h" #include "base/strings/string_util.h" #include "base/strings/stringprintf.h" #include "components/url_pattern_index/url_pattern_index.h" @@ -96,13 +97,20 @@ RegexRulesMatcher::RegexRulesMatcher(const ExtensionId& extension_id, RegexRulesMatcher::~RegexRulesMatcher() = default; std::vector<RequestAction> RegexRulesMatcher::GetModifyHeadersActions( - const RequestParams& params) const { + const RequestParams& params, + base::Optional<uint64_t> min_priority) const { const std::vector<RegexRuleInfo>& potential_matches = GetPotentialMatches(params); std::vector<const flat_rule::UrlRule*> rules; for (const RegexRuleInfo& info : potential_matches) { - if (info.regex_rule->action_type() == flat::ActionType_modify_headers && + // Check for the rule's priority iff |min_priority| is specified. + bool has_sufficient_priority = + !min_priority || + info.regex_rule->url_rule()->priority() > *min_priority; + + if (has_sufficient_priority && + info.regex_rule->action_type() == flat::ActionType_modify_headers && re2::RE2::PartialMatch(params.url->spec(), *info.regex)) { rules.push_back(info.regex_rule->url_rule()); } diff --git a/chromium/extensions/browser/api/declarative_net_request/regex_rules_matcher.h b/chromium/extensions/browser/api/declarative_net_request/regex_rules_matcher.h index 5db5980e752..85b75bdbb5b 100644 --- a/chromium/extensions/browser/api/declarative_net_request/regex_rules_matcher.h +++ b/chromium/extensions/browser/api/declarative_net_request/regex_rules_matcher.h @@ -58,7 +58,8 @@ class RegexRulesMatcher final : public RulesetMatcherBase { // RulesetMatcherBase override: ~RegexRulesMatcher() override; std::vector<RequestAction> GetModifyHeadersActions( - const RequestParams& params) const override; + const RequestParams& params, + base::Optional<uint64_t> min_priority) const override; bool IsExtraHeadersMatcher() const override { return is_extra_headers_matcher_; } diff --git a/chromium/extensions/browser/api/declarative_net_request/request_action.cc b/chromium/extensions/browser/api/declarative_net_request/request_action.cc index aab953ca723..d6e19391b0f 100644 --- a/chromium/extensions/browser/api/declarative_net_request/request_action.cc +++ b/chromium/extensions/browser/api/declarative_net_request/request_action.cc @@ -4,6 +4,7 @@ #include "extensions/browser/api/declarative_net_request/request_action.h" +#include <tuple> #include <utility> #include "extensions/browser/api/declarative_net_request/flat/extension_ruleset_generated.h" @@ -20,6 +21,10 @@ namespace dnr_api = api::declarative_net_request; dnr_api::HeaderOperation ConvertFlatHeaderOperation( flat::HeaderOperation operation) { switch (operation) { + case flat::HeaderOperation_append: + return dnr_api::HEADER_OPERATION_APPEND; + case flat::HeaderOperation_set: + return dnr_api::HEADER_OPERATION_SET; case flat::HeaderOperation_remove: return dnr_api::HEADER_OPERATION_REMOVE; } @@ -28,12 +33,27 @@ dnr_api::HeaderOperation ConvertFlatHeaderOperation( } // namespace RequestAction::HeaderInfo::HeaderInfo(std::string header, - dnr_api::HeaderOperation operation) - : header(std::move(header)), operation(operation) {} + dnr_api::HeaderOperation operation, + base::Optional<std::string> value) + : header(std::move(header)), + operation(operation), + value(std::move(value)) {} RequestAction::HeaderInfo::HeaderInfo(const flat::ModifyHeaderInfo& info) : header(CreateString<std::string>(*info.header())), - operation(ConvertFlatHeaderOperation(info.operation())) {} + operation(ConvertFlatHeaderOperation(info.operation())) { + if (info.value()) + value = CreateString<std::string>(*info.value()); +} + +RequestAction::HeaderInfo::~HeaderInfo() = default; +RequestAction::HeaderInfo::HeaderInfo(const RequestAction::HeaderInfo& other) = + default; +RequestAction::HeaderInfo& RequestAction::HeaderInfo::operator=( + const RequestAction::HeaderInfo& other) = default; +RequestAction::HeaderInfo::HeaderInfo(RequestAction::HeaderInfo&&) = default; +RequestAction::HeaderInfo& RequestAction::HeaderInfo::operator=( + RequestAction::HeaderInfo&&) = default; RequestAction::RequestAction(RequestAction::Type type, uint32_t rule_id, @@ -56,6 +76,16 @@ RequestAction RequestAction::Clone() const { RequestAction::RequestAction(const RequestAction&) = default; +bool operator<(const RequestAction& lhs, const RequestAction& rhs) { + return std::tie(lhs.index_priority, lhs.ruleset_id, lhs.rule_id) < + std::tie(rhs.index_priority, rhs.ruleset_id, rhs.rule_id); +} + +bool operator>(const RequestAction& lhs, const RequestAction& rhs) { + return std::tie(lhs.index_priority, lhs.ruleset_id, lhs.rule_id) > + std::tie(rhs.index_priority, rhs.ruleset_id, rhs.rule_id); +} + base::Optional<RequestAction> GetMaxPriorityAction( base::Optional<RequestAction> lhs, base::Optional<RequestAction> rhs) { @@ -63,8 +93,7 @@ base::Optional<RequestAction> GetMaxPriorityAction( return rhs; if (!rhs) return lhs; - return lhs->index_priority >= rhs->index_priority ? std::move(lhs) - : std::move(rhs); + return lhs > rhs ? std::move(lhs) : std::move(rhs); } } // namespace declarative_net_request diff --git a/chromium/extensions/browser/api/declarative_net_request/request_action.h b/chromium/extensions/browser/api/declarative_net_request/request_action.h index 7bc7be29dbd..96781f28262 100644 --- a/chromium/extensions/browser/api/declarative_net_request/request_action.h +++ b/chromium/extensions/browser/api/declarative_net_request/request_action.h @@ -29,12 +29,20 @@ struct RequestAction { // This is used instead of ModifyHeaderInfo so it can be copied in Clone(). struct HeaderInfo { HeaderInfo(std::string header, - api::declarative_net_request::HeaderOperation operation); + api::declarative_net_request::HeaderOperation operation, + base::Optional<std::string> value); explicit HeaderInfo(const flat::ModifyHeaderInfo& info); + ~HeaderInfo(); + HeaderInfo(const HeaderInfo& other); + HeaderInfo& operator=(const HeaderInfo& other); + HeaderInfo(HeaderInfo&&); + HeaderInfo& operator=(HeaderInfo&&); // The name of the header to be modified, specified in lowercase. std::string header; api::declarative_net_request::HeaderOperation operation; + // The value for |header| to be appended or set. + base::Optional<std::string> value; }; enum class Type { @@ -112,6 +120,11 @@ struct RequestAction { RequestAction(const RequestAction&); }; +// Compares RequestAction by |index_priority|, breaking ties by |ruleset_id| +// then |rule_id|. +bool operator<(const RequestAction& lhs, const RequestAction& rhs); +bool operator>(const RequestAction& lhs, const RequestAction& rhs); + base::Optional<RequestAction> GetMaxPriorityAction( base::Optional<RequestAction> lhs, base::Optional<RequestAction> rhs); diff --git a/chromium/extensions/browser/api/declarative_net_request/request_params.h b/chromium/extensions/browser/api/declarative_net_request/request_params.h index bcb169e8a2b..0b1eb4e5ee1 100644 --- a/chromium/extensions/browser/api/declarative_net_request/request_params.h +++ b/chromium/extensions/browser/api/declarative_net_request/request_params.h @@ -22,7 +22,7 @@ namespace extensions { struct WebRequestInfo; namespace declarative_net_request { -class RulesetMatcher; +class CompositeMatcher; // Struct to hold parameters for a network request. struct RequestParams { @@ -44,10 +44,12 @@ struct RequestParams { // ID of the parent RenderFrameHost. content::GlobalFrameRoutingId parent_routing_id; - // A map from RulesetMatchers to whether it has a matching allow or - // allowAllRequests rule. Used as a cache to prevent additional calls to + // A map from CompositeMatcher to the priority of its highest priority + // matching allow or allowAllRequests rule if there is one, or base::nullopt + // otherwise. Used as a cache to prevent additional calls to // GetBeforeRequestAction. - mutable base::flat_map<const RulesetMatcher*, bool> allow_rule_cache; + mutable base::flat_map<const CompositeMatcher*, base::Optional<uint64_t>> + allow_rule_max_priority; // Lower cased url, used for regex matching. Cached for performance. mutable base::Optional<std::string> lower_cased_url_spec; diff --git a/chromium/extensions/browser/api/declarative_net_request/rules_monitor_service.cc b/chromium/extensions/browser/api/declarative_net_request/rules_monitor_service.cc index 1b00322037b..6e1123b8fe9 100644 --- a/chromium/extensions/browser/api/declarative_net_request/rules_monitor_service.cc +++ b/chromium/extensions/browser/api/declarative_net_request/rules_monitor_service.cc @@ -14,6 +14,7 @@ #include "base/lazy_instance.h" #include "base/location.h" #include "base/memory/ptr_util.h" +#include "base/metrics/histogram_macros.h" #include "base/stl_util.h" #include "base/task/post_task.h" #include "base/threading/thread_restrictions.h" @@ -55,6 +56,10 @@ bool RulesetInfoCompareByID(const RulesetInfo& lhs, const RulesetInfo& rhs) { return lhs.source().id() < rhs.source().id(); } +void LogLoadRulesetResult(LoadRulesetResult result) { + UMA_HISTOGRAM_ENUMERATION(kLoadRulesetResultHistogram, result); +} + } // namespace // Helper to bridge tasks to FileSequenceHelper. Lives on the UI thread. @@ -223,6 +228,7 @@ void RulesMonitorService::OnExtensionLoaded( if (!prefs_->GetDNRStaticRulesetChecksum(extension->id(), source.id(), &expected_ruleset_checksum)) { // This might happen on prefs corruption. + LogLoadRulesetResult(LoadRulesetResult::kErrorChecksumNotFound); ruleset_failed_to_load = true; continue; } @@ -306,9 +312,8 @@ void RulesMonitorService::OnExtensionUninstalled( RulesetSource::CreateDynamic(browser_context, extension->id()); DCHECK_EQ(source.json_path().DirName(), source.indexed_path().DirName()); GetExtensionFileTaskRunner()->PostTask( - FROM_HERE, - base::BindOnce(base::IgnoreResult(&base::DeleteFile), - source.json_path().DirName(), false /* recursive */)); + FROM_HERE, base::BindOnce(base::GetDeleteFileCallback(), + source.json_path().DirName())); } void RulesMonitorService::UpdateDynamicRulesInternal( @@ -356,21 +361,16 @@ void RulesMonitorService::UpdateEnabledStaticRulesetsInternal( LoadRequestData load_data(extension_id); - // Don't hop to the file sequence if there are no rulesets to load. - // TODO(karandeepb): Hop to the file sequence in this case as well to ensure - // that subsequent updateEnabledRulesets calls complete in FIFO order. - if (ids_to_enable.empty()) { - OnNewStaticRulesetsLoaded(std::move(callback), std::move(ids_to_disable), - std::move(ids_to_enable), std::move(load_data)); - return; - } + // Don't short-circuit the case of |ids_to_enable| being empty by calling + // OnNewStaticRulesetsLoaded directly. This can interfere with the expected + // FIFO ordering of updateEnabledRulesets calls. int expected_ruleset_checksum = -1; for (const RulesetID& id_to_enable : ids_to_enable) { if (!prefs_->GetDNRStaticRulesetChecksum(extension_id, id_to_enable, &expected_ruleset_checksum)) { // This might happen on prefs corruption. - // TODO(crbug.com/754526): Log metrics on how often this happens. + LogLoadRulesetResult(LoadRulesetResult::kErrorChecksumNotFound); std::move(callback).Run(kInternalErrorUpdatingEnabledRulesets); return; } @@ -405,7 +405,7 @@ void RulesMonitorService::OnInitialRulesetsLoaded(LoadRequestData load_data) { if (test_observer_) test_observer_->OnRulesetLoadComplete(load_data.extension_id); - UpdateRulesetChecksumsIfNeeded(load_data); + LogMetricsAndUpdateChecksumsIfNeeded(load_data); // It's possible that the extension has been disabled since the initial load // ruleset request. If it's disabled, do nothing. @@ -441,7 +441,7 @@ void RulesMonitorService::OnInitialRulesetsLoaded(LoadRequestData load_data) { // Per-ruleset limits should have been enforced during // indexing/installation. DCHECK_LE(matcher->GetRegexRulesCount(), - static_cast<size_t>(dnr_api::MAX_NUMBER_OF_REGEX_RULES)); + static_cast<size_t>(GetRegexRuleLimit())); DCHECK_LE(matcher->GetRulesCount(), ruleset.source().rule_count_limit()); if (ruleset.source().is_dynamic_ruleset()) { @@ -450,13 +450,12 @@ void RulesMonitorService::OnInitialRulesetsLoaded(LoadRequestData load_data) { } size_t new_rules_count = static_rules_count + matcher->GetRulesCount(); - if (new_rules_count > static_cast<size_t>(dnr_api::MAX_NUMBER_OF_RULES)) + if (new_rules_count > static_cast<size_t>(GetStaticRuleLimit())) continue; size_t new_regex_rules_count = static_regex_rules_count + matcher->GetRegexRulesCount(); - if (new_regex_rules_count > - static_cast<size_t>(dnr_api::MAX_NUMBER_OF_REGEX_RULES)) { + if (new_regex_rules_count > static_cast<size_t>(GetRegexRuleLimit())) { continue; } @@ -482,7 +481,7 @@ void RulesMonitorService::OnNewStaticRulesetsLoaded( std::set<RulesetID> ids_to_disable, std::set<RulesetID> ids_to_enable, LoadRequestData load_data) { - UpdateRulesetChecksumsIfNeeded(load_data); + LogMetricsAndUpdateChecksumsIfNeeded(load_data); // It's possible that the extension has been disabled since the initial // request. If it's disabled, return early. @@ -523,7 +522,6 @@ void RulesMonitorService::OnNewStaticRulesetsLoaded( new_matchers.reserve(load_data.rulesets.size()); for (RulesetInfo& ruleset : load_data.rulesets) { if (!ruleset.did_load_successfully()) { - // TODO(crbug.com/754526): Log metrics on how often this happens. std::move(callback).Run(kInternalErrorUpdatingEnabledRulesets); return; } @@ -533,7 +531,7 @@ void RulesMonitorService::OnNewStaticRulesetsLoaded( // Per-ruleset limits should have been enforced during // indexing/installation. DCHECK_LE(matcher->GetRegexRulesCount(), - static_cast<size_t>(dnr_api::MAX_NUMBER_OF_REGEX_RULES)); + static_cast<size_t>(GetRegexRuleLimit())); DCHECK_LE(matcher->GetRulesCount(), ruleset.source().rule_count_limit()); static_rules_count += matcher->GetRulesCount(); @@ -541,13 +539,12 @@ void RulesMonitorService::OnNewStaticRulesetsLoaded( new_matchers.push_back(std::move(matcher)); } - if (static_rules_count > static_cast<size_t>(dnr_api::MAX_NUMBER_OF_RULES)) { + if (static_rules_count > static_cast<size_t>(GetStaticRuleLimit())) { std::move(callback).Run(kEnabledRulesetsRuleCountExceeded); return; } - if (static_regex_rules_count > - static_cast<size_t>(dnr_api::MAX_NUMBER_OF_REGEX_RULES)) { + if (static_regex_rules_count > static_cast<size_t>(GetRegexRuleLimit())) { std::move(callback).Run(kEnabledRulesetsRegexRuleCountExceeded); return; } @@ -564,26 +561,8 @@ void RulesMonitorService::OnNewStaticRulesetsLoaded( bool had_extra_headers_matcher = ruleset_manager_.HasAnyExtraHeadersMatcher(); - // Do another pass over the existing matchers for the extension to compute the - // final set of matchers. - { - CompositeMatcher::MatcherList old_matchers = matcher->GetAndResetMatchers(); - base::EraseIf(old_matchers, - [&ids_to_disable, &ids_to_enable]( - const std::unique_ptr<RulesetMatcher>& matcher) { - // We also check |ids_to_enable| to omit duplicate matchers. - // |new_matchers| already contains RulesetMatchers - // corresponding to |ids_to_enable|. - return base::Contains(ids_to_disable, matcher->id()) || - base::Contains(ids_to_enable, matcher->id()); - }); - - new_matchers.insert(new_matchers.end(), - std::make_move_iterator(old_matchers.begin()), - std::make_move_iterator(old_matchers.end())); - } - - matcher->SetMatchers(std::move(new_matchers)); + matcher->RemoveRulesetsWithIDs(ids_to_disable); + matcher->AddOrUpdateRulesets(std::move(new_matchers)); prefs_->SetDNREnabledStaticRulesets(load_data.extension_id, matcher->ComputeStaticRulesetIDs()); @@ -615,34 +594,14 @@ void RulesMonitorService::OnDynamicRulesUpdated( base::Optional<std::string> error) { DCHECK_EQ(1u, load_data.rulesets.size()); - RulesetInfo& dynamic_ruleset = load_data.rulesets[0]; - DCHECK_EQ(dynamic_ruleset.did_load_successfully(), !error.has_value()); - - // The extension may have been uninstalled by this point. Return early if - // that's the case. - if (!extension_registry_->GetInstalledExtension(load_data.extension_id)) { - // Still dispatch the |callback|, even though it's probably a no-op. - std::move(callback).Run(std::move(error)); - return; - } - - // Update the ruleset checksums if needed. Note it's possible that - // new_checksum() is valid while did_load_successfully() returns false below. - // This should be rare but can happen when updating the rulesets succeeds but - // we fail to create a RulesetMatcher from the indexed ruleset file (e.g. due - // to a file read error). We still update the prefs checksum to ensure the - // next ruleset load succeeds. - // Note: We also do this for a non-enabled extension. The ruleset on the disk - // has already been modified at this point. So we do want to update the - // checksum for it to be in sync with what's on disk. - if (dynamic_ruleset.new_checksum()) { - prefs_->SetDNRDynamicRulesetChecksum(load_data.extension_id, - *dynamic_ruleset.new_checksum()); - } + LogMetricsAndUpdateChecksumsIfNeeded(load_data); // Respond to the extension. std::move(callback).Run(std::move(error)); + RulesetInfo& dynamic_ruleset = load_data.rulesets[0]; + DCHECK_EQ(dynamic_ruleset.did_load_successfully(), !error.has_value()); + if (!dynamic_ruleset.did_load_successfully()) return; @@ -709,8 +668,15 @@ void RulesMonitorService::AdjustExtraHeaderListenerCountIfNeeded( } } -void RulesMonitorService::UpdateRulesetChecksumsIfNeeded( +void RulesMonitorService::LogMetricsAndUpdateChecksumsIfNeeded( const LoadRequestData& load_data) { + for (const RulesetInfo& ruleset : load_data.rulesets) { + // The |load_ruleset_result()| might be empty if CreateVerifiedMatcher + // wasn't called on the ruleset. + if (ruleset.load_ruleset_result()) + LogLoadRulesetResult(*ruleset.load_ruleset_result()); + } + // The extension may have been uninstalled by this point. Return early if // that's the case. if (!extension_registry_->GetInstalledExtension(load_data.extension_id)) diff --git a/chromium/extensions/browser/api/declarative_net_request/rules_monitor_service.h b/chromium/extensions/browser/api/declarative_net_request/rules_monitor_service.h index 72b4c52a1ce..60ba79a2ad3 100644 --- a/chromium/extensions/browser/api/declarative_net_request/rules_monitor_service.h +++ b/chromium/extensions/browser/api/declarative_net_request/rules_monitor_service.h @@ -183,8 +183,9 @@ class RulesMonitorService : public BrowserContextKeyedAPI, // RulesetManager had an extra headers matcher before the update. void AdjustExtraHeaderListenerCountIfNeeded(bool had_extra_headers_matcher); - // Updates ruleset checksum in preferences from |load_data|. - void UpdateRulesetChecksumsIfNeeded(const LoadRequestData& load_data); + // Logs metrics related to the result of loading rulesets and updates ruleset + // checksum in preferences from |load_data|. + void LogMetricsAndUpdateChecksumsIfNeeded(const LoadRequestData& load_data); ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver> registry_observer_{this}; diff --git a/chromium/extensions/browser/api/declarative_net_request/ruleset_manager.cc b/chromium/extensions/browser/api/declarative_net_request/ruleset_manager.cc index 2701918b228..7437d6ff597 100644 --- a/chromium/extensions/browser/api/declarative_net_request/ruleset_manager.cc +++ b/chromium/extensions/browser/api/declarative_net_request/ruleset_manager.cc @@ -16,7 +16,6 @@ #include "base/stl_util.h" #include "base/time/time.h" #include "base/timer/elapsed_timer.h" -#include "components/version_info/channel.h" #include "components/web_cache/browser/web_cache_manager.h" #include "extensions/browser/api/declarative_net_request/composite_matcher.h" #include "extensions/browser/api/declarative_net_request/constants.h" @@ -32,7 +31,6 @@ #include "extensions/common/api/declarative_net_request.h" #include "extensions/common/api/declarative_net_request/utils.h" #include "extensions/common/constants.h" -#include "extensions/common/features/feature_channel.h" #include "url/origin.h" namespace extensions { @@ -415,15 +413,11 @@ std::vector<RequestAction> RulesetManager::EvaluateRequestInternal( return actions; } - // TODO(crbug.com/947591): Remove the channel check once implementation of - // modifyHeaders action is complete. - if (GetCurrentChannel() != version_info::Channel::STABLE) { - std::vector<RequestAction> modify_headers_actions = - GetModifyHeadersActions(rulesets_to_evaluate, request, params); + std::vector<RequestAction> modify_headers_actions = + GetModifyHeadersActions(rulesets_to_evaluate, request, params); - if (!modify_headers_actions.empty()) - return modify_headers_actions; - } + if (!modify_headers_actions.empty()) + return modify_headers_actions; return actions; } diff --git a/chromium/extensions/browser/api/declarative_net_request/ruleset_matcher.cc b/chromium/extensions/browser/api/declarative_net_request/ruleset_matcher.cc index b76d4917edf..9c2779095b7 100644 --- a/chromium/extensions/browser/api/declarative_net_request/ruleset_matcher.cc +++ b/chromium/extensions/browser/api/declarative_net_request/ruleset_matcher.cc @@ -23,7 +23,7 @@ namespace extensions { namespace declarative_net_request { // static -RulesetMatcher::LoadRulesetResult RulesetMatcher::CreateVerifiedMatcher( +LoadRulesetResult RulesetMatcher::CreateVerifiedMatcher( const RulesetSource& source, int expected_ruleset_checksum, std::unique_ptr<RulesetMatcher>* matcher) { @@ -33,21 +33,21 @@ RulesetMatcher::LoadRulesetResult RulesetMatcher::CreateVerifiedMatcher( base::ElapsedTimer timer; if (!base::PathExists(source.indexed_path())) - return kLoadErrorInvalidPath; + return LoadRulesetResult::kErrorInvalidPath; std::string ruleset_data; if (!base::ReadFileToString(source.indexed_path(), &ruleset_data)) - return kLoadErrorFileRead; + return LoadRulesetResult::kErrorCannotReadFile; if (!StripVersionHeaderAndParseVersion(&ruleset_data)) - return kLoadErrorVersionMismatch; + return LoadRulesetResult::kErrorVersionMismatch; // This guarantees that no memory access will end up outside the buffer. if (!IsValidRulesetData( base::make_span(reinterpret_cast<const uint8_t*>(ruleset_data.data()), ruleset_data.size()), expected_ruleset_checksum)) { - return kLoadErrorChecksumMismatch; + return LoadRulesetResult::kErrorChecksumMismatch; } UMA_HISTOGRAM_TIMES( @@ -58,7 +58,7 @@ RulesetMatcher::LoadRulesetResult RulesetMatcher::CreateVerifiedMatcher( // constructor. *matcher = base::WrapUnique(new RulesetMatcher( std::move(ruleset_data), source.id(), source.extension_id())); - return kLoadSuccess; + return LoadRulesetResult::kSuccess; } RulesetMatcher::~RulesetMatcher() = default; @@ -71,12 +71,13 @@ base::Optional<RequestAction> RulesetMatcher::GetBeforeRequestAction( } std::vector<RequestAction> RulesetMatcher::GetModifyHeadersActions( - const RequestParams& params) const { + const RequestParams& params, + base::Optional<uint64_t> min_priority) const { std::vector<RequestAction> modify_header_actions = - url_pattern_index_matcher_.GetModifyHeadersActions(params); + url_pattern_index_matcher_.GetModifyHeadersActions(params, min_priority); std::vector<RequestAction> regex_modify_header_actions = - regex_matcher_.GetModifyHeadersActions(params); + regex_matcher_.GetModifyHeadersActions(params, min_priority); modify_header_actions.insert( modify_header_actions.end(), diff --git a/chromium/extensions/browser/api/declarative_net_request/ruleset_matcher.h b/chromium/extensions/browser/api/declarative_net_request/ruleset_matcher.h index 728abb28c10..0a964b9a98d 100644 --- a/chromium/extensions/browser/api/declarative_net_request/ruleset_matcher.h +++ b/chromium/extensions/browser/api/declarative_net_request/ruleset_matcher.h @@ -22,6 +22,7 @@ namespace extensions { namespace declarative_net_request { class RulesetSource; +enum class LoadRulesetResult; namespace flat { struct ExtensionIndexedRuleset; @@ -35,35 +36,8 @@ struct UrlRuleMetadata; // inherits from RulesetMatcherBase. class RulesetMatcher { public: - // Describes the result of creating a RulesetMatcher instance. - // This is logged as part of UMA. Hence existing values should not be re- - // numbered or deleted. New values should be added before kLoadRulesetMax. - enum LoadRulesetResult { - // Ruleset loading succeeded. - kLoadSuccess = 0, - - // Ruleset loading failed since the provided path did not exist. - kLoadErrorInvalidPath = 1, - - // Ruleset loading failed due to a file read error. - kLoadErrorFileRead = 2, - - // Ruleset loading failed due to a checksum mismatch. - kLoadErrorChecksumMismatch = 3, - - // Ruleset loading failed due to version header mismatch. - // TODO(karandeepb): This should be split into two cases: - // - When the indexed ruleset doesn't have the version header in the - // correct format. - // - When the indexed ruleset's version is not the same as that used by - // Chrome. - kLoadErrorVersionMismatch = 4, - - kLoadResultMax - }; - // Factory function to create a verified RulesetMatcher for |source|. Must be - // called on a sequence where file IO is allowed. Returns kLoadSuccess on + // called on a sequence where file IO is allowed. Returns kSuccess on // success along with the ruleset |matcher|. static LoadRulesetResult CreateVerifiedMatcher( const RulesetSource& source, @@ -74,8 +48,12 @@ class RulesetMatcher { base::Optional<RequestAction> GetBeforeRequestAction( const RequestParams& params) const; + + // Returns a list of actions corresponding to all matched + // modifyHeaders rules with priority greater than |min_priority| if specified. std::vector<RequestAction> GetModifyHeadersActions( - const RequestParams& params) const; + const RequestParams& params, + base::Optional<uint64_t> min_priority) const; bool IsExtraHeadersMatcher() const; size_t GetRulesCount() const; diff --git a/chromium/extensions/browser/api/declarative_net_request/ruleset_matcher_base.h b/chromium/extensions/browser/api/declarative_net_request/ruleset_matcher_base.h index 6abe5602393..34c62a64607 100644 --- a/chromium/extensions/browser/api/declarative_net_request/ruleset_matcher_base.h +++ b/chromium/extensions/browser/api/declarative_net_request/ruleset_matcher_base.h @@ -41,9 +41,11 @@ class RulesetMatcherBase { base::Optional<RequestAction> GetBeforeRequestAction( const RequestParams& params) const; - // Returns a vector of RequestAction for all matching modifyHeaders rules. + // Returns a vector of RequestAction for all matching modifyHeaders rules + // with priority greater than |min_priority| if specified. virtual std::vector<RequestAction> GetModifyHeadersActions( - const RequestParams& params) const = 0; + const RequestParams& params, + base::Optional<uint64_t> min_priority) const = 0; // Returns whether this modifies "extraHeaders". virtual bool IsExtraHeadersMatcher() const = 0; diff --git a/chromium/extensions/browser/api/declarative_net_request/ruleset_matcher_unittest.cc b/chromium/extensions/browser/api/declarative_net_request/ruleset_matcher_unittest.cc index 9643aa5a875..63d4011a0d4 100644 --- a/chromium/extensions/browser/api/declarative_net_request/ruleset_matcher_unittest.cc +++ b/chromium/extensions/browser/api/declarative_net_request/ruleset_matcher_unittest.cc @@ -4,6 +4,7 @@ #include "extensions/browser/api/declarative_net_request/ruleset_matcher.h" +#include <limits> #include <utility> #include <vector> @@ -13,7 +14,6 @@ #include "base/strings/string_util.h" #include "base/strings/stringprintf.h" #include "components/url_pattern_index/flat/url_pattern_index_generated.h" -#include "components/version_info/channel.h" #include "content/public/browser/render_process_host.h" #include "content/public/browser/web_contents.h" #include "content/public/test/navigation_simulator.h" @@ -29,7 +29,6 @@ #include "extensions/common/api/declarative_net_request.h" #include "extensions/common/api/declarative_net_request/constants.h" #include "extensions/common/api/declarative_net_request/test_utils.h" -#include "extensions/common/features/feature_channel.h" #include "net/http/http_request_headers.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -166,7 +165,7 @@ TEST_F(RulesetMatcherTest, FailedVerification) { std::string data = "invalid data"; ASSERT_EQ(static_cast<int>(data.size()), base::WriteFile(source.indexed_path(), data.c_str(), data.size())); - EXPECT_EQ(RulesetMatcher::kLoadErrorVersionMismatch, + EXPECT_EQ(LoadRulesetResult::kErrorVersionMismatch, RulesetMatcher::CreateVerifiedMatcher(source, expected_checksum, &matcher)); @@ -176,16 +175,12 @@ TEST_F(RulesetMatcherTest, FailedVerification) { data = GetVersionHeaderForTesting() + "invalid data"; ASSERT_EQ(static_cast<int>(data.size()), base::WriteFile(source.indexed_path(), data.c_str(), data.size())); - EXPECT_EQ(RulesetMatcher::kLoadErrorChecksumMismatch, + EXPECT_EQ(LoadRulesetResult::kErrorChecksumMismatch, RulesetMatcher::CreateVerifiedMatcher(source, expected_checksum, &matcher)); } TEST_F(RulesetMatcherTest, ModifyHeaders_IsExtraHeaderMatcher) { - // TODO(crbug.com/947591): Remove the channel override once implementation of - // modifyHeaders action is complete. - ScopedCurrentChannel channel(::version_info::Channel::UNKNOWN); - TestRule rule = CreateGenericRule(); rule.condition->url_filter = std::string("example.com"); std::unique_ptr<RulesetMatcher> matcher; @@ -193,33 +188,29 @@ TEST_F(RulesetMatcherTest, ModifyHeaders_IsExtraHeaderMatcher) { EXPECT_FALSE(matcher->IsExtraHeadersMatcher()); rule.action->type = std::string("modifyHeaders"); - rule.action->response_headers = - std::vector<TestHeaderInfo>({TestHeaderInfo("HEADER3", "remove")}); + rule.action->response_headers = std::vector<TestHeaderInfo>( + {TestHeaderInfo("HEADER3", "append", "VALUE3")}); ASSERT_TRUE(CreateVerifiedMatcher({rule}, CreateTemporarySource(), &matcher)); EXPECT_TRUE(matcher->IsExtraHeadersMatcher()); } TEST_F(RulesetMatcherTest, ModifyHeaders) { - // TODO(crbug.com/947591): Remove the channel override once implementation of - // modifyHeaders action is complete. - ScopedCurrentChannel channel(::version_info::Channel::UNKNOWN); - TestRule rule_1 = CreateGenericRule(); rule_1.id = kMinValidID; rule_1.priority = kMinValidPriority + 1; rule_1.condition->url_filter = std::string("example.com"); rule_1.action->type = std::string("modifyHeaders"); - rule_1.action->request_headers = - std::vector<TestHeaderInfo>({TestHeaderInfo("header1", "remove")}); + rule_1.action->request_headers = std::vector<TestHeaderInfo>( + {TestHeaderInfo("header1", "remove", base::nullopt)}); TestRule rule_2 = CreateGenericRule(); rule_2.id = kMinValidID + 1; rule_2.priority = kMinValidPriority; rule_2.condition->url_filter = std::string("example.com"); rule_2.action->type = std::string("modifyHeaders"); - rule_2.action->request_headers = - std::vector<TestHeaderInfo>({TestHeaderInfo("header1", "remove"), - TestHeaderInfo("header2", "remove")}); + rule_2.action->request_headers = std::vector<TestHeaderInfo>( + {TestHeaderInfo("header1", "set", "value1"), + TestHeaderInfo("header2", "remove", base::nullopt)}); std::unique_ptr<RulesetMatcher> matcher; ASSERT_TRUE(CreateVerifiedMatcher({rule_1, rule_2}, CreateTemporarySource(), @@ -233,20 +224,21 @@ TEST_F(RulesetMatcherTest, ModifyHeaders) { params.is_third_party = true; std::vector<RequestAction> modify_header_actions = - matcher->GetModifyHeadersActions(params); + matcher->GetModifyHeadersActions(params, 0u /* min_priority */); RequestAction expected_rule_1_action = CreateRequestActionForTesting( RequestAction::Type::MODIFY_HEADERS, *rule_1.id, *rule_1.priority); - expected_rule_1_action.request_headers_to_modify = { - RequestAction::HeaderInfo("header1", dnr_api::HEADER_OPERATION_REMOVE)}; + expected_rule_1_action.request_headers_to_modify = {RequestAction::HeaderInfo( + "header1", dnr_api::HEADER_OPERATION_REMOVE, base::nullopt)}; RequestAction expected_rule_2_action = CreateRequestActionForTesting( RequestAction::Type::MODIFY_HEADERS, *rule_2.id, *rule_2.priority); expected_rule_2_action.request_headers_to_modify = { - RequestAction::HeaderInfo("header1", dnr_api::HEADER_OPERATION_REMOVE), - RequestAction::HeaderInfo("header2", dnr_api::HEADER_OPERATION_REMOVE)}; + RequestAction::HeaderInfo("header1", dnr_api::HEADER_OPERATION_SET, + "value1"), + RequestAction::HeaderInfo("header2", dnr_api::HEADER_OPERATION_REMOVE, + base::nullopt)}; - ASSERT_EQ(2u, modify_header_actions.size()); EXPECT_THAT(modify_header_actions, testing::UnorderedElementsAre( testing::Eq(testing::ByRef(expected_rule_1_action)), @@ -455,10 +447,6 @@ TEST_F(RulesetMatcherTest, UrlTransform) { // Tests regex rules are evaluated correctly for different action types. TEST_F(RulesetMatcherTest, RegexRules) { - // TODO(crbug.com/947591): Remove the channel override once implementation of - // modifyHeaders action is complete. - ScopedCurrentChannel channel(::version_info::Channel::UNKNOWN); - auto create_regex_rule = [](size_t id, const std::string& regex_filter) { TestRule rule = CreateGenericRule(); rule.id = id; @@ -497,7 +485,7 @@ TEST_F(RulesetMatcherTest, RegexRules) { create_regex_rule(6, R"(^(?:http|https)://[a-z\.]+\.org)"); modify_headers_rule.action->type = "modifyHeaders"; modify_headers_rule.action->request_headers = - std::vector<TestHeaderInfo>({TestHeaderInfo("header1", "remove")}); + std::vector<TestHeaderInfo>({TestHeaderInfo("header1", "set", "value1")}); rules.push_back(modify_headers_rule); std::unique_ptr<RulesetMatcher> matcher; @@ -564,7 +552,8 @@ TEST_F(RulesetMatcherTest, RegexRules) { test_case.expected_modify_header_action = CreateRequestActionForTesting( RequestAction::Type::MODIFY_HEADERS, *modify_headers_rule.id); test_case.expected_modify_header_action->request_headers_to_modify = { - RequestAction::HeaderInfo("header1", dnr_api::HEADER_OPERATION_REMOVE)}; + RequestAction::HeaderInfo("header1", dnr_api::HEADER_OPERATION_SET, + "value1")}; test_cases.push_back(std::move(test_case)); } @@ -584,7 +573,7 @@ TEST_F(RulesetMatcherTest, RegexRules) { matcher->GetBeforeRequestAction(params)); std::vector<RequestAction> modify_header_actions = - matcher->GetModifyHeadersActions(params); + matcher->GetModifyHeadersActions(params, 0u /* min_priority */); if (test_case.expected_modify_header_action) { EXPECT_THAT(modify_header_actions, @@ -862,10 +851,6 @@ TEST_F(RulesetMatcherTest, RegexAndFilterListRules_RedirectPriority) { } TEST_F(RulesetMatcherTest, RegexAndFilterListRules_ModifyHeaders) { - // TODO(crbug.com/947591): Remove the channel override once implementation of - // modifyHeaders action is complete. - ScopedCurrentChannel channel(::version_info::Channel::UNKNOWN); - std::vector<TestRule> rules; TestRule rule = CreateGenericRule(); @@ -873,16 +858,18 @@ TEST_F(RulesetMatcherTest, RegexAndFilterListRules_ModifyHeaders) { rule.priority = kMinValidPriority + 1; rule.action->type = "modifyHeaders"; rule.condition->url_filter = "abc"; - rule.action->request_headers = - std::vector<TestHeaderInfo>({TestHeaderInfo("header1", "remove"), - TestHeaderInfo("header2", "remove")}); + rule.action->request_headers = std::vector<TestHeaderInfo>( + {TestHeaderInfo("header1", "remove", base::nullopt), + TestHeaderInfo("header2", "remove", base::nullopt)}); rules.push_back(rule); RequestAction action_1 = CreateRequestActionForTesting( RequestAction::Type::MODIFY_HEADERS, 1, *rule.priority); action_1.request_headers_to_modify = { - RequestAction::HeaderInfo("header1", dnr_api::HEADER_OPERATION_REMOVE), - RequestAction::HeaderInfo("header2", dnr_api::HEADER_OPERATION_REMOVE)}; + RequestAction::HeaderInfo("header1", dnr_api::HEADER_OPERATION_REMOVE, + base::nullopt), + RequestAction::HeaderInfo("header2", dnr_api::HEADER_OPERATION_REMOVE, + base::nullopt)}; rule = CreateGenericRule(); rule.id = 2; @@ -890,16 +877,18 @@ TEST_F(RulesetMatcherTest, RegexAndFilterListRules_ModifyHeaders) { rule.condition->url_filter.reset(); rule.condition->regex_filter = "example"; rule.action->type = "modifyHeaders"; - rule.action->request_headers = - std::vector<TestHeaderInfo>({TestHeaderInfo("header1", "remove"), - TestHeaderInfo("header3", "remove")}); + rule.action->request_headers = std::vector<TestHeaderInfo>( + {TestHeaderInfo("header1", "remove", base::nullopt), + TestHeaderInfo("header3", "remove", base::nullopt)}); rules.push_back(rule); RequestAction action_2 = CreateRequestActionForTesting( RequestAction::Type::MODIFY_HEADERS, 2, *rule.priority); action_2.request_headers_to_modify = { - RequestAction::HeaderInfo("header1", dnr_api::HEADER_OPERATION_REMOVE), - RequestAction::HeaderInfo("header3", dnr_api::HEADER_OPERATION_REMOVE)}; + RequestAction::HeaderInfo("header1", dnr_api::HEADER_OPERATION_REMOVE, + base::nullopt), + RequestAction::HeaderInfo("header3", dnr_api::HEADER_OPERATION_REMOVE, + base::nullopt)}; std::unique_ptr<RulesetMatcher> matcher; ASSERT_TRUE(CreateVerifiedMatcher(rules, CreateTemporarySource(), &matcher)); @@ -910,7 +899,8 @@ TEST_F(RulesetMatcherTest, RegexAndFilterListRules_ModifyHeaders) { RequestParams params; params.url = &url; - EXPECT_TRUE(matcher->GetModifyHeadersActions(params).empty()); + EXPECT_TRUE(matcher->GetModifyHeadersActions(params, 0u /* min_priority */) + .empty()); } { @@ -920,7 +910,7 @@ TEST_F(RulesetMatcherTest, RegexAndFilterListRules_ModifyHeaders) { params.url = &url; std::vector<RequestAction> actions = - matcher->GetModifyHeadersActions(params); + matcher->GetModifyHeadersActions(params, 0u /* min_priority */); EXPECT_THAT(actions, testing::UnorderedElementsAre( testing::Eq(testing::ByRef(action_1)))); } @@ -932,7 +922,7 @@ TEST_F(RulesetMatcherTest, RegexAndFilterListRules_ModifyHeaders) { params.url = &url; std::vector<RequestAction> actions = - matcher->GetModifyHeadersActions(params); + matcher->GetModifyHeadersActions(params, 0u /* min_priority */); EXPECT_THAT(actions, testing::UnorderedElementsAre( testing::Eq(testing::ByRef(action_2)))); } @@ -945,10 +935,18 @@ TEST_F(RulesetMatcherTest, RegexAndFilterListRules_ModifyHeaders) { params.url = &url; std::vector<RequestAction> actions = - matcher->GetModifyHeadersActions(params); + matcher->GetModifyHeadersActions(params, 0u /* min_priority */); EXPECT_THAT(actions, testing::UnorderedElementsAre( testing::Eq(testing::ByRef(action_1)), testing::Eq(testing::ByRef(action_2)))); + + // GetModifyHeadersActions specifies a minimum priority greater than the + // rules' priority, so no actions should be returned. + EXPECT_TRUE( + matcher + ->GetModifyHeadersActions( + params, std::numeric_limits<uint64_t>::max() /* min_priority */) + .empty()); } } diff --git a/chromium/extensions/browser/api/declarative_net_request/ruleset_source.cc b/chromium/extensions/browser/api/declarative_net_request/ruleset_source.cc index 4189b4b36ac..5f25b196f97 100644 --- a/chromium/extensions/browser/api/declarative_net_request/ruleset_source.cc +++ b/chromium/extensions/browser/api/declarative_net_request/ruleset_source.cc @@ -23,7 +23,6 @@ #include "base/strings/utf_string_conversions.h" #include "base/timer/elapsed_timer.h" #include "base/values.h" -#include "components/version_info/channel.h" #include "content/public/browser/browser_context.h" #include "extensions/browser/api/declarative_net_request/constants.h" #include "extensions/browser/api/declarative_net_request/flat_ruleset_indexer.h" @@ -36,7 +35,6 @@ #include "extensions/common/error_utils.h" #include "extensions/common/extension.h" #include "extensions/common/extension_resource.h" -#include "extensions/common/features/feature_channel.h" #include "extensions/common/file_util.h" #include "extensions/common/install_warning.h" #include "extensions/common/manifest_constants.h" @@ -142,8 +140,7 @@ ReadJSONRulesResult ParseRulesFromJSON(const base::FilePath& json_path, } const bool is_regex_rule = !!parsed_rule.condition.regex_filter; - if (is_regex_rule && - ++regex_rule_count > dnr_api::MAX_NUMBER_OF_REGEX_RULES) { + if (is_regex_rule && ++regex_rule_count > GetRegexRuleLimit()) { // Only add the install warning once. if (!regex_rule_count_exceeded) { regex_rule_count_exceeded = true; @@ -317,7 +314,7 @@ RulesetSource RulesetSource::CreateStatic( extension.path().Append(info.relative_path), extension.path().Append( file_util::GetIndexedRulesetRelativePath(info.id.value())), - info.id, dnr_api::MAX_NUMBER_OF_RULES, extension.id(), info.enabled); + info.id, GetStaticRuleLimit(), extension.id(), info.enabled); } // static @@ -330,7 +327,7 @@ RulesetSource RulesetSource::CreateDynamic(content::BrowserContext* context, return RulesetSource( dynamic_ruleset_directory.AppendASCII(kDynamicRulesJSONFilename), dynamic_ruleset_directory.AppendASCII(kDynamicIndexedRulesFilename), - kDynamicRulesetID, dnr_api::MAX_NUMBER_OF_DYNAMIC_RULES, extension_id, + kDynamicRulesetID, GetDynamicRuleLimit(), extension_id, true /* enabled_by_default */); } @@ -414,15 +411,6 @@ ParseInfo RulesetSource::IndexAndPersistRules( if (!inserted) return ParseInfo(ParseResult::ERROR_DUPLICATE_IDS, &rule_id); - // Ensure modifyHeaders actions don't have any side-effects on Stable - // since it's under development. - // TODO(crbug.com/947591): Remove the channel check once implementation - // of modifyHeaders action is complete. - if (rule.action.type == dnr_api::RULE_ACTION_TYPE_MODIFYHEADERS && - GetCurrentChannel() == version_info::Channel::STABLE) { - continue; - } - IndexedRule indexed_rule; ParseResult parse_result = IndexedRule::CreateIndexedRule( std::move(rule), base_url, &indexed_rule); diff --git a/chromium/extensions/browser/api/declarative_net_request/test_utils.cc b/chromium/extensions/browser/api/declarative_net_request/test_utils.cc index 34eeec46669..a6c468bd1f8 100644 --- a/chromium/extensions/browser/api/declarative_net_request/test_utils.cc +++ b/chromium/extensions/browser/api/declarative_net_request/test_utils.cc @@ -61,8 +61,13 @@ bool operator==(const RequestAction::HeaderInfo& lhs, std::ostream& operator<<(std::ostream& output, const RequestAction::HeaderInfo& header_info) { - return output << dnr_api::ToString(header_info.operation) << ":" - << header_info.header; + output << "\nRequestAction::HeaderInfo\n"; + output << "\t|operation| " << dnr_api::ToString(header_info.operation) + << "\n"; + output << "\t|header| " << header_info.header << "\n"; + output << "\t|value| " + << (header_info.value ? *header_info.value : std::string("nullopt")); + return output; } // Note: This is not declared in the anonymous namespace so that we can use it @@ -83,8 +88,8 @@ bool operator==(const RequestAction& lhs, const RequestAction& rhs) { std::vector<RequestAction::HeaderInfo> b) { auto header_info_comparator = [](const RequestAction::HeaderInfo& lhs, const RequestAction::HeaderInfo& rhs) { - return std::make_pair(lhs.header, lhs.operation) > - std::make_pair(rhs.header, rhs.operation); + return std::make_tuple(lhs.header, lhs.operation, lhs.value) > + std::make_tuple(rhs.header, rhs.operation, rhs.value); }; std::sort(a.begin(), a.end(), header_info_comparator); @@ -138,9 +143,9 @@ std::ostream& operator<<(std::ostream& output, const RequestAction& action) { output << "|index_priority| " << action.index_priority << "\n"; output << "|ruleset_id| " << action.ruleset_id << "\n"; output << "|extension_id| " << action.extension_id << "\n"; - output << "|request_headers_to_modify| " + output << "|request_headers_to_modify|" << ::testing::PrintToString(action.request_headers_to_modify) << "\n"; - output << "|response_headers_to_modify| " + output << "|response_headers_to_modify|" << ::testing::PrintToString(action.response_headers_to_modify); return output; } @@ -238,6 +243,21 @@ std::ostream& operator<<(std::ostream& output, const ParseResult& result) { case ParseResult::ERROR_INVALID_REGEX_FILTER: output << "ERROR_INVALID_REGEX_FILTER"; break; + case ParseResult::ERROR_REGEX_TOO_LARGE: + output << "ERROR_REGEX_TOO_LARGE"; + break; + case ParseResult::ERROR_MULTIPLE_FILTERS_SPECIFIED: + output << "ERROR_MULTIPLE_FILTERS_SPECIFIED"; + break; + case ParseResult::ERROR_REGEX_SUBSTITUTION_WITHOUT_FILTER: + output << "ERROR_REGEX_SUBSTITUTION_WITHOUT_FILTER"; + break; + case ParseResult::ERROR_INVALID_REGEX_SUBSTITUTION: + output << "ERROR_INVALID_REGEX_SUBSTITUTION"; + break; + case ParseResult::ERROR_INVALID_ALLOW_ALL_REQUESTS_RESOURCE_TYPE: + output << "ERROR_INVALID_ALLOW_ALL_REQUESTS_RESOURCE_TYPE"; + break; case ParseResult::ERROR_NO_HEADERS_SPECIFIED: output << "ERROR_NO_HEADERS_SPECIFIED"; break; @@ -250,20 +270,41 @@ std::ostream& operator<<(std::ostream& output, const ParseResult& result) { case ParseResult::ERROR_INVALID_HEADER_NAME: output << "ERROR_INVALID_HEADER_NAME"; break; - case ParseResult::ERROR_REGEX_TOO_LARGE: - output << "ERROR_REGEX_TOO_LARGE"; + case ParseResult::ERROR_INVALID_HEADER_VALUE: + output << "ERROR_INVALID_HEADER_VALUE"; break; - case ParseResult::ERROR_MULTIPLE_FILTERS_SPECIFIED: - output << "ERROR_MULTIPLE_FILTERS_SPECIFIED"; + case ParseResult::ERROR_HEADER_VALUE_NOT_SPECIFIED: + output << "ERROR_HEADER_VALUE_NOT_SPECIFIED"; break; - case ParseResult::ERROR_REGEX_SUBSTITUTION_WITHOUT_FILTER: - output << "ERROR_REGEX_SUBSTITUTION_WITHOUT_FILTER"; + case ParseResult::ERROR_HEADER_VALUE_PRESENT: + output << "ERROR_HEADER_VALUE_PRESENT"; break; - case ParseResult::ERROR_INVALID_REGEX_SUBSTITUTION: - output << "ERROR_INVALID_REGEX_SUBSTITUTION"; + case ParseResult::ERROR_APPEND_REQUEST_HEADER_UNSUPPORTED: + output << "ERROR_APPEND_REQUEST_HEADER_UNSUPPORTED"; break; - case ParseResult::ERROR_INVALID_ALLOW_ALL_REQUESTS_RESOURCE_TYPE: - output << "ERROR_INVALID_ALLOW_ALL_REQUESTS_RESOURCE_TYPE"; + } + return output; +} + +std::ostream& operator<<(std::ostream& output, LoadRulesetResult result) { + switch (result) { + case LoadRulesetResult::kSuccess: + output << "kSuccess"; + break; + case LoadRulesetResult::kErrorInvalidPath: + output << "kErrorInvalidPath"; + break; + case LoadRulesetResult::kErrorCannotReadFile: + output << "kErrorCannotReadFile"; + break; + case LoadRulesetResult::kErrorChecksumMismatch: + output << "kErrorChecksumMismatch"; + break; + case LoadRulesetResult::kErrorVersionMismatch: + output << "kErrorVersionMismatch"; + break; + case LoadRulesetResult::kErrorChecksumNotFound: + output << "kErrorChecksumNotFound"; break; } return output; @@ -285,7 +326,7 @@ bool AreAllIndexedStaticRulesetsValid( std::unique_ptr<RulesetMatcher> matcher; if (RulesetMatcher::CreateVerifiedMatcher(std::move(source), expected_checksum, &matcher) != - RulesetMatcher::kLoadSuccess) { + LoadRulesetResult::kSuccess) { return false; } } @@ -318,10 +359,9 @@ bool CreateVerifiedMatcher(const std::vector<TestRule>& rules, *expected_checksum = result.ruleset_checksum; // Create verified matcher. - RulesetMatcher::LoadRulesetResult load_result = - RulesetMatcher::CreateVerifiedMatcher(source, result.ruleset_checksum, - matcher); - return load_result == RulesetMatcher::kLoadSuccess; + LoadRulesetResult load_result = RulesetMatcher::CreateVerifiedMatcher( + source, result.ruleset_checksum, matcher); + return load_result == LoadRulesetResult::kSuccess; } RulesetSource CreateTemporarySource(RulesetID id, @@ -335,18 +375,25 @@ RulesetSource CreateTemporarySource(RulesetID id, dnr_api::ModifyHeaderInfo CreateModifyHeaderInfo( dnr_api::HeaderOperation operation, - std::string header) { + std::string header, + base::Optional<std::string> value) { dnr_api::ModifyHeaderInfo header_info; header_info.operation = operation; header_info.header = header; + if (value) + header_info.value = std::make_unique<std::string>(*value); + return header_info; } bool EqualsForTesting(const dnr_api::ModifyHeaderInfo& lhs, const dnr_api::ModifyHeaderInfo& rhs) { - return lhs.operation == rhs.operation && lhs.header == rhs.header; + bool are_values_equal = lhs.value && rhs.value ? *lhs.value == *rhs.value + : lhs.value == rhs.value; + return lhs.operation == rhs.operation && lhs.header == rhs.header && + are_values_equal; } RulesetManagerObserver::RulesetManagerObserver(RulesetManager* manager) diff --git a/chromium/extensions/browser/api/declarative_net_request/test_utils.h b/chromium/extensions/browser/api/declarative_net_request/test_utils.h index c67fbc39053..378ca7a29ee 100644 --- a/chromium/extensions/browser/api/declarative_net_request/test_utils.h +++ b/chromium/extensions/browser/api/declarative_net_request/test_utils.h @@ -57,6 +57,7 @@ std::ostream& operator<<(std::ostream& output, const RequestAction& action); std::ostream& operator<<(std::ostream& output, const ParseResult& result); std::ostream& operator<<(std::ostream& output, const base::Optional<RequestAction>& action); +std::ostream& operator<<(std::ostream& output, LoadRulesetResult result); // Returns true if the given extension's indexed static rulesets are all valid. // Should be called on a sequence where file IO is allowed. @@ -77,7 +78,8 @@ RulesetSource CreateTemporarySource(RulesetID id = kMinValidStaticRulesetID, api::declarative_net_request::ModifyHeaderInfo CreateModifyHeaderInfo( api::declarative_net_request::HeaderOperation operation, - std::string header); + std::string header, + base::Optional<std::string> value); bool EqualsForTesting( const api::declarative_net_request::ModifyHeaderInfo& lhs, diff --git a/chromium/extensions/browser/api/declarative_net_request/utils.cc b/chromium/extensions/browser/api/declarative_net_request/utils.cc index 792709e090f..a0428894c32 100644 --- a/chromium/extensions/browser/api/declarative_net_request/utils.cc +++ b/chromium/extensions/browser/api/declarative_net_request/utils.cc @@ -39,7 +39,7 @@ namespace dnr_api = api::declarative_net_request; // url_pattern_index.fbs. Whenever an extension with an indexed ruleset format // version different from the one currently used by Chrome is loaded, the // extension ruleset will be reindexed. -constexpr int kIndexedRulesetFormatVersion = 17; +constexpr int kIndexedRulesetFormatVersion = 18; // This static assert is meant to catch cases where // url_pattern_index::kUrlPatternIndexFormatVersion is incremented without @@ -49,13 +49,16 @@ static_assert(url_pattern_index::kUrlPatternIndexFormatVersion == 6, "also updated kIndexedRulesetFormatVersion above."); constexpr int kInvalidIndexedRulesetFormatVersion = -1; - int g_indexed_ruleset_format_version_for_testing = kInvalidIndexedRulesetFormatVersion; constexpr int kInvalidOverrideChecksumForTest = -1; int g_override_checksum_for_test = kInvalidOverrideChecksumForTest; +constexpr int kInvalidRuleLimit = -1; +int g_static_rule_limit_for_testing = kInvalidRuleLimit; +int g_regex_rule_limit_for_testing = kInvalidRuleLimit; + int GetIndexedRulesetFormatVersion() { return g_indexed_ruleset_format_version_for_testing == kInvalidIndexedRulesetFormatVersion @@ -287,5 +290,31 @@ std::vector<std::string> GetPublicRulesetIDs(const Extension& extension, return ids; } +int GetStaticRuleLimit() { + return g_static_rule_limit_for_testing == kInvalidRuleLimit + ? dnr_api::MAX_NUMBER_OF_RULES + : g_static_rule_limit_for_testing; +} + +int GetDynamicRuleLimit() { + return dnr_api::MAX_NUMBER_OF_DYNAMIC_RULES; +} + +int GetRegexRuleLimit() { + return g_regex_rule_limit_for_testing == kInvalidRuleLimit + ? dnr_api::MAX_NUMBER_OF_REGEX_RULES + : g_regex_rule_limit_for_testing; +} + +ScopedRuleLimitOverride CreateScopedStaticRuleLimitOverrideForTesting( + int limit) { + return base::AutoReset<int>(&g_static_rule_limit_for_testing, limit); +} + +ScopedRuleLimitOverride CreateScopedRegexRuleLimitOverrideForTesting( + int limit) { + return base::AutoReset<int>(&g_regex_rule_limit_for_testing, limit); +} + } // namespace declarative_net_request } // namespace extensions diff --git a/chromium/extensions/browser/api/declarative_net_request/utils.h b/chromium/extensions/browser/api/declarative_net_request/utils.h index f8f36f352f2..6e840a8d42f 100644 --- a/chromium/extensions/browser/api/declarative_net_request/utils.h +++ b/chromium/extensions/browser/api/declarative_net_request/utils.h @@ -99,6 +99,23 @@ std::string GetPublicRulesetID(const Extension& extension, std::vector<std::string> GetPublicRulesetIDs(const Extension& extension, const CompositeMatcher& matcher); +// Returns the per-extension static rule limit. +int GetStaticRuleLimit(); + +// Returns the per-extension dynamic rule limit. +int GetDynamicRuleLimit(); + +// Returns the per-extension regex rules limit. This is enforced separately for +// static and dynamic rulesets. +int GetRegexRuleLimit(); + +// Test helpers to override the various rule limits until the returned value is +// in scope. +using ScopedRuleLimitOverride = base::AutoReset<int>; +ScopedRuleLimitOverride CreateScopedStaticRuleLimitOverrideForTesting( + int limit); +ScopedRuleLimitOverride CreateScopedRegexRuleLimitOverrideForTesting(int limit); + // Helper to convert a flatbufffers::String to a string-like object with type T. template <typename T> T CreateString(const flatbuffers::String& str) { diff --git a/chromium/extensions/browser/api/device_permissions_prompt.h b/chromium/extensions/browser/api/device_permissions_prompt.h index 72d238d53d3..972bacd6197 100644 --- a/chromium/extensions/browser/api/device_permissions_prompt.h +++ b/chromium/extensions/browser/api/device_permissions_prompt.h @@ -11,7 +11,6 @@ #include <vector> #include "base/callback_forward.h" -#include "base/logging.h" #include "base/macros.h" #include "base/memory/ref_counted.h" #include "base/strings/string16.h" diff --git a/chromium/extensions/browser/api/display_source/display_source_apitestbase.cc b/chromium/extensions/browser/api/display_source/display_source_apitestbase.cc index b9a4e9e5b2f..a230c358c3f 100644 --- a/chromium/extensions/browser/api/display_source/display_source_apitestbase.cc +++ b/chromium/extensions/browser/api/display_source/display_source_apitestbase.cc @@ -10,7 +10,6 @@ #include "base/bind.h" #include "base/memory/ptr_util.h" -#include "base/task/post_task.h" #include "content/public/browser/browser_task_traits.h" #include "content/public/browser/browser_thread.h" #include "net/base/ip_address.h" @@ -339,8 +338,8 @@ void MockDisplaySourceConnectionDelegate::Connect( // And store udp port to udp_port_ string in order to be used // In a message exchange. Then make a base::PostTask // on UI thread and call OnSinkConnected() to proceed with the test - base::PostTask( - FROM_HERE, {BrowserThread::IO}, + content::GetIOThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&MockDisplaySourceConnectionDelegate::BindToUdpSocket, base::Unretained(this))); } @@ -432,8 +431,8 @@ EnqueueSinkMessage(std::string message) { AdaptMessagePattern(found_clientport_key, kClientPortKey, kUdpPortLength, udp_port_, message); - base::PostTask(FROM_HERE, {BrowserThread::UI}, - base::BindOnce(message_received_cb_, message)); + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(message_received_cb_, message)); } void MockDisplaySourceConnectionDelegate:: @@ -488,8 +487,8 @@ void MockDisplaySourceConnectionDelegate::BindToUdpSocket() { udp_port_ = std::to_string(port); // When we got an udp socket established and udp port is known // Change sink's status to connected and proceed with the test. - base::PostTask( - FROM_HERE, {BrowserThread::UI}, + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&MockDisplaySourceConnectionDelegate::OnSinkConnected, base::Unretained(this))); break; @@ -526,8 +525,8 @@ void MockDisplaySourceConnectionDelegate::OnMediaPacketReceived( // We received at least one media packet. // Test is completed. socket_->Close(); - base::PostTask( - FROM_HERE, {BrowserThread::UI}, + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&MockDisplaySourceConnectionDelegate::Disconnect, base::Unretained(this), StringCallback())); return; diff --git a/chromium/extensions/browser/api/display_source/wifi_display/wifi_display_media_service_impl.cc b/chromium/extensions/browser/api/display_source/wifi_display/wifi_display_media_service_impl.cc index 49fbd813d15..6da57bb3cb1 100644 --- a/chromium/extensions/browser/api/display_source/wifi_display/wifi_display_media_service_impl.cc +++ b/chromium/extensions/browser/api/display_source/wifi_display/wifi_display_media_service_impl.cc @@ -10,7 +10,6 @@ #include "base/big_endian.h" #include "base/bind.h" #include "base/memory/ptr_util.h" -#include "base/task/post_task.h" #include "content/public/browser/browser_task_traits.h" #include "content/public/browser/browser_thread.h" #include "mojo/public/cpp/bindings/strong_binding.h" @@ -56,8 +55,8 @@ void WiFiDisplayMediaServiceImpl::Create( void WiFiDisplayMediaServiceImpl::BindToReceiver( mojo::PendingReceiver<mojom::WiFiDisplayMediaService> receiver, content::RenderFrameHost* render_frame_host) { - base::PostTask( - FROM_HERE, {BrowserThread::IO}, + content::GetIOThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(WiFiDisplayMediaServiceImpl::Create, std::move(receiver))); } diff --git a/chromium/extensions/browser/api/document_scan/BUILD.gn b/chromium/extensions/browser/api/document_scan/BUILD.gn index 7ac8b406a51..141506a87b5 100644 --- a/chromium/extensions/browser/api/document_scan/BUILD.gn +++ b/chromium/extensions/browser/api/document_scan/BUILD.gn @@ -13,15 +13,22 @@ source_set("document_scan") { "document_scan_api.h", "document_scan_interface.cc", "document_scan_interface.h", - "document_scan_interface_chromeos.cc", - "document_scan_interface_chromeos.h", ] - if (!is_chromeos) { + if (is_chromeos) { + sources += [ + "document_scan_interface_chromeos.cc", + "document_scan_interface_chromeos.h", + ] + } else { sources += [ "document_scan_interface_nonchromeos.cc" ] } deps = [ "//extensions/common/api" ] + if (is_chromeos) { + deps += [ "//chromeos/dbus:lorgnette_proto" ] + } + public_deps = [ "//extensions/browser:browser_sources" ] } diff --git a/chromium/extensions/browser/api/document_scan/document_scan_api.h b/chromium/extensions/browser/api/document_scan/document_scan_api.h index ba490af6616..3044227c624 100644 --- a/chromium/extensions/browser/api/document_scan/document_scan_api.h +++ b/chromium/extensions/browser/api/document_scan/document_scan_api.h @@ -9,7 +9,6 @@ #include <string> #include <vector> -#include "base/macros.h" #include "extensions/browser/api/document_scan/document_scan_interface.h" #include "extensions/browser/extension_function.h" #include "extensions/common/api/document_scan.h" @@ -21,6 +20,8 @@ class DocumentScanScanFunction : public ExtensionFunction { public: DECLARE_EXTENSION_FUNCTION("documentScan.scan", DOCUMENT_SCAN_SCAN) DocumentScanScanFunction(); + DocumentScanScanFunction(const DocumentScanScanFunction&) = delete; + DocumentScanScanFunction& operator=(const DocumentScanScanFunction&) = delete; protected: ~DocumentScanScanFunction() override; @@ -41,8 +42,6 @@ class DocumentScanScanFunction : public ExtensionFunction { std::unique_ptr<document_scan::Scan::Params> params_; std::unique_ptr<DocumentScanInterface> document_scan_interface_; - - DISALLOW_COPY_AND_ASSIGN(DocumentScanScanFunction); }; } // namespace api diff --git a/chromium/extensions/browser/api/document_scan/document_scan_interface_chromeos.cc b/chromium/extensions/browser/api/document_scan/document_scan_interface_chromeos.cc index 2517319a22b..11a5169008e 100644 --- a/chromium/extensions/browser/api/document_scan/document_scan_interface_chromeos.cc +++ b/chromium/extensions/browser/api/document_scan/document_scan_interface_chromeos.cc @@ -9,6 +9,7 @@ #include "base/base64.h" #include "base/bind.h" +#include "base/logging.h" #include "chromeos/dbus/dbus_thread_manager.h" #include "chromeos/dbus/lorgnette_manager_client.h" #include "third_party/cros_system_api/dbus/service_constants.h" @@ -42,25 +43,15 @@ void DocumentScanInterfaceChromeos::ListScanners( void DocumentScanInterfaceChromeos::OnScannerListReceived( ListScannersResultsCallback callback, - base::Optional<chromeos::LorgnetteManagerClient::ScannerTable> scanners) { + base::Optional<lorgnette::ListScannersResponse> response) { std::vector<ScannerDescription> scanner_descriptions; - if (scanners.has_value()) { - for (const auto& scanner : scanners.value()) { + if (response) { + for (const auto& scanner : response->scanners()) { ScannerDescription description; - description.name = scanner.first; - const auto& entry = scanner.second; - auto info_it = entry.find(lorgnette::kScannerPropertyManufacturer); - if (info_it != entry.end()) { - description.manufacturer = info_it->second; - } - info_it = entry.find(lorgnette::kScannerPropertyModel); - if (info_it != entry.end()) { - description.model = info_it->second; - } - info_it = entry.find(lorgnette::kScannerPropertyType); - if (info_it != entry.end()) { - description.scanner_type = info_it->second; - } + description.name = scanner.name(); + description.manufacturer = scanner.manufacturer(); + description.model = scanner.model(); + description.scanner_type = scanner.type(); description.image_mime_type = kScannerImageMimeTypePng; scanner_descriptions.push_back(description); } diff --git a/chromium/extensions/browser/api/document_scan/document_scan_interface_chromeos.h b/chromium/extensions/browser/api/document_scan/document_scan_interface_chromeos.h index b25eaa39fa5..ebbc0031dce 100644 --- a/chromium/extensions/browser/api/document_scan/document_scan_interface_chromeos.h +++ b/chromium/extensions/browser/api/document_scan/document_scan_interface_chromeos.h @@ -7,9 +7,8 @@ #include <string> -#include "base/macros.h" #include "base/optional.h" -#include "chromeos/dbus/lorgnette_manager_client.h" +#include "chromeos/dbus/lorgnette/lorgnette_service.pb.h" #include "extensions/browser/api/document_scan/document_scan_interface.h" namespace extensions { @@ -18,6 +17,9 @@ namespace api { class DocumentScanInterfaceChromeos : public DocumentScanInterface { public: DocumentScanInterfaceChromeos(); + DocumentScanInterfaceChromeos(const DocumentScanInterfaceChromeos&) = delete; + DocumentScanInterfaceChromeos& operator=( + const DocumentScanInterfaceChromeos&) = delete; ~DocumentScanInterfaceChromeos() override; // DocumentScanInterface: @@ -30,11 +32,9 @@ class DocumentScanInterfaceChromeos : public DocumentScanInterface { private: void OnScannerListReceived( ListScannersResultsCallback callback, - base::Optional<chromeos::LorgnetteManagerClient::ScannerTable> scanners); + base::Optional<lorgnette::ListScannersResponse> response); void OnScanCompleted(ScanResultsCallback callback, base::Optional<std::string> image_data); - - DISALLOW_COPY_AND_ASSIGN(DocumentScanInterfaceChromeos); }; } // namespace api diff --git a/chromium/extensions/browser/api/document_scan/document_scan_interface_chromeos_unittest.cc b/chromium/extensions/browser/api/document_scan/document_scan_interface_chromeos_unittest.cc index 0d99d9a1f2f..f969fb6434a 100644 --- a/chromium/extensions/browser/api/document_scan/document_scan_interface_chromeos_unittest.cc +++ b/chromium/extensions/browser/api/document_scan/document_scan_interface_chromeos_unittest.cc @@ -46,11 +46,14 @@ TEST_F(DocumentScanInterfaceChromeosTest, ListScanners) { constexpr const char* kScannerManufacturer = "Jacques-Louis David"; constexpr const char* kScannerModel = "Le Havre"; constexpr const char* kScannerType = "Impressionism"; - GetLorgnetteManagerClient()->AddScannerTableEntry( - kScannerName, - {{lorgnette::kScannerPropertyManufacturer, kScannerManufacturer}, - {lorgnette::kScannerPropertyModel, kScannerModel}, - {lorgnette::kScannerPropertyType, kScannerType}}); + lorgnette::ScannerInfo scanner; + scanner.set_name(kScannerName); + scanner.set_manufacturer(kScannerManufacturer); + scanner.set_model(kScannerModel); + scanner.set_type(kScannerType); + lorgnette::ListScannersResponse response; + *response.add_scanners() = std::move(scanner); + GetLorgnetteManagerClient()->SetListScannersResponse(response); base::RunLoop run_loop; scan_interface_.ListScanners(base::BindOnce( diff --git a/chromium/extensions/browser/api/document_scan/document_scan_interface_nonchromeos.cc b/chromium/extensions/browser/api/document_scan/document_scan_interface_nonchromeos.cc index e5a0ca0b18f..16b476599b5 100644 --- a/chromium/extensions/browser/api/document_scan/document_scan_interface_nonchromeos.cc +++ b/chromium/extensions/browser/api/document_scan/document_scan_interface_nonchromeos.cc @@ -4,7 +4,6 @@ #include <utility> -#include "base/macros.h" #include "extensions/browser/api/document_scan/document_scan_interface.h" namespace { @@ -18,8 +17,12 @@ namespace api { class DocumentScanInterfaceImpl : public DocumentScanInterface { public: - DocumentScanInterfaceImpl() {} - ~DocumentScanInterfaceImpl() override {} + DocumentScanInterfaceImpl() = default; + ~DocumentScanInterfaceImpl() override = default; + + DocumentScanInterfaceImpl(const DocumentScanInterfaceImpl&) = delete; + DocumentScanInterfaceImpl& operator=(const DocumentScanInterfaceImpl&) = + delete; void ListScanners(ListScannersResultsCallback callback) override { std::move(callback).Run(std::vector<ScannerDescription>(), ""); @@ -30,9 +33,6 @@ class DocumentScanInterfaceImpl : public DocumentScanInterface { ScanResultsCallback callback) override { std::move(callback).Run("", "", kScanFunctionNotImplementedError); } - - private: - DISALLOW_COPY_AND_ASSIGN(DocumentScanInterfaceImpl); }; // static diff --git a/chromium/extensions/browser/api/extensions_api_client.cc b/chromium/extensions/browser/api/extensions_api_client.cc index abb2202284b..18e27f0ee78 100644 --- a/chromium/extensions/browser/api/extensions_api_client.cc +++ b/chromium/extensions/browser/api/extensions_api_client.cc @@ -5,12 +5,12 @@ #include "extensions/browser/api/extensions_api_client.h" #include "extensions/browser/api/device_permissions_prompt.h" -#include "extensions/browser/api/management/supervised_user_service_delegate.h" #include "extensions/browser/api/system_display/display_info_provider.h" #include "extensions/browser/api/virtual_keyboard_private/virtual_keyboard_delegate.h" #include "extensions/browser/guest_view/extensions_guest_view_manager_delegate.h" #include "extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest_delegate.h" #include "extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h" +#include "extensions/browser/supervised_user_extensions_delegate.h" namespace extensions { class AppViewGuestDelegate; @@ -120,8 +120,8 @@ ManagementAPIDelegate* ExtensionsAPIClient::CreateManagementAPIDelegate() return nullptr; } -std::unique_ptr<SupervisedUserServiceDelegate> -ExtensionsAPIClient::CreateSupervisedUserServiceDelegate() const { +std::unique_ptr<SupervisedUserExtensionsDelegate> +ExtensionsAPIClient::CreateSupervisedUserExtensionsDelegate() const { return nullptr; } diff --git a/chromium/extensions/browser/api/extensions_api_client.h b/chromium/extensions/browser/api/extensions_api_client.h index 81a7cad5880..67b8c3fda8f 100644 --- a/chromium/extensions/browser/api/extensions_api_client.h +++ b/chromium/extensions/browser/api/extensions_api_client.h @@ -55,7 +55,7 @@ class NetworkingCastPrivateDelegate; class NonNativeFileSystemDelegate; class RulesCacheDelegate; class SettingsObserver; -class SupervisedUserServiceDelegate; +class SupervisedUserExtensionsDelegate; class ValueStoreCache; class ValueStoreFactory; class VirtualKeyboardDelegate; @@ -170,8 +170,8 @@ class ExtensionsAPIClient { // Creates a delegate for calling into the SupervisedUserService from the // Management API. - virtual std::unique_ptr<SupervisedUserServiceDelegate> - CreateSupervisedUserServiceDelegate() const; + virtual std::unique_ptr<SupervisedUserExtensionsDelegate> + CreateSupervisedUserExtensionsDelegate() const; // Creates and returns the DisplayInfoProvider used by the // chrome.system.display extension API. diff --git a/chromium/extensions/browser/api/feedback_private/feedback_service.cc b/chromium/extensions/browser/api/feedback_private/feedback_service.cc index 9ef9363edc7..260b3ec7996 100644 --- a/chromium/extensions/browser/api/feedback_private/feedback_service.cc +++ b/chromium/extensions/browser/api/feedback_private/feedback_service.cc @@ -20,7 +20,7 @@ #if defined(OS_CHROMEOS) #include "ash/public/cpp/assistant/controller/assistant_controller.h" -#include "chromeos/services/assistant/public/mojom/assistant.mojom.h" +#include "chromeos/services/assistant/public/cpp/assistant_service.h" #include "extensions/browser/api/feedback_private/log_source_access_manager.h" #include "mojo/public/cpp/bindings/remote.h" #endif // defined(OS_CHROMEOS) diff --git a/chromium/extensions/browser/api/file_system/BUILD.gn b/chromium/extensions/browser/api/file_system/BUILD.gn index 262b58d976a..516aba105f1 100644 --- a/chromium/extensions/browser/api/file_system/BUILD.gn +++ b/chromium/extensions/browser/api/file_system/BUILD.gn @@ -19,6 +19,7 @@ source_set("file_system") { deps = [ "//base", + "//base/util/values:values_util", "//content/public/browser", "//extensions/browser/api/file_handlers", "//extensions/common", diff --git a/chromium/extensions/browser/api/file_system/file_system_api.cc b/chromium/extensions/browser/api/file_system/file_system_api.cc index 706354d8ecb..5c899c4eed3 100644 --- a/chromium/extensions/browser/api/file_system/file_system_api.cc +++ b/chromium/extensions/browser/api/file_system/file_system_api.cc @@ -22,9 +22,8 @@ #include "base/strings/string_util.h" #include "base/strings/stringprintf.h" #include "base/strings/utf_string_conversions.h" -#include "base/task/post_task.h" #include "base/task/thread_pool.h" -#include "base/value_conversions.h" +#include "base/util/values/values_util.h" #include "base/values.h" #include "build/build_config.h" #include "content/public/browser/browser_context.h" @@ -188,8 +187,8 @@ void PassFileInfoToUIThread(const FileInfoOptCallback& callback, DCHECK_CURRENTLY_ON(content::BrowserThread::IO); std::unique_ptr<base::File::Info> file_info( result == base::File::FILE_OK ? new base::File::Info(info) : NULL); - base::PostTask(FROM_HERE, {content::BrowserThread::UI}, - base::BindOnce(callback, std::move(file_info))); + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(callback, std::move(file_info))); } // Gets a WebContents instance handle for a platform app hosted in @@ -227,7 +226,7 @@ void SetLastChooseEntryDirectory(ExtensionPrefs* prefs, const base::FilePath& path) { prefs->UpdateExtensionPref( extension_id, kLastChooseEntryDirectory, - base::Value::ToUniquePtrValue(base::CreateFilePathValue(path))); + base::Value::ToUniquePtrValue(::util::FilePathToValue(path))); } } // namespace file_system_api @@ -395,8 +394,8 @@ void FileSystemChooseEntryFunction::ShowPicker( else if (g_paths_to_be_picked_for_test) test_paths = *g_paths_to_be_picked_for_test; - base::PostTask( - FROM_HERE, {content::BrowserThread::UI}, + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, test_paths.size() > 0 ? base::BindOnce(&FileSystemChooseEntryFunction::FilesSelected, this, test_paths) @@ -498,9 +497,16 @@ void FileSystemChooseEntryFunction::FilesSelected( } else { last_choose_directory = paths[0].DirName(); } - file_system_api::SetLastChooseEntryDirectory( - ExtensionPrefs::Get(browser_context()), extension()->id(), - last_choose_directory); + + if (extension_->is_extension()) { + ExtensionsBrowserClient::Get()->SetLastSaveFilePath(browser_context(), + last_choose_directory); + } else { + file_system_api::SetLastChooseEntryDirectory( + ExtensionPrefs::Get(browser_context()), extension()->id(), + last_choose_directory); + } + if (is_directory_) { // Get the WebContents for the app window to be the parent window of the // confirmation dialog if necessary. @@ -542,8 +548,8 @@ void FileSystemChooseEntryFunction::ConfirmDirectoryAccessAsync( const base::FilePath check_path = non_native_path ? paths[0] : base::MakeAbsoluteFilePath(paths[0]); if (check_path.empty()) { - base::PostTask( - FROM_HERE, {content::BrowserThread::UI}, + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&FileSystemChooseEntryFunction::FileSelectionCanceled, this)); return; @@ -559,23 +565,23 @@ void FileSystemChooseEntryFunction::ConfirmDirectoryAccessAsync( if (g_skip_directory_confirmation_for_test) { if (g_allow_directory_access_for_test) break; - base::PostTask( - FROM_HERE, {content::BrowserThread::UI}, + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&FileSystemChooseEntryFunction::FileSelectionCanceled, this)); return; } - base::PostTask( - FROM_HERE, {content::BrowserThread::UI}, + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce( &FileSystemChooseEntryFunction::ConfirmSensitiveDirectoryAccess, this, paths, web_contents)); return; } - base::PostTask( - FROM_HERE, {content::BrowserThread::UI}, + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&FileSystemChooseEntryFunction::OnDirectoryAccessConfirmed, this, paths)); } @@ -619,10 +625,10 @@ void FileSystemChooseEntryFunction::BuildFileTypeInfo( ui::SelectFileDialog::FileTypeInfo* file_type_info, const base::FilePath::StringType& suggested_extension, const AcceptOptions* accepts, - const bool* acceptsAllTypes) { + const bool* accepts_all_types) { file_type_info->include_all_files = true; - if (acceptsAllTypes) - file_type_info->include_all_files = *acceptsAllTypes; + if (accepts_all_types) + file_type_info->include_all_files = *accepts_all_types; bool need_suggestion = !file_type_info->include_all_files && !suggested_extension.empty(); @@ -745,8 +751,14 @@ ExtensionFunction::ResponseAction FileSystemChooseEntryFunction::Run() { file_type_info.allowed_paths = ui::SelectFileDialog::FileTypeInfo::ANY_PATH; - base::FilePath previous_path = file_system_api::GetLastChooseEntryDirectory( - ExtensionPrefs::Get(browser_context()), extension()->id()); + base::FilePath previous_path; + if (extension_->is_extension()) { + previous_path = + ExtensionsBrowserClient::Get()->GetSaveFilePath(browser_context()); + } else { + previous_path = file_system_api::GetLastChooseEntryDirectory( + ExtensionPrefs::Get(browser_context()), extension()->id()); + } if (previous_path.empty()) { SetInitialPathAndShowPicker(previous_path, suggested_name, file_type_info, @@ -826,8 +838,8 @@ ExtensionFunction::ResponseAction FileSystemRetainEntryFunction::Run() { // It is safe to use base::Unretained() for operation_runner(), since it // is owned by |context| which will delete it on the IO thread. - base::PostTask( - FROM_HERE, {content::BrowserThread::IO}, + content::GetIOThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce( base::IgnoreResult( &storage::FileSystemOperationRunner::GetMetadata), diff --git a/chromium/extensions/browser/api/file_system/file_system_api.h b/chromium/extensions/browser/api/file_system/file_system_api.h index 7df036af4c6..2a95c4d89fd 100644 --- a/chromium/extensions/browser/api/file_system/file_system_api.h +++ b/chromium/extensions/browser/api/file_system/file_system_api.h @@ -140,7 +140,7 @@ class FileSystemChooseEntryFunction : public FileSystemEntryFunction { ui::SelectFileDialog::FileTypeInfo* file_type_info, const base::FilePath::StringType& suggested_extension, const AcceptOptions* accepts, - const bool* acceptsAllTypes); + const bool* accepts_all_types); static void BuildSuggestion(const std::string* opt_name, base::FilePath* suggested_name, base::FilePath::StringType* suggested_extension); diff --git a/chromium/extensions/browser/api/management/BUILD.gn b/chromium/extensions/browser/api/management/BUILD.gn index 7692a4e1b99..7670df786f0 100644 --- a/chromium/extensions/browser/api/management/BUILD.gn +++ b/chromium/extensions/browser/api/management/BUILD.gn @@ -14,7 +14,6 @@ source_set("management") { "management_api_constants.cc", "management_api_constants.h", "management_api_delegate.h", - "supervised_user_service_delegate.h", ] deps = [ "//extensions/common/api" ] diff --git a/chromium/extensions/browser/api/management/management_api.cc b/chromium/extensions/browser/api/management/management_api.cc index f19d3e9253f..c44d29df281 100644 --- a/chromium/extensions/browser/api/management/management_api.cc +++ b/chromium/extensions/browser/api/management/management_api.cc @@ -48,11 +48,6 @@ #include "url/gurl.h" #include "url/url_constants.h" -#if defined(OS_CHROMEOS) -#include "components/web_modal/web_contents_modal_dialog_manager.h" -#include "content/public/browser/web_contents.h" -#endif - using content::BrowserThread; namespace keys = extension_management_api_constants; @@ -436,53 +431,42 @@ ExtensionFunction::ResponseAction ManagementSetEnabledFunction::Run() { bool should_enable = params->enabled; - const SupervisedUserServiceDelegate* supervised_user_service_delegate = - ManagementAPI::GetFactoryInstance() - ->Get(browser_context()) - ->GetSupervisedUserServiceDelegate(); - - const bool is_supervised_child_who_may_install_extensions = - supervised_user_service_delegate - ? supervised_user_service_delegate - ->IsSupervisedChildWhoMayInstallExtensions(browser_context()) - : false; - const ManagementPolicy* policy = ExtensionSystem::Get(browser_context())->management_policy(); - if (!policy->ExtensionMayModifySettings(extension(), target_extension, nullptr)) { return RespondNow(Error(keys::kUserCantModifyError, extension_id_)); } - disable_reason::DisableReason reason = disable_reason::DISABLE_NONE; - bool disallow_enable = - should_enable && - policy->MustRemainDisabled(target_extension, &reason, nullptr); - - // Figure out if we should prompt for parental approval. - bool prompt_parent_for_approval = - disallow_enable && is_supervised_child_who_may_install_extensions && - reason == disable_reason::DISABLE_CUSTODIAN_APPROVAL_REQUIRED; - - // If the extension can't be enabled, only continue if we plan to prompt for - // parental approval. For any child or other type of managed user, if - // extension installation has been blocked, we stop the enabling of the - // extension here. - if (disallow_enable && !prompt_parent_for_approval) { - LOG(ERROR) << "ManagementSetEnabledFunction::Run: extension may not be " - "enabled, and we're not prompting for parent approval"; + SupervisedUserExtensionsDelegate* supervised_user_extensions_delegate = + ManagementAPI::GetFactoryInstance() + ->Get(browser_context()) + ->GetSupervisedUserExtensionsDelegate(); + if (supervised_user_extensions_delegate && + supervised_user_extensions_delegate->IsChild(browser_context()) && + // Don't prompt the user if the extension has unsupported requirements. + // TODO(crbug/1071978): If OnRequirementsChecked() passes, the extension + // will enable, bypassing parent approval. + !HasUnsupportedRequirements(extension_id_) && + // Only ask for parent approval if the extension still requires approval. + !supervised_user_extensions_delegate->IsExtensionAllowedByParent( + *target_extension, browser_context())) { + // Either ask for parent permission or notify the child that their parent + // has disabled this action. + auto parent_permission_callback = base::BindOnce( + &ManagementSetEnabledFunction::OnParentPermissionDialogDone, this); + auto error_callback = base::BindOnce( + &ManagementSetEnabledFunction::OnBlockedByParentDialogDone, this); + AddRef(); // Matched in OnParentPermissionDialogDone() or + // OnBlockedByParentDialogDone(). + supervised_user_extensions_delegate->PromptForParentPermissionOrShowError( + *target_extension, browser_context(), GetSenderWebContents(), + std::move(parent_permission_callback), std::move(error_callback)); + return RespondLater(); + } -#if defined(OS_CHROMEOS) - // On ChromeOS, if this is a child, show the dialog indicating that enabling - // extensions has been blocked by a parent. - if (supervised_user_service_delegate && - supervised_user_service_delegate->IsChild(browser_context())) { - AddRef(); // Matched in OnBlockedByParentDialogDone(). - ShowBlockedByParentDialog(target_extension); - return RespondLater(); - } -#endif + if (should_enable && + policy->MustRemainDisabled(target_extension, nullptr, nullptr)) { return RespondNow(Error(keys::kUserCantModifyError, extension_id_)); } @@ -492,8 +476,7 @@ ExtensionFunction::ResponseAction ManagementSetEnabledFunction::Run() { if (!currently_enabled && should_enable) { ExtensionPrefs* prefs = ExtensionPrefs::Get(browser_context()); - if (!prompt_parent_for_approval && - prefs->DidExtensionEscalatePermissions(extension_id_)) { + if (prefs->DidExtensionEscalatePermissions(extension_id_)) { if (!user_gesture()) return RespondNow(Error(keys::kGestureNeededForEscalationError)); @@ -503,8 +486,7 @@ ExtensionFunction::ResponseAction ManagementSetEnabledFunction::Run() { base::Bind(&ManagementSetEnabledFunction::OnInstallPromptDone, this)); return RespondLater(); } - if (prefs->GetDisableReasons(extension_id_) & - disable_reason::DISABLE_UNSUPPORTED_REQUIREMENT) { + if (HasUnsupportedRequirements(extension_id_)) { // Recheck the requirements. requirements_checker_ = std::make_unique<RequirementsChecker>(target_extension); @@ -513,18 +495,6 @@ ExtensionFunction::ResponseAction ManagementSetEnabledFunction::Run() { this)); // This bind creates a reference. return RespondLater(); } - // Handle parental approval for child accounts that have the ability to - // install extensions. - if (prompt_parent_for_approval && - // Don't re-prompt the parent for extensions that have already been - // approved for a child. - !supervised_user_service_delegate->IsExtensionAllowedByParent( - *target_extension, browser_context())) { - LOG(ERROR) << "ManagementSetEnabledFunction::Run: prompting for parent " - "approval"; - return RequestParentPermission(target_extension); - } - delegate->EnableExtension(browser_context(), extension_id_); } else if (currently_enabled && !params->enabled) { delegate->DisableExtension( @@ -551,6 +521,13 @@ void ManagementSetEnabledFunction::OnInstallPromptDone(bool did_accept) { Release(); // Balanced in Run(). } +bool ManagementSetEnabledFunction::HasUnsupportedRequirements( + const std::string& extension_id) { + ExtensionPrefs* prefs = ExtensionPrefs::Get(browser_context()); + return prefs->GetDisableReasons(extension_id) & + disable_reason::DISABLE_UNSUPPORTED_REQUIREMENT; +} + void ManagementSetEnabledFunction::OnRequirementsChecked( const PreloadCheck::Errors& errors) { if (errors.empty()) { @@ -564,30 +541,11 @@ void ManagementSetEnabledFunction::OnRequirementsChecked( } } -ExtensionFunction::ResponseAction -ManagementSetEnabledFunction::RequestParentPermission( - const Extension* extension) { - content::WebContents* web_contents = GetSenderWebContents(); - if (!web_contents) - return RespondNow(Error(keys::kWebContentsDisappearedError)); - - // Show parental approval prompt. - auto callback = base::BindOnce( - &ManagementSetEnabledFunction::OnParentPermissionDone, this); - SupervisedUserServiceDelegate* supervised_user_service_delegate = - ManagementAPI::GetFactoryInstance() - ->Get(browser_context()) - ->GetSupervisedUserServiceDelegate(); - DCHECK(supervised_user_service_delegate); - supervised_user_service_delegate->ShowParentPermissionDialogForExtension( - *extension, browser_context(), web_contents, std::move(callback)); - return RespondLater(); -} - -void ManagementSetEnabledFunction::OnParentPermissionDone( - SupervisedUserServiceDelegate::ParentPermissionDialogResult result) { +void ManagementSetEnabledFunction::OnParentPermissionDialogDone( + SupervisedUserExtensionsDelegate::ParentPermissionDialogResult result) { +#if defined(OS_CHROMEOS) switch (result) { - case SupervisedUserServiceDelegate::ParentPermissionDialogResult:: + case SupervisedUserExtensionsDelegate::ParentPermissionDialogResult:: kParentPermissionReceived: { const ManagementAPIDelegate* delegate = ManagementAPI::GetFactoryInstance() @@ -598,49 +556,21 @@ void ManagementSetEnabledFunction::OnParentPermissionDone( break; } - case SupervisedUserServiceDelegate::ParentPermissionDialogResult:: + case SupervisedUserExtensionsDelegate::ParentPermissionDialogResult:: kParentPermissionCanceled: { Respond(Error(keys::kUserDidNotReEnableError)); break; } - case SupervisedUserServiceDelegate::ParentPermissionDialogResult:: + case SupervisedUserExtensionsDelegate::ParentPermissionDialogResult:: kParentPermissionFailed: { Respond(Error(keys::kParentPermissionFailedError)); break; } } -} - -void ManagementSetEnabledFunction::ShowBlockedByParentDialog( - const Extension* extension) { -#if defined(OS_CHROMEOS) - DCHECK(extension); - SupervisedUserServiceDelegate* supervised_user_service_delegate = - ManagementAPI::GetFactoryInstance() - ->Get(browser_context()) - ->GetSupervisedUserServiceDelegate(); - - supervised_user_service_delegate - ->RecordExtensionEnableBlockedByParentDialogUmaMetric(); - - content::WebContents* contents = GetSenderWebContents(); - web_modal::WebContentsModalDialogManager* manager = - web_modal::WebContentsModalDialogManager::FromWebContents(contents); - if (!contents || !contents->GetTopLevelNativeWindow() || !manager) { - // If the contents are null, or there is no top level native window to - // anchor the dialog on, or no dialog manager, skip showing the dialog and - // return the error immediately. - OnBlockedByParentDialogDone(); - return; - } - supervised_user_service_delegate - ->ShowExtensionEnableBlockedByParentDialogForExtension( - extension, contents, - base::BindOnce( - &ManagementSetEnabledFunction::OnBlockedByParentDialogDone, - this)); -#endif + // Matches the AddRef in Run(). + Release(); +#endif // defined(OS_CHROMEOS) } void ManagementSetEnabledFunction::OnBlockedByParentDialogDone() { @@ -648,7 +578,7 @@ void ManagementSetEnabledFunction::OnBlockedByParentDialogDone() { Respond(Error(keys::kUserCantModifyError, extension_id_)); // Matches the AddRef in Run(). Release(); -#endif +#endif // defined(OS_CHROMEOS) } ManagementUninstallFunctionBase::ManagementUninstallFunctionBase() = default; @@ -1180,8 +1110,9 @@ void ManagementEventRouter::BroadcastEvent( ManagementAPI::ManagementAPI(content::BrowserContext* context) : browser_context_(context), delegate_(ExtensionsAPIClient::Get()->CreateManagementAPIDelegate()), - supervised_user_service_delegate_( - ExtensionsAPIClient::Get()->CreateSupervisedUserServiceDelegate()) { + supervised_user_extensions_delegate_( + ExtensionsAPIClient::Get() + ->CreateSupervisedUserExtensionsDelegate()) { EventRouter* event_router = EventRouter::Get(browser_context_); event_router->RegisterObserver(this, management::OnInstalled::kEventName); event_router->RegisterObserver(this, management::OnUninstalled::kEventName); diff --git a/chromium/extensions/browser/api/management/management_api.h b/chromium/extensions/browser/api/management/management_api.h index 1e195465151..89b4205333a 100644 --- a/chromium/extensions/browser/api/management/management_api.h +++ b/chromium/extensions/browser/api/management/management_api.h @@ -14,13 +14,13 @@ #include "base/strings/string16.h" #include "components/keyed_service/core/keyed_service.h" #include "extensions/browser/api/management/management_api_delegate.h" -#include "extensions/browser/api/management/supervised_user_service_delegate.h" #include "extensions/browser/browser_context_keyed_api_factory.h" #include "extensions/browser/event_router.h" #include "extensions/browser/extension_event_histogram_value.h" #include "extensions/browser/extension_function.h" #include "extensions/browser/extension_registry_observer.h" #include "extensions/browser/preload_check.h" +#include "extensions/browser/supervised_user_extensions_delegate.h" #include "services/data_decoder/public/cpp/data_decoder.h" namespace extensions { @@ -115,24 +115,18 @@ class ManagementSetEnabledFunction : public ExtensionFunction { private: void OnInstallPromptDone(bool did_accept); - void OnRequirementsChecked(const PreloadCheck::Errors& errors); - - ExtensionFunction::ResponseAction RequestParentPermission( - const Extension* extension); + bool HasUnsupportedRequirements(const std::string& extension_id); - void OnParentPermissionDone( - SupervisedUserServiceDelegate::ParentPermissionDialogResult result); + void OnRequirementsChecked(const PreloadCheck::Errors& errors); - // Shows the dialog that tells the user that the parent has blocked the - // installation of extensions, apps, etc. - void ShowBlockedByParentDialog(const Extension* extension); + // Called when the user dismisses the Parent Permission Dialog. + void OnParentPermissionDialogDone( + SupervisedUserExtensionsDelegate::ParentPermissionDialogResult result); - // Called when the dialog shown by ShowBlockedByParentDialog() is dismissed. + // Called when the user dismisses the Extension Install Blocked By Parent + // Dialog. void OnBlockedByParentDialogDone(); - std::unique_ptr<SupervisedUserServiceDelegate::ParentPermissionDialogResult> - parental_permission_dialog_; - std::string extension_id_; std::unique_ptr<InstallPromptDelegate> install_prompt_; @@ -334,16 +328,17 @@ class ManagementAPI : public BrowserContextKeyedAPI, // Returns the SupervisedUserService delegate, which might be null depending // on the extensions embedder. - SupervisedUserServiceDelegate* GetSupervisedUserServiceDelegate() const { - return supervised_user_service_delegate_.get(); + SupervisedUserExtensionsDelegate* GetSupervisedUserExtensionsDelegate() + const { + return supervised_user_extensions_delegate_.get(); } void set_delegate_for_test(std::unique_ptr<ManagementAPIDelegate> delegate) { delegate_ = std::move(delegate); } - void set_supervised_user_service_delegate_for_test( - std::unique_ptr<SupervisedUserServiceDelegate> delegate) { - supervised_user_service_delegate_ = std::move(delegate); + void set_supervised_user_extensions_delegate_for_test( + std::unique_ptr<SupervisedUserExtensionsDelegate> delegate) { + supervised_user_extensions_delegate_ = std::move(delegate); } private: @@ -360,8 +355,8 @@ class ManagementAPI : public BrowserContextKeyedAPI, std::unique_ptr<ManagementEventRouter> management_event_router_; std::unique_ptr<ManagementAPIDelegate> delegate_; - std::unique_ptr<SupervisedUserServiceDelegate> - supervised_user_service_delegate_; + std::unique_ptr<SupervisedUserExtensionsDelegate> + supervised_user_extensions_delegate_; DISALLOW_COPY_AND_ASSIGN(ManagementAPI); }; diff --git a/chromium/extensions/browser/api/management/management_api_constants.cc b/chromium/extensions/browser/api/management/management_api_constants.cc index 5069a509e44..686075749ac 100644 --- a/chromium/extensions/browser/api/management/management_api_constants.cc +++ b/chromium/extensions/browser/api/management/management_api_constants.cc @@ -63,8 +63,6 @@ const char kInstallReplacementAndroidAppNotFromWebstoreError[] = "Only extensions from the web store can install replacement Android apps."; const char kInstallReplacementAndroidAppCannotInstallApp[] = "Could not install Android App."; -const char kWebContentsDisappearedError[] = - "Web contents disappeared while attempting to enable extension."; const char kParentPermissionFailedError[] = "Parent Permission Request Failed."; } // namespace extension_management_api_constants diff --git a/chromium/extensions/browser/api/management/management_api_constants.h b/chromium/extensions/browser/api/management/management_api_constants.h index d93cbbdcf57..a0c5aeb7736 100644 --- a/chromium/extensions/browser/api/management/management_api_constants.h +++ b/chromium/extensions/browser/api/management/management_api_constants.h @@ -44,7 +44,6 @@ extern const char kGestureNeededForInstallReplacementAndroidAppError[]; extern const char kInstallReplacementAndroidAppCannotInstallApp[]; extern const char kInstallReplacementAndroidAppInvalidContextError[]; extern const char kInstallReplacementAndroidAppNotFromWebstoreError[]; -extern const char kWebContentsDisappearedError[]; extern const char kParentPermissionFailedError[]; } // namespace extension_management_api_constants diff --git a/chromium/extensions/browser/api/management/supervised_user_service_delegate.h b/chromium/extensions/browser/api/management/supervised_user_service_delegate.h deleted file mode 100644 index 57b879d844c..00000000000 --- a/chromium/extensions/browser/api/management/supervised_user_service_delegate.h +++ /dev/null @@ -1,68 +0,0 @@ -// Copyright 2020 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef EXTENSIONS_BROWSER_API_MANAGEMENT_SUPERVISED_USER_SERVICE_DELEGATE_H_ -#define EXTENSIONS_BROWSER_API_MANAGEMENT_SUPERVISED_USER_SERVICE_DELEGATE_H_ - -#include "base/callback.h" -#include "extensions/common/extension.h" - -namespace content { -class BrowserContext; -class WebContents; -} // namespace content - -namespace extensions { - -class SupervisedUserServiceDelegate { - public: - virtual ~SupervisedUserServiceDelegate() = default; - - // Returns true if |context| represents a supervised child account. - virtual bool IsChild(content::BrowserContext* context) const = 0; - - // Returns true if |context| represents a supervised child account - // who may install extensions with parent permission. - virtual bool IsSupervisedChildWhoMayInstallExtensions( - content::BrowserContext* context) const = 0; - - // Returns true if the current child user is allowed to install the specified - // |extension|. - virtual bool IsExtensionAllowedByParent( - const extensions::Extension& extension, - content::BrowserContext* context) const = 0; - - // Result of the parent permission dialog invocation. - enum class ParentPermissionDialogResult { - kParentPermissionReceived, - kParentPermissionCanceled, - kParentPermissionFailed, - }; - - using ParentPermissionDialogDoneCallback = - base::OnceCallback<void(ParentPermissionDialogResult)>; - - // Shows a parent permission dialog for |extension| and call |done_callback| - // when it completes. - virtual void ShowParentPermissionDialogForExtension( - const extensions::Extension& extension, - content::BrowserContext* context, - content::WebContents* contents, - ParentPermissionDialogDoneCallback done_callback) = 0; - - // Shows a dialog indicating that |extension| has been blocked and call - // |done_callback| when it completes. - virtual void ShowExtensionEnableBlockedByParentDialogForExtension( - const extensions::Extension* extension, - content::WebContents* contents, - base::OnceClosure done_callback) = 0; - - // Records UMA metrics for supervised users trying to install or enable an - // extension when this action is blocked by the parent. - virtual void RecordExtensionEnableBlockedByParentDialogUmaMetric() = 0; -}; - -} // namespace extensions - -#endif // EXTENSIONS_BROWSER_API_MANAGEMENT_SUPERVISED_USER_SERVICE_DELEGATE_H_ diff --git a/chromium/extensions/browser/api/media_perception_private/media_perception_api_manager.cc b/chromium/extensions/browser/api/media_perception_private/media_perception_api_manager.cc index a06c5c79819..ca357c884dc 100644 --- a/chromium/extensions/browser/api/media_perception_private/media_perception_api_manager.cc +++ b/chromium/extensions/browser/api/media_perception_private/media_perception_api_manager.cc @@ -413,7 +413,6 @@ void MediaPerceptionAPIManager::SendMojoInvitation( base::kNullProcessHandle, channel.TakeLocalEndpoint()); - media_perception_service_.reset(); media_perception_service_.Bind( mojo::PendingRemote< chromeos::media_perception::mojom::MediaPerceptionService>( @@ -469,7 +468,6 @@ void MediaPerceptionAPIManager::OnBootstrapMojoConnection( return; } - media_perception_controller_.reset(); auto controller_receiver = media_perception_controller_.BindNewPipeAndPassReceiver(); diff --git a/chromium/extensions/browser/api/messaging/extension_message_port.cc b/chromium/extensions/browser/api/messaging/extension_message_port.cc index d0213afb567..fc89cf6ac3d 100644 --- a/chromium/extensions/browser/api/messaging/extension_message_port.cc +++ b/chromium/extensions/browser/api/messaging/extension_message_port.cc @@ -45,7 +45,7 @@ class ExtensionMessagePort::FrameTracker : public content::WebContentsObserver, public ProcessManagerObserver { public: explicit FrameTracker(ExtensionMessagePort* port) - : pm_observer_(this), port_(port), interstitial_frame_(nullptr) {} + : pm_observer_(this), port_(port) {} ~FrameTracker() override {} void TrackExtensionProcessFrames() { @@ -56,17 +56,6 @@ class ExtensionMessagePort::FrameTracker : public content::WebContentsObserver, Observe(tab); } - void TrackInterstitialFrame(content::WebContents* tab, - content::RenderFrameHost* interstitial_frame) { - // |tab| should never be nullptr, because an interstitial's lifetime is - // tied to a tab. This is a CHECK, not a DCHECK because we really need an - // observer subject to detect frame removal (via DidDetachInterstitialPage). - CHECK(tab); - DCHECK(interstitial_frame); - interstitial_frame_ = interstitial_frame; - Observe(tab); - } - private: // content::WebContentsObserver overrides: void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) @@ -82,11 +71,6 @@ class ExtensionMessagePort::FrameTracker : public content::WebContentsObserver, } } - void DidDetachInterstitialPage() override { - if (interstitial_frame_) - port_->UnregisterFrame(interstitial_frame_); - } - // extensions::ProcessManagerObserver overrides: void OnExtensionFrameUnregistered( const std::string& extension_id, @@ -102,11 +86,6 @@ class ExtensionMessagePort::FrameTracker : public content::WebContentsObserver, ScopedObserver<ProcessManager, ProcessManagerObserver> pm_observer_; ExtensionMessagePort* port_; // Owns this FrameTracker. - // Set to the main frame of an interstitial if we are tracking an interstitial - // page, because RenderFrameDeleted is never triggered for frames in an - // interstitial (and we only support tracking the interstitial's main frame). - content::RenderFrameHost* interstitial_frame_; - DISALLOW_COPY_AND_ASSIGN(FrameTracker); }; diff --git a/chromium/extensions/browser/api/networking_private/networking_private_chromeos_unittest.cc b/chromium/extensions/browser/api/networking_private/networking_private_chromeos_unittest.cc index 208cfd3398d..fed448ac062 100644 --- a/chromium/extensions/browser/api/networking_private/networking_private_chromeos_unittest.cc +++ b/chromium/extensions/browser/api/networking_private/networking_private_chromeos_unittest.cc @@ -1099,6 +1099,7 @@ TEST_F(NetworkingPrivateApiTest, GetCellularProperties) { .Build()) .Set("ConnectionState", "Connected") .Set("GUID", "cellular_guid") + .Set("Metered", true) .Set("Name", "cellular") .Set("Source", "User") .Set("Type", "Cellular") @@ -1158,6 +1159,7 @@ TEST_F(NetworkingPrivateApiTest, GetCellularPropertiesFromWebUi) { .Build()) .Set("ConnectionState", "Connected") .Set("GUID", "cellular_guid") + .Set("Metered", true) .Set("Name", "cellular") .Set("Source", "User") .Set("Type", "Cellular") diff --git a/chromium/extensions/browser/api/networking_private/networking_private_linux.cc b/chromium/extensions/browser/api/networking_private/networking_private_linux.cc index f938c2d4715..0a0bcd94f9d 100644 --- a/chromium/extensions/browser/api/networking_private/networking_private_linux.cc +++ b/chromium/extensions/browser/api/networking_private/networking_private_linux.cc @@ -13,7 +13,6 @@ #include "base/strings/string16.h" #include "base/strings/string_split.h" #include "base/strings/utf_string_conversions.h" -#include "base/task/post_task.h" #include "components/onc/onc_constants.h" #include "content/public/browser/browser_task_traits.h" #include "content/public/browser/browser_thread.h" @@ -136,7 +135,7 @@ void GetCachedNetworkPropertiesCallback( } // namespace NetworkingPrivateLinux::NetworkingPrivateLinux() - : dbus_thread_("Networking Private DBus"), network_manager_proxy_(NULL) { + : dbus_thread_("Networking Private DBus"), network_manager_proxy_(nullptr) { base::Thread::Options thread_options(base::MessagePumpType::IO, 0); dbus_thread_.StartWithOptions(thread_options); @@ -1221,8 +1220,8 @@ void NetworkingPrivateLinux::PostOnNetworksChangedToUIThread( std::unique_ptr<GuidList> guid_list) { AssertOnDBusThread(); - base::PostTask( - FROM_HERE, {content::BrowserThread::UI}, + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&NetworkingPrivateLinux::OnNetworksChangedEventTask, base::Unretained(this), std::move(guid_list))); } diff --git a/chromium/extensions/browser/api/serial/serial_api.cc b/chromium/extensions/browser/api/serial/serial_api.cc index 096f736cba1..a0bfd248b16 100644 --- a/chromium/extensions/browser/api/serial/serial_api.cc +++ b/chromium/extensions/browser/api/serial/serial_api.cc @@ -81,8 +81,35 @@ ExtensionFunction::ResponseAction SerialGetDevicesFunction::Run() { void SerialGetDevicesFunction::OnGotDevices( std::vector<device::mojom::SerialPortInfoPtr> devices) { - Respond(ArgumentList(serial::GetDevices::Results::Create( - mojo::ConvertTo<std::vector<serial::DeviceInfo>>(devices)))); + std::vector<extensions::api::serial::DeviceInfo> results; + for (const auto& device : devices) { + extensions::api::serial::DeviceInfo info; + info.path = device->path.AsUTF8Unsafe(); + if (device->has_vendor_id) + info.vendor_id = std::make_unique<int>(device->vendor_id); + if (device->has_product_id) + info.product_id = std::make_unique<int>(device->product_id); + if (device->display_name) + info.display_name = std::make_unique<std::string>(*device->display_name); + results.push_back(std::move(info)); + +#if defined(OS_MACOSX) + if (device->alternate_path) { + extensions::api::serial::DeviceInfo alternate_info; + alternate_info.path = device->alternate_path->AsUTF8Unsafe(); + if (device->has_vendor_id) + alternate_info.vendor_id = std::make_unique<int>(device->vendor_id); + if (device->has_product_id) + alternate_info.product_id = std::make_unique<int>(device->product_id); + if (device->display_name) { + alternate_info.display_name = + std::make_unique<std::string>(*device->display_name); + } + results.push_back(std::move(alternate_info)); + } +#endif // defined(OS_MACOSX) + } + Respond(ArgumentList(serial::GetDevices::Results::Create(results))); } SerialConnectFunction::SerialConnectFunction() {} @@ -438,23 +465,3 @@ void SerialClearBreakFunction::OnClearBreak(bool success) { } // namespace api } // namespace extensions - -namespace mojo { - -// static -extensions::api::serial::DeviceInfo -TypeConverter<extensions::api::serial::DeviceInfo, - device::mojom::SerialPortInfoPtr>:: - Convert(const device::mojom::SerialPortInfoPtr& device) { - extensions::api::serial::DeviceInfo info; - info.path = device->path.AsUTF8Unsafe(); - if (device->has_vendor_id) - info.vendor_id.reset(new int(static_cast<int>(device->vendor_id))); - if (device->has_product_id) - info.product_id.reset(new int(static_cast<int>(device->product_id))); - if (device->display_name) - info.display_name.reset(new std::string(device->display_name.value())); - return info; -} - -} // namespace mojo diff --git a/chromium/extensions/browser/api/serial/serial_apitest.cc b/chromium/extensions/browser/api/serial/serial_apitest.cc index c268ac59e72..9aa02113827 100644 --- a/chromium/extensions/browser/api/serial/serial_apitest.cc +++ b/chromium/extensions/browser/api/serial/serial_apitest.cc @@ -86,8 +86,6 @@ class FakeSerialPort : public device::mojom::SerialPort { private: // device::mojom::SerialPort methods: void Open(device::mojom::SerialConnectionOptionsPtr options, - mojo::ScopedDataPipeConsumerHandle in_stream, - mojo::ScopedDataPipeProducerHandle out_stream, mojo::PendingRemote<device::mojom::SerialPortClient> client, OpenCallback callback) override { if (client_) { @@ -99,22 +97,35 @@ class FakeSerialPort : public device::mojom::SerialPort { DoConfigurePort(*options); DCHECK(client); client_.Bind(std::move(client)); - SetUpInStreamPipe(std::move(in_stream)); - SetUpOutStreamPipe(std::move(out_stream)); std::move(callback).Run(true); } - void ClearSendError(mojo::ScopedDataPipeConsumerHandle consumer) override { - if (in_stream_) { + + void StartWriting(mojo::ScopedDataPipeConsumerHandle consumer) override { + if (in_stream_) return; - } - SetUpInStreamPipe(std::move(consumer)); + + in_stream_ = std::move(consumer); + in_stream_watcher_.Watch( + in_stream_.get(), + MOJO_HANDLE_SIGNAL_READABLE | MOJO_HANDLE_SIGNAL_PEER_CLOSED, + MOJO_TRIGGER_CONDITION_SIGNALS_SATISFIED, + base::BindRepeating(&FakeSerialPort::DoWrite, base::Unretained(this))); + in_stream_watcher_.ArmOrNotify(); } - void ClearReadError(mojo::ScopedDataPipeProducerHandle producer) override { - if (out_stream_) { + + void StartReading(mojo::ScopedDataPipeProducerHandle producer) override { + if (out_stream_) return; - } - SetUpOutStreamPipe(std::move(producer)); + + out_stream_ = std::move(producer); + out_stream_watcher_.Watch( + out_stream_.get(), + MOJO_HANDLE_SIGNAL_WRITABLE | MOJO_HANDLE_SIGNAL_PEER_CLOSED, + MOJO_TRIGGER_CONDITION_SIGNALS_SATISFIED, + base::BindRepeating(&FakeSerialPort::DoRead, base::Unretained(this))); + out_stream_watcher_.ArmOrNotify(); } + void Flush(FlushCallback callback) override { std::move(callback).Run(true); } void GetControlSignals(GetControlSignalsCallback callback) override { auto signals = device::mojom::SerialPortControlSignals::New(); @@ -152,16 +163,6 @@ class FakeSerialPort : public device::mojom::SerialPort { std::move(callback).Run(); } - void SetUpInStreamPipe(mojo::ScopedDataPipeConsumerHandle consumer) { - in_stream_.swap(consumer); - in_stream_watcher_.Watch( - in_stream_.get(), - MOJO_HANDLE_SIGNAL_READABLE | MOJO_HANDLE_SIGNAL_PEER_CLOSED, - MOJO_TRIGGER_CONDITION_SIGNALS_SATISFIED, - base::BindRepeating(&FakeSerialPort::DoWrite, base::Unretained(this))); - in_stream_watcher_.ArmOrNotify(); - } - void DoWrite(MojoResult result, const mojo::HandleSignalsState& state) { const void* data; uint32_t num_bytes; @@ -201,16 +202,6 @@ class FakeSerialPort : public device::mojom::SerialPort { NOTREACHED(); } - void SetUpOutStreamPipe(mojo::ScopedDataPipeProducerHandle producer) { - out_stream_.swap(producer); - out_stream_watcher_.Watch( - out_stream_.get(), - MOJO_HANDLE_SIGNAL_WRITABLE | MOJO_HANDLE_SIGNAL_PEER_CLOSED, - MOJO_TRIGGER_CONDITION_SIGNALS_SATISFIED, - base::BindRepeating(&FakeSerialPort::DoRead, base::Unretained(this))); - out_stream_watcher_.ArmOrNotify(); - } - void DoRead(MojoResult result, const mojo::HandleSignalsState& state) { if (result != MOJO_RESULT_OK) { out_stream_.reset(); @@ -311,6 +302,7 @@ class FakeSerialPortManager : public device::mojom::SerialPortManager { } void GetPort(const base::UnguessableToken& token, + bool use_alternate_path, mojo::PendingReceiver<device::mojom::SerialPort> receiver, mojo::PendingRemote<device::mojom::SerialPortConnectionWatcher> watcher) override { diff --git a/chromium/extensions/browser/api/serial/serial_connection.cc b/chromium/extensions/browser/api/serial/serial_connection.cc index f8fb970a6e5..5df8db36db8 100644 --- a/chromium/extensions/browser/api/serial/serial_connection.cc +++ b/chromium/extensions/browser/api/serial/serial_connection.cc @@ -209,13 +209,8 @@ void SerialConnection::SetPaused(bool paused) { } else { // If |receive_pipe_| is closed and there is no pending ReceiveError event, // try to reconnect the data pipe. - if (!receive_pipe_ && !read_error_) { - mojo::ScopedDataPipeProducerHandle producer; - mojo::ScopedDataPipeConsumerHandle consumer; - CreatePipe(&producer, &consumer); - SetUpReceiveDataPipe(std::move(consumer)); - serial_port_->ClearReadError(std::move(producer)); - } + if (!receive_pipe_ && !read_error_) + SetUpReceiveDataPipe(); receive_pipe_watcher_.ArmOrNotify(); receive_timeout_task_.Cancel(); SetTimeoutCallback(); @@ -249,22 +244,12 @@ void SerialConnection::Open(const api::serial::ConnectionOptions& options, if (options.send_timeout.get()) set_send_timeout(*options.send_timeout); - mojo::ScopedDataPipeProducerHandle receive_producer; - mojo::ScopedDataPipeConsumerHandle receive_consumer; - CreatePipe(&receive_producer, &receive_consumer); - - mojo::ScopedDataPipeProducerHandle send_producer; - mojo::ScopedDataPipeConsumerHandle send_consumer; - CreatePipe(&send_producer, &send_consumer); - mojo::PendingRemote<device::mojom::SerialPortClient> client; auto client_receiver = client.InitWithNewPipeAndPassReceiver(); serial_port_->Open( - device::mojom::SerialConnectionOptions::From(options), - std::move(send_consumer), std::move(receive_producer), std::move(client), + device::mojom::SerialConnectionOptions::From(options), std::move(client), mojo::WrapCallbackWithDefaultInvokeIfNotRun( base::BindOnce(&SerialConnection::OnOpen, weak_factory_.GetWeakPtr(), - std::move(receive_consumer), std::move(send_producer), std::move(client_receiver), std::move(callback)), false)); } @@ -281,9 +266,12 @@ void SerialConnection::CreatePipe( CHECK_EQ(MOJO_RESULT_OK, mojo::CreateDataPipe(&options, producer, consumer)); } -void SerialConnection::SetUpReceiveDataPipe( - mojo::ScopedDataPipeConsumerHandle consumer) { - receive_pipe_ = std::move(consumer); +void SerialConnection::SetUpReceiveDataPipe() { + mojo::ScopedDataPipeProducerHandle producer; + CreatePipe(&producer, &receive_pipe_); + + serial_port_->StartReading(std::move(producer)); + receive_pipe_watcher_.Watch( receive_pipe_.get(), MOJO_HANDLE_SIGNAL_READABLE | MOJO_HANDLE_SIGNAL_PEER_CLOSED, @@ -292,9 +280,12 @@ void SerialConnection::SetUpReceiveDataPipe( weak_factory_.GetWeakPtr())); } -void SerialConnection::SetUpSendDataPipe( - mojo::ScopedDataPipeProducerHandle producer) { - send_pipe_ = std::move(producer); +void SerialConnection::SetUpSendDataPipe() { + mojo::ScopedDataPipeConsumerHandle consumer; + CreatePipe(&send_pipe_, &consumer); + + serial_port_->StartWriting(std::move(consumer)); + send_pipe_watcher_.Watch( send_pipe_.get(), MOJO_HANDLE_SIGNAL_WRITABLE | MOJO_HANDLE_SIGNAL_PEER_CLOSED, @@ -330,8 +321,6 @@ void SerialConnection::OnSendError(device::mojom::SerialSendError error) { } void SerialConnection::OnOpen( - mojo::ScopedDataPipeConsumerHandle consumer, - mojo::ScopedDataPipeProducerHandle producer, mojo::PendingReceiver<device::mojom::SerialPortClient> client_receiver, OpenCompleteCallback callback, bool success) { @@ -340,8 +329,8 @@ void SerialConnection::OnOpen( return; } - SetUpReceiveDataPipe(std::move(consumer)); - SetUpSendDataPipe(std::move(producer)); + SetUpReceiveDataPipe(); + SetUpSendDataPipe(); client_receiver_.Bind(std::move(client_receiver)); client_receiver_.set_disconnect_handler(base::BindOnce( &SerialConnection::OnClientReceiverClosed, weak_factory_.GetWeakPtr())); @@ -419,13 +408,8 @@ bool SerialConnection::Send(const std::vector<uint8_t>& data, DCHECK(data_to_send_.empty()); data_to_send_.assign(data.begin(), data.end()); - if (!send_pipe_) { - mojo::ScopedDataPipeProducerHandle producer; - mojo::ScopedDataPipeConsumerHandle consumer; - CreatePipe(&producer, &consumer); - SetUpSendDataPipe(std::move(producer)); - serial_port_->ClearSendError(std::move(consumer)); - } + if (!send_pipe_) + SetUpSendDataPipe(); send_pipe_watcher_.ArmOrNotify(); send_timeout_task_.Cancel(); diff --git a/chromium/extensions/browser/api/serial/serial_connection.h b/chromium/extensions/browser/api/serial/serial_connection.h index 9ee96ad3758..29cf85cb16e 100644 --- a/chromium/extensions/browser/api/serial/serial_connection.h +++ b/chromium/extensions/browser/api/serial/serial_connection.h @@ -146,8 +146,6 @@ class SerialConnection : public ApiResource, void OnSendError(device::mojom::SerialSendError error) override; void OnOpen( - mojo::ScopedDataPipeConsumerHandle consumer, - mojo::ScopedDataPipeProducerHandle producer, mojo::PendingReceiver<device::mojom::SerialPortClient> client_receiver, OpenCompleteCallback callback, bool success); @@ -160,8 +158,8 @@ class SerialConnection : public ApiResource, void CreatePipe(mojo::ScopedDataPipeProducerHandle* producer, mojo::ScopedDataPipeConsumerHandle* consumer); - void SetUpReceiveDataPipe(mojo::ScopedDataPipeConsumerHandle producer); - void SetUpSendDataPipe(mojo::ScopedDataPipeProducerHandle consumer); + void SetUpReceiveDataPipe(); + void SetUpSendDataPipe(); void SetTimeoutCallback(); diff --git a/chromium/extensions/browser/api/serial/serial_port_manager.cc b/chromium/extensions/browser/api/serial/serial_port_manager.cc index 4194ea83be0..559eb121954 100644 --- a/chromium/extensions/browser/api/serial/serial_port_manager.cc +++ b/chromium/extensions/browser/api/serial/serial_port_manager.cc @@ -9,6 +9,7 @@ #include "base/bind.h" #include "base/lazy_instance.h" #include "base/no_destructor.h" +#include "build/build_config.h" #include "content/public/browser/browser_task_traits.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/device_service.h" @@ -188,10 +189,21 @@ void SerialPortManager::OnGotDevicesToGetPort( for (auto& device : devices) { if (device->path.AsUTF8Unsafe() == path) { - port_manager_->GetPort(device->token, std::move(receiver), + port_manager_->GetPort(device->token, /*use_alternate_path=*/false, + std::move(receiver), /*watcher=*/mojo::NullRemote()); return; } + +#if defined(OS_MACOSX) + if (device->alternate_path && + device->alternate_path->AsUTF8Unsafe() == path) { + port_manager_->GetPort(device->token, /*use_alternate_path=*/true, + std::move(receiver), + /*watcher=*/mojo::NullRemote()); + return; + } +#endif // defined(OS_MACOSX) } } diff --git a/chromium/extensions/browser/api/socket/socket_api.cc b/chromium/extensions/browser/api/socket/socket_api.cc index 5227ee331cd..6daca9a2902 100644 --- a/chromium/extensions/browser/api/socket/socket_api.cc +++ b/chromium/extensions/browser/api/socket/socket_api.cc @@ -9,7 +9,6 @@ #include <vector> #include "base/bind.h" -#include "base/task/post_task.h" #include "base/values.h" #include "build/build_config.h" #include "content/public/browser/browser_context.h" @@ -127,8 +126,8 @@ void SocketAsyncApiFunction::OpenFirewallHole(const std::string& address, ? AppFirewallHole::PortType::TCP : AppFirewallHole::PortType::UDP; - base::PostTask( - FROM_HERE, {BrowserThread::UI}, + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&SocketAsyncApiFunction::OpenFirewallHoleOnUIThread, this, type, local_address.port(), socket_id)); return; @@ -148,8 +147,8 @@ void SocketAsyncApiFunction::OpenFirewallHoleOnUIThread( AppFirewallHoleManager::Get(browser_context()); std::unique_ptr<AppFirewallHole, BrowserThread::DeleteOnUIThread> hole( manager->Open(type, port, extension_id()).release()); - base::PostTask(FROM_HERE, {BrowserThread::IO}, - base::BindOnce(&SocketAsyncApiFunction::OnFirewallHoleOpened, + content::GetIOThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&SocketAsyncApiFunction::OnFirewallHoleOpened, this, socket_id, std::move(hole))); } diff --git a/chromium/extensions/browser/api/socket/socket_api.h b/chromium/extensions/browser/api/socket/socket_api.h index 8b2ac098706..c1c7904d8fd 100644 --- a/chromium/extensions/browser/api/socket/socket_api.h +++ b/chromium/extensions/browser/api/socket/socket_api.h @@ -72,7 +72,7 @@ class SocketResourceManagerInterface { template <typename T> class SocketResourceManager : public SocketResourceManagerInterface { public: - SocketResourceManager() : manager_(NULL) {} + SocketResourceManager() : manager_(nullptr) {} bool SetBrowserContext(content::BrowserContext* context) override { manager_ = ApiResourceManager<T>::Get(context); @@ -81,7 +81,7 @@ class SocketResourceManager : public SocketResourceManagerInterface { "If this assertion is failing during a test, then it is likely that " "TestExtensionSystem is failing to provide an instance of " "ApiResourceManager<Socket>."; - return manager_ != NULL; + return !!manager_; } int Add(Socket* socket) override { diff --git a/chromium/extensions/browser/api/socket/tcp_socket.cc b/chromium/extensions/browser/api/socket/tcp_socket.cc index b01331a2c09..9aafd37de0f 100644 --- a/chromium/extensions/browser/api/socket/tcp_socket.cc +++ b/chromium/extensions/browser/api/socket/tcp_socket.cc @@ -12,7 +12,6 @@ #include "base/logging.h" #include "base/macros.h" #include "base/memory/ptr_util.h" -#include "base/task/post_task.h" #include "content/public/browser/browser_context.h" #include "content/public/browser/browser_task_traits.h" #include "content/public/browser/browser_thread.h" @@ -139,8 +138,8 @@ void TCPSocket::Connect(const net::AddressList& address, base::BindOnce(&TCPSocket::OnConnectCompleteOnUIThread, task_runner_, std::move(completion_callback)); - base::PostTask(FROM_HERE, {content::BrowserThread::UI}, - base::BindOnce(&TCPSocket::ConnectOnUIThread, + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&TCPSocket::ConnectOnUIThread, storage_partition_, browser_context_, address, client_socket_.BindNewPipeAndPassReceiver(), std::move(completion_callback_ui))); @@ -263,8 +262,8 @@ void TCPSocket::Listen(const std::string& address, base::BindOnce(&TCPSocket::OnListenCompleteOnUIThread, task_runner_, std::move(completion_callback)); - base::PostTask( - FROM_HERE, {content::BrowserThread::UI}, + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&TCPSocket::ListenOnUIThread, storage_partition_, browser_context_, ip_end_point, backlog, server_socket_.BindNewPipeAndPassReceiver(), diff --git a/chromium/extensions/browser/api/sockets_tcp/sockets_tcp_api.cc b/chromium/extensions/browser/api/sockets_tcp/sockets_tcp_api.cc index 73597138d33..1cd59b1f01f 100644 --- a/chromium/extensions/browser/api/sockets_tcp/sockets_tcp_api.cc +++ b/chromium/extensions/browser/api/sockets_tcp/sockets_tcp_api.cc @@ -163,7 +163,7 @@ void SocketsTcpUpdateFunction::Work() { } SocketsTcpSetPausedFunction::SocketsTcpSetPausedFunction() - : socket_event_dispatcher_(NULL) {} + : socket_event_dispatcher_(nullptr) {} SocketsTcpSetPausedFunction::~SocketsTcpSetPausedFunction() {} @@ -177,7 +177,7 @@ bool SocketsTcpSetPausedFunction::Prepare() { "If this assertion is failing during a test, then it is likely that " "TestExtensionSystem is failing to provide an instance of " "TCPSocketEventDispatcher."; - return socket_event_dispatcher_ != NULL; + return !!socket_event_dispatcher_; } void SocketsTcpSetPausedFunction::Work() { @@ -264,7 +264,7 @@ void SocketsTcpSetNoDelayFunction::OnCompleted(bool success) { } SocketsTcpConnectFunction::SocketsTcpConnectFunction() - : socket_event_dispatcher_(NULL) {} + : socket_event_dispatcher_(nullptr) {} SocketsTcpConnectFunction::~SocketsTcpConnectFunction() {} @@ -278,7 +278,7 @@ bool SocketsTcpConnectFunction::Prepare() { "If this assertion is failing during a test, then it is likely that " "TestExtensionSystem is failing to provide an instance of " "TCPSocketEventDispatcher."; - return socket_event_dispatcher_ != NULL; + return !!socket_event_dispatcher_; } void SocketsTcpConnectFunction::AsyncWorkStart() { @@ -457,7 +457,7 @@ bool SocketsTcpGetSocketsFunction::Prepare() { return true; } void SocketsTcpGetSocketsFunction::Work() { std::vector<sockets_tcp::SocketInfo> socket_infos; std::unordered_set<int>* resource_ids = GetSocketIds(); - if (resource_ids != NULL) { + if (resource_ids) { for (int socket_id : *resource_ids) { ResumableTCPSocket* socket = GetTcpSocket(socket_id); if (socket) { diff --git a/chromium/extensions/browser/api/sockets_tcp/tcp_socket_event_dispatcher.cc b/chromium/extensions/browser/api/sockets_tcp/tcp_socket_event_dispatcher.cc index 69f7e9c1be8..8bab3a92a30 100644 --- a/chromium/extensions/browser/api/sockets_tcp/tcp_socket_event_dispatcher.cc +++ b/chromium/extensions/browser/api/sockets_tcp/tcp_socket_event_dispatcher.cc @@ -183,8 +183,8 @@ void TCPSocketEventDispatcher::PostEvent(const ReadParams& params, std::unique_ptr<Event> event) { DCHECK_CURRENTLY_ON(params.thread_id); - base::PostTask(FROM_HERE, {BrowserThread::UI}, - base::BindOnce(&DispatchEvent, params.browser_context_id, + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&DispatchEvent, params.browser_context_id, params.extension_id, std::move(event))); } diff --git a/chromium/extensions/browser/api/sockets_tcp_server/sockets_tcp_server_api.cc b/chromium/extensions/browser/api/sockets_tcp_server/sockets_tcp_server_api.cc index 4664d5808c2..f7b4cf5d008 100644 --- a/chromium/extensions/browser/api/sockets_tcp_server/sockets_tcp_server_api.cc +++ b/chromium/extensions/browser/api/sockets_tcp_server/sockets_tcp_server_api.cc @@ -123,7 +123,7 @@ void SocketsTcpServerUpdateFunction::Work() { } SocketsTcpServerSetPausedFunction::SocketsTcpServerSetPausedFunction() - : socket_event_dispatcher_(NULL) {} + : socket_event_dispatcher_(nullptr) {} SocketsTcpServerSetPausedFunction::~SocketsTcpServerSetPausedFunction() {} @@ -138,7 +138,7 @@ bool SocketsTcpServerSetPausedFunction::Prepare() { "If this assertion is failing during a test, then it is likely that " "TestExtensionSystem is failing to provide an instance of " "TCPServerSocketEventDispatcher."; - return socket_event_dispatcher_ != NULL; + return !!socket_event_dispatcher_; } void SocketsTcpServerSetPausedFunction::Work() { @@ -160,7 +160,7 @@ void SocketsTcpServerSetPausedFunction::Work() { } SocketsTcpServerListenFunction::SocketsTcpServerListenFunction() - : socket_event_dispatcher_(NULL) {} + : socket_event_dispatcher_(nullptr) {} SocketsTcpServerListenFunction::~SocketsTcpServerListenFunction() {} @@ -175,7 +175,7 @@ bool SocketsTcpServerListenFunction::Prepare() { "If this assertion is failing during a test, then it is likely that " "TestExtensionSystem is failing to provide an instance of " "TCPServerSocketEventDispatcher."; - return socket_event_dispatcher_ != NULL; + return !!socket_event_dispatcher_; } void SocketsTcpServerListenFunction::AsyncWorkStart() { @@ -297,7 +297,7 @@ bool SocketsTcpServerGetSocketsFunction::Prepare() { return true; } void SocketsTcpServerGetSocketsFunction::Work() { std::vector<sockets_tcp_server::SocketInfo> socket_infos; std::unordered_set<int>* resource_ids = GetSocketIds(); - if (resource_ids != NULL) { + if (resource_ids) { for (int socket_id : *resource_ids) { ResumableTCPServerSocket* socket = GetTcpSocket(socket_id); if (socket) { diff --git a/chromium/extensions/browser/api/sockets_tcp_server/tcp_server_socket_event_dispatcher.cc b/chromium/extensions/browser/api/sockets_tcp_server/tcp_server_socket_event_dispatcher.cc index 14d16fd8f87..587a0d12a33 100644 --- a/chromium/extensions/browser/api/sockets_tcp_server/tcp_server_socket_event_dispatcher.cc +++ b/chromium/extensions/browser/api/sockets_tcp_server/tcp_server_socket_event_dispatcher.cc @@ -185,8 +185,8 @@ void TCPServerSocketEventDispatcher::PostEvent(const AcceptParams& params, std::unique_ptr<Event> event) { DCHECK_CURRENTLY_ON(params.thread_id); - base::PostTask(FROM_HERE, {BrowserThread::UI}, - base::BindOnce(&DispatchEvent, params.browser_context_id, + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&DispatchEvent, params.browser_context_id, params.extension_id, std::move(event))); } diff --git a/chromium/extensions/browser/api/sockets_udp/sockets_udp_api.cc b/chromium/extensions/browser/api/sockets_udp/sockets_udp_api.cc index 2c17cbe5308..2c7ca4a6503 100644 --- a/chromium/extensions/browser/api/sockets_udp/sockets_udp_api.cc +++ b/chromium/extensions/browser/api/sockets_udp/sockets_udp_api.cc @@ -142,7 +142,7 @@ void SocketsUdpUpdateFunction::Work() { } SocketsUdpSetPausedFunction::SocketsUdpSetPausedFunction() - : socket_event_dispatcher_(NULL) {} + : socket_event_dispatcher_(nullptr) {} SocketsUdpSetPausedFunction::~SocketsUdpSetPausedFunction() {} @@ -156,7 +156,7 @@ bool SocketsUdpSetPausedFunction::Prepare() { "If this assertion is failing during a test, then it is likely that " "TestExtensionSystem is failing to provide an instance of " "UDPSocketEventDispatcher."; - return socket_event_dispatcher_ != NULL; + return !!socket_event_dispatcher_; } void SocketsUdpSetPausedFunction::Work() { @@ -178,7 +178,7 @@ void SocketsUdpSetPausedFunction::Work() { } SocketsUdpBindFunction::SocketsUdpBindFunction() - : socket_event_dispatcher_(NULL) {} + : socket_event_dispatcher_(nullptr) {} SocketsUdpBindFunction::~SocketsUdpBindFunction() {} @@ -192,7 +192,7 @@ bool SocketsUdpBindFunction::Prepare() { "If this assertion is failing during a test, then it is likely that " "TestExtensionSystem is failing to provide an instance of " "UDPSocketEventDispatcher."; - return socket_event_dispatcher_ != NULL; + return !!socket_event_dispatcher_; } void SocketsUdpBindFunction::AsyncWorkStart() { @@ -363,7 +363,7 @@ bool SocketsUdpGetSocketsFunction::Prepare() { return true; } void SocketsUdpGetSocketsFunction::Work() { std::vector<sockets_udp::SocketInfo> socket_infos; std::unordered_set<int>* resource_ids = GetSocketIds(); - if (resource_ids != NULL) { + if (resource_ids) { for (int socket_id : *resource_ids) { ResumableUDPSocket* socket = GetUdpSocket(socket_id); if (socket) { diff --git a/chromium/extensions/browser/api/sockets_udp/udp_socket_event_dispatcher.cc b/chromium/extensions/browser/api/sockets_udp/udp_socket_event_dispatcher.cc index 14774d2cf85..a1fd9f34e15 100644 --- a/chromium/extensions/browser/api/sockets_udp/udp_socket_event_dispatcher.cc +++ b/chromium/extensions/browser/api/sockets_udp/udp_socket_event_dispatcher.cc @@ -171,8 +171,8 @@ void UDPSocketEventDispatcher::PostEvent(const ReceiveParams& params, std::unique_ptr<Event> event) { DCHECK_CURRENTLY_ON(params.thread_id); - base::PostTask(FROM_HERE, {BrowserThread::UI}, - base::BindOnce(&DispatchEvent, params.browser_context_id, + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&DispatchEvent, params.browser_context_id, params.extension_id, std::move(event))); } diff --git a/chromium/extensions/browser/api/storage/settings_storage_quota_enforcer.cc b/chromium/extensions/browser/api/storage/settings_storage_quota_enforcer.cc index 87c6bc64dea..6523da98fac 100644 --- a/chromium/extensions/browser/api/storage/settings_storage_quota_enforcer.cc +++ b/chromium/extensions/browser/api/storage/settings_storage_quota_enforcer.cc @@ -8,6 +8,7 @@ #include "base/bind.h" #include "base/json/json_writer.h" +#include "base/logging.h" #include "base/stl_util.h" #include "base/strings/stringprintf.h" #include "extensions/common/extension_api.h" diff --git a/chromium/extensions/browser/api/storage/storage_api.cc b/chromium/extensions/browser/api/storage/storage_api.cc index 04768c98a98..9a3b94d6cf8 100644 --- a/chromium/extensions/browser/api/storage/storage_api.cc +++ b/chromium/extensions/browser/api/storage/storage_api.cc @@ -12,7 +12,6 @@ #include "base/bind.h" #include "base/strings/stringprintf.h" -#include "base/task/post_task.h" #include "base/values.h" #include "content/public/browser/browser_task_traits.h" #include "content/public/browser/browser_thread.h" @@ -22,8 +21,6 @@ namespace extensions { -using content::BrowserThread; - // SettingsFunction SettingsFunction::SettingsFunction() @@ -77,8 +74,8 @@ ExtensionFunction::ResponseAction SettingsFunction::Run() { void SettingsFunction::AsyncRunWithStorage(ValueStore* storage) { ResponseValue response = RunWithStorage(storage); - base::PostTask( - FROM_HERE, {BrowserThread::UI}, + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&SettingsFunction::Respond, this, std::move(response))); } diff --git a/chromium/extensions/browser/api/system_storage/system_storage_eject_apitest.cc b/chromium/extensions/browser/api/system_storage/system_storage_eject_apitest.cc index 5fb71798efc..d87a81a33b1 100644 --- a/chromium/extensions/browser/api/system_storage/system_storage_eject_apitest.cc +++ b/chromium/extensions/browser/api/system_storage/system_storage_eject_apitest.cc @@ -34,7 +34,7 @@ using storage_monitor::TestStorageMonitor; class SystemStorageEjectApiTest : public extensions::ShellApiTest { public: - SystemStorageEjectApiTest() : monitor_(NULL) {} + SystemStorageEjectApiTest() : monitor_(nullptr) {} ~SystemStorageEjectApiTest() override {} protected: diff --git a/chromium/extensions/browser/api/test/test_api.cc b/chromium/extensions/browser/api/test/test_api.cc index 01e9d167057..8458efa2f41 100644 --- a/chromium/extensions/browser/api/test/test_api.cc +++ b/chromium/extensions/browser/api/test/test_api.cc @@ -129,7 +129,7 @@ void TestGetConfigFunction::set_test_config_state( } TestGetConfigFunction::TestConfigState::TestConfigState() - : config_state_(NULL) {} + : config_state_(nullptr) {} // static TestGetConfigFunction::TestConfigState* diff --git a/chromium/extensions/browser/api/vpn_provider/vpn_service.cc b/chromium/extensions/browser/api/vpn_provider/vpn_service.cc index 16c453e67fa..ee4624091d8 100644 --- a/chromium/extensions/browser/api/vpn_provider/vpn_service.cc +++ b/chromium/extensions/browser/api/vpn_provider/vpn_service.cc @@ -545,7 +545,7 @@ void VpnService::OnExtensionUnloaded( base::DoNothing(), base::BindOnce(DoNothingFailureCallback)); } if (reason == extensions::UnloadedExtensionReason::DISABLE || - reason == extensions::UnloadedExtensionReason::BLACKLIST) { + reason == extensions::UnloadedExtensionReason::BLOCKLIST) { DestroyConfigurationsForExtension(extension); } } diff --git a/chromium/extensions/browser/api/web_request/BUILD.gn b/chromium/extensions/browser/api/web_request/BUILD.gn index 117c87d9f72..ac64cd54722 100644 --- a/chromium/extensions/browser/api/web_request/BUILD.gn +++ b/chromium/extensions/browser/api/web_request/BUILD.gn @@ -43,6 +43,7 @@ source_set("web_request") { ] deps = [ + "//components/ukm/content", "//components/web_cache/browser", "//content/public/browser", "//content/public/common", diff --git a/chromium/extensions/browser/api/web_request/web_request_api.cc b/chromium/extensions/browser/api/web_request/web_request_api.cc index 50db1d2502b..7c5498c16b1 100644 --- a/chromium/extensions/browser/api/web_request/web_request_api.cc +++ b/chromium/extensions/browser/api/web_request/web_request_api.cc @@ -26,6 +26,7 @@ #include "base/task/post_task.h" #include "base/time/time.h" #include "base/values.h" +#include "components/ukm/content/source_url_recorder.h" #include "content/public/browser/browser_context.h" #include "content/public/browser/browser_task_traits.h" #include "content/public/browser/browser_thread.h" @@ -689,11 +690,11 @@ bool WebRequestAPI::MaybeProxyURLLoaderFactory( mojo::PendingRemote<network::mojom::TrustedURLLoaderHeaderClient>* header_client) { DCHECK_CURRENTLY_ON(BrowserThread::UI); + auto* web_contents = content::WebContents::FromRenderFrameHost(frame); if (!MayHaveProxies()) { bool skip_proxy = true; // There are a few internal WebUIs that use WebView tag that are whitelisted // for webRequest. - auto* web_contents = content::WebContents::FromRenderFrameHost(frame); if (web_contents && WebViewGuest::IsGuest(web_contents)) { auto* guest_web_contents = WebViewGuest::GetTopLevelWebContents(web_contents); @@ -742,12 +743,15 @@ bool WebRequestAPI::MaybeProxyURLLoaderFactory( (browser_context->IsOffTheRecord() && ExtensionsBrowserClient::Get()->GetOriginalContext(browser_context) == browser_context_)); + const ukm::SourceId ukm_source_id = + web_contents ? ukm::GetSourceIdForWebContentsDocument(web_contents) + : ukm::kInvalidSourceId; WebRequestProxyingURLLoaderFactory::StartProxying( browser_context, is_navigation ? -1 : render_process_id, &request_id_generator_, std::move(navigation_ui_data), std::move(navigation_id), std::move(proxied_receiver), std::move(target_factory_remote), std::move(header_client_receiver), - proxies_.get(), type); + proxies_.get(), type, ukm_source_id); return true; } @@ -893,10 +897,9 @@ struct ExtensionWebRequestEventRouter::BlockedRequest { // for OnBeforeSendHeaders. net::HttpRequestHeaders* request_headers = nullptr; - // The response headers that were received from the server and subsequently - // filtered by the Declarative Net Request API. Only valid for + // The response headers that were received from the server. Only valid for // OnHeadersReceived. - scoped_refptr<const net::HttpResponseHeaders> filtered_response_headers; + scoped_refptr<const net::HttpResponseHeaders> original_response_headers; // Location where to override response headers. Only valid for // OnHeadersReceived. @@ -1104,8 +1107,14 @@ int ExtensionWebRequestEventRouter::OnBeforeRequest( *new_url = action.redirect_url.value(); return net::OK; case DNRRequestAction::Type::MODIFY_HEADERS: - // TODO(crbug.com/947591): Evaluate modify headers DNR actions. - NOTREACHED(); + // Unlike other actions, allow web request extensions to intercept the + // request here. The headers will be modified during subsequent request + // stages. + DCHECK(std::all_of(request->dnr_actions->begin(), + request->dnr_actions->end(), [](const auto& action) { + return action.type == + DNRRequestAction::Type::MODIFY_HEADERS; + })); break; } } @@ -1136,15 +1145,20 @@ int ExtensionWebRequestEventRouter::OnBeforeSendHeaders( if (ShouldHideEvent(browser_context, *request)) return net::OK; - // TODO(crbug.com/947591): Handle request header modification by the - // Declarative Net Request API. - bool initialize_blocked_requests = false; initialize_blocked_requests |= ProcessDeclarativeRules(browser_context, keys::kOnBeforeSendHeadersEvent, request, ON_BEFORE_SEND_HEADERS, nullptr); + DCHECK(request->dnr_actions); + initialize_blocked_requests |= std::any_of( + request->dnr_actions->begin(), request->dnr_actions->end(), + [](const DNRRequestAction& action) { + return action.type == DNRRequestAction::Type::MODIFY_HEADERS && + !action.request_headers_to_modify.empty(); + }); + int extra_info_spec = 0; RawListeners listeners = GetMatchingListeners(browser_context, keys::kOnBeforeSendHeadersEvent, @@ -1217,16 +1231,19 @@ int ExtensionWebRequestEventRouter::OnHeadersReceived( if (ShouldHideEvent(browser_context, *request)) return net::OK; - // TODO(crbug.com/947591): Handle header modification by the Declarative Net - // Request API. - scoped_refptr<const net::HttpResponseHeaders> filtered_response_headers = - original_response_headers; - bool initialize_blocked_requests = false; + DCHECK(request->dnr_actions); + initialize_blocked_requests |= std::any_of( + request->dnr_actions->begin(), request->dnr_actions->end(), + [](const DNRRequestAction& action) { + return action.type == DNRRequestAction::Type::MODIFY_HEADERS && + !action.response_headers_to_modify.empty(); + }); + initialize_blocked_requests |= ProcessDeclarativeRules( browser_context, keys::kOnHeadersReceivedEvent, request, - ON_HEADERS_RECEIVED, filtered_response_headers.get()); + ON_HEADERS_RECEIVED, original_response_headers); int extra_info_spec = 0; RawListeners listeners = @@ -1237,8 +1254,7 @@ int ExtensionWebRequestEventRouter::OnHeadersReceived( !GetAndSetSignaled(request->id, kOnHeadersReceived)) { std::unique_ptr<WebRequestEventDetails> event_details( CreateEventDetails(*request, extra_info_spec)); - event_details->SetResponseHeaders(*request, - filtered_response_headers.get()); + event_details->SetResponseHeaders(*request, original_response_headers); initialize_blocked_requests |= DispatchEvent( browser_context, request, listeners, std::move(event_details)); @@ -1258,7 +1274,7 @@ int ExtensionWebRequestEventRouter::OnHeadersReceived( blocked_request.request = request; blocked_request.callback = std::move(callback); blocked_request.override_response_headers = override_response_headers; - blocked_request.filtered_response_headers = filtered_response_headers; + blocked_request.original_response_headers = original_response_headers; blocked_request.new_url = preserve_fragment_on_redirect_url; if (blocked_request.num_handlers_blocking == 0) { @@ -2014,7 +2030,7 @@ helpers::EventResponseDelta CalculateDelta( } case ExtensionWebRequestEventRouter::kOnHeadersReceived: { const net::HttpResponseHeaders* old_headers = - blocked_request->filtered_response_headers.get(); + blocked_request->original_response_headers.get(); helpers::ResponseHeaders* new_headers = response->response_headers.get(); return helpers::CalculateOnHeadersReceivedDelta( @@ -2229,6 +2245,7 @@ int ExtensionWebRequestEventRouter::ExecuteDeltas( helpers::MergeCancelOfResponses(blocked_request.response_deltas, &canceled); extension_web_request_api_helpers::IgnoredActions ignored_actions; + std::vector<const DNRRequestAction*> matched_dnr_actions; if (blocked_request.event == kOnBeforeRequest) { CHECK(!blocked_request.callback.is_null()); helpers::MergeOnBeforeRequestResponses( @@ -2240,15 +2257,14 @@ int ExtensionWebRequestEventRouter::ExecuteDeltas( *request, blocked_request.response_deltas, blocked_request.request_headers, &ignored_actions, &request_headers_removed, &request_headers_set, - &request_headers_modified); - + &request_headers_modified, &matched_dnr_actions); } else if (blocked_request.event == kOnHeadersReceived) { CHECK(!blocked_request.callback.is_null()); helpers::MergeOnHeadersReceivedResponses( *request, blocked_request.response_deltas, - blocked_request.filtered_response_headers.get(), + blocked_request.original_response_headers.get(), blocked_request.override_response_headers, blocked_request.new_url, - &ignored_actions, &response_headers_modified); + &ignored_actions, &response_headers_modified, &matched_dnr_actions); } else if (blocked_request.event == kOnAuthRequired) { CHECK(blocked_request.callback.is_null()); CHECK(!blocked_request.auth_callback.is_null()); @@ -2266,6 +2282,9 @@ int ExtensionWebRequestEventRouter::ExecuteDeltas( std::move(ignored_actions)); } + for (const DNRRequestAction* action : matched_dnr_actions) + OnDNRActionMatched(browser_context, *request, *action); + const bool redirected = blocked_request.new_url && !blocked_request.new_url->is_empty(); @@ -2330,7 +2349,7 @@ bool ExtensionWebRequestEventRouter::ProcessDeclarativeRules( const std::string& event_name, const WebRequestInfo* request, RequestStage request_stage, - const net::HttpResponseHeaders* filtered_response_headers) { + const net::HttpResponseHeaders* original_response_headers) { int rules_registry_id = request->is_web_view ? request->web_view_rules_registry_id : RulesRegistryService::kDefaultRulesRegistryID; @@ -2384,7 +2403,7 @@ bool ExtensionWebRequestEventRouter::ProcessDeclarativeRules( blocked_request.request = request; blocked_request.is_incognito |= IsIncognitoBrowserContext(browser_context); blocked_request.blocking_time = base::Time::Now(); - blocked_request.filtered_response_headers = filtered_response_headers; + blocked_request.original_response_headers = original_response_headers; return true; } @@ -2393,7 +2412,7 @@ bool ExtensionWebRequestEventRouter::ProcessDeclarativeRules( WebRequestRulesRegistry* rules_registry = it.first; helpers::EventResponseDeltas result = rules_registry->CreateDeltas( PermissionHelper::Get(browser_context), - WebRequestData(request, request_stage, filtered_response_headers), + WebRequestData(request, request_stage, original_response_headers), it.second); if (!result.empty()) { @@ -2422,7 +2441,7 @@ void ExtensionWebRequestEventRouter::OnRulesRegistryReady( BlockedRequest& blocked_request = it->second; ProcessDeclarativeRules(browser_context, event_name, blocked_request.request, request_stage, - blocked_request.filtered_response_headers.get()); + blocked_request.original_response_headers.get()); DecrementBlockCount(browser_context, std::string(), event_name, request_id, nullptr, 0 /* extra_info_spec */); } diff --git a/chromium/extensions/browser/api/web_request/web_request_api_helpers.cc b/chromium/extensions/browser/api/web_request/web_request_api_helpers.cc index 2e5874d5ac8..58012fe6fe5 100644 --- a/chromium/extensions/browser/api/web_request/web_request_api_helpers.cc +++ b/chromium/extensions/browser/api/web_request/web_request_api_helpers.cc @@ -23,7 +23,6 @@ #include "base/strings/string_number_conversions.h" #include "base/strings/string_util.h" #include "base/strings/stringprintf.h" -#include "base/task/post_task.h" #include "base/time/time.h" #include "base/values.h" #include "components/web_cache/browser/web_cache_manager.h" @@ -31,6 +30,7 @@ #include "content/public/browser/browser_task_traits.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/render_process_host.h" +#include "extensions/browser/api/declarative_net_request/request_action.h" #include "extensions/browser/api/extensions_api_client.h" #include "extensions/browser/api/web_request/web_request_api_constants.h" #include "extensions/browser/api/web_request/web_request_info.h" @@ -38,6 +38,7 @@ #include "extensions/browser/extension_system.h" #include "extensions/browser/extensions_browser_client.h" #include "extensions/browser/runtime_data.h" +#include "extensions/common/api/declarative_net_request.h" #include "extensions/common/extension_messages.h" #include "net/cookies/cookie_util.h" #include "net/cookies/parsed_cookie.h" @@ -56,11 +57,13 @@ using net::cookie_util::ParsedRequestCookies; namespace keys = extension_web_request_api_constants; namespace web_request = extensions::api::web_request; +using DNRRequestAction = extensions::declarative_net_request::RequestAction; namespace extension_web_request_api_helpers { namespace { +namespace dnr_api = extensions::api::declarative_net_request; using ParsedResponseCookies = std::vector<std::unique_ptr<net::ParsedCookie>>; void ClearCacheOnNavigationOnUI() { @@ -329,20 +332,178 @@ static_assert(static_cast<size_t>(ResponseHeaderType::kMaxValue) - 1 == static_assert(ValidateHeaderEntries(kResponseHeaderEntries), "Invalid response header entries"); -bool HasMatchingRemovedDNRRequestHeader( - const extensions::WebRequestInfo& request, - const std::string& header) { - // TODO(crbug.com/947591): Reimplement this method with - // |action.request_headers_to_modify|. - return false; +// Represents an action to be taken on a given header. +struct DNRHeaderAction { + DNRHeaderAction(const DNRRequestAction::HeaderInfo* header_info, + const extensions::ExtensionId* extension_id) + : header_info(header_info), extension_id(extension_id) {} + + // Returns whether for the same header, the operation specified by + // |next_action| conflicts with the operation specified by this action. + bool ConflictsWithSubsequentAction(const DNRHeaderAction& next_action) const { + DCHECK_EQ(header_info->header, next_action.header_info->header); + + switch (header_info->operation) { + case dnr_api::HEADER_OPERATION_APPEND: + return next_action.header_info->operation != + dnr_api::HEADER_OPERATION_APPEND; + case dnr_api::HEADER_OPERATION_SET: + return *extension_id != *next_action.extension_id || + next_action.header_info->operation != + dnr_api::HEADER_OPERATION_APPEND; + case dnr_api::HEADER_OPERATION_REMOVE: + return true; + case dnr_api::HEADER_OPERATION_NONE: + NOTREACHED(); + return true; + } + } + + // Non-owning pointers to HeaderInfo and ExtensionId. + const DNRRequestAction::HeaderInfo* header_info; + const extensions::ExtensionId* extension_id; +}; + +// Helper to modify request headers from +// |request_action.request_headers_to_modify|. Returns whether or not request +// headers were actually modified and modifies |removed_headers|, |set_headers| +// and |header_actions|. |header_actions| maps a header name to the operation +// to be performed on the header. +bool ModifyRequestHeadersForAction( + net::HttpRequestHeaders* headers, + const DNRRequestAction& request_action, + std::set<std::string>* removed_headers, + std::set<std::string>* set_headers, + std::map<base::StringPiece, DNRHeaderAction>* header_actions) { + bool request_headers_modified = false; + for (const DNRRequestAction::HeaderInfo& header_info : + request_action.request_headers_to_modify) { + bool header_modified = false; + const std::string& header = header_info.header; + + DNRHeaderAction header_action(&header_info, &request_action.extension_id); + auto iter = header_actions->find(header); + if (iter != header_actions->end() && + iter->second.ConflictsWithSubsequentAction(header_action)) { + continue; + } + header_actions->emplace(header, header_action); + + // TODO(crbug.com/1088103): Record request headers modified by the + // Declarative Net Request API. + switch (header_info.operation) { + case extensions::api::declarative_net_request::HEADER_OPERATION_SET: + headers->SetHeader(header, *header_info.value); + header_modified = true; + set_headers->insert(header); + break; + case extensions::api::declarative_net_request::HEADER_OPERATION_REMOVE: { + while (headers->HasHeader(header)) { + header_modified = true; + headers->RemoveHeader(header); + } + + if (header_modified) + removed_headers->insert(header); + break; + } + case extensions::api::declarative_net_request::HEADER_OPERATION_APPEND: + case extensions::api::declarative_net_request::HEADER_OPERATION_NONE: + NOTREACHED(); + } + + request_headers_modified |= header_modified; + } + + return request_headers_modified; } -bool HasMatchingRemovedDNRResponseHeader( - const extensions::WebRequestInfo& request, - const std::string& header) { - // TODO(crbug.com/947591): Reimplement this method with - // |action.response_headers_to_modify|. - return false; +// Helper to modify response headers from |request_action|. Returns whether or +// not response headers were actually modified and modifies |header_actions|. +// |header_actions| maps a header name to a list of operations to be performed +// on the header. +bool ModifyResponseHeadersForAction( + const net::HttpResponseHeaders* original_response_headers, + scoped_refptr<net::HttpResponseHeaders>* override_response_headers, + const DNRRequestAction& request_action, + std::map<base::StringPiece, std::vector<DNRHeaderAction>>* header_actions) { + bool response_headers_modified = false; + + // Check for |header| in |override_response_headers| if headers have been + // modified, otherwise, check in |original_response_headers|. + auto has_header = [&original_response_headers, + &override_response_headers](std::string header) { + return override_response_headers->get() + ? override_response_headers->get()->HasHeader(header) + : original_response_headers->HasHeader(header); + }; + + // Create a copy of |original_response_headers| iff we really want to modify + // the response headers. + auto create_override_headers_if_needed = + [&original_response_headers]( + scoped_refptr<net::HttpResponseHeaders>* override_response_headers) { + if (override_response_headers->get() == nullptr) { + *override_response_headers = + base::MakeRefCounted<net::HttpResponseHeaders>( + original_response_headers->raw_headers()); + } + }; + + for (const DNRRequestAction::HeaderInfo& header_info : + request_action.response_headers_to_modify) { + bool header_modified = false; + const std::string& header = header_info.header; + + DNRHeaderAction header_action(&header_info, &request_action.extension_id); + auto iter = header_actions->find(header); + + // Checking the first DNRHeaderAction should suffice for determining if a + // conflict exists, since the contents of |header_actions| for a given + // header will always be one of: + // [remove] + // [append+] one or more appends + // [set, append*] set, any number of appends from the same extension + if (iter != header_actions->end() && + (*header_actions)[header][0].ConflictsWithSubsequentAction( + header_action)) { + continue; + } + (*header_actions)[header].push_back(header_action); + + // TODO(crbug.com/1088103): Record response headers modified by the + // Declarative Net Request API. + switch (header_info.operation) { + case extensions::api::declarative_net_request::HEADER_OPERATION_REMOVE: { + if (has_header(header)) { + header_modified = true; + create_override_headers_if_needed(override_response_headers); + override_response_headers->get()->RemoveHeader(header); + } + + break; + } + case extensions::api::declarative_net_request::HEADER_OPERATION_APPEND: { + header_modified = true; + create_override_headers_if_needed(override_response_headers); + override_response_headers->get()->AddHeader(header, *header_info.value); + break; + } + case extensions::api::declarative_net_request::HEADER_OPERATION_SET: { + header_modified = true; + create_override_headers_if_needed(override_response_headers); + override_response_headers->get()->RemoveHeader(header); + override_response_headers->get()->AddHeader(header, *header_info.value); + break; + } + case extensions::api::declarative_net_request::HEADER_OPERATION_NONE: + NOTREACHED(); + } + + response_headers_modified |= header_modified; + } + + return response_headers_modified; } } // namespace @@ -920,16 +1081,36 @@ void MergeOnBeforeSendHeadersResponses( IgnoredActions* ignored_actions, std::set<std::string>* removed_headers, std::set<std::string>* set_headers, - bool* request_headers_modified) { + bool* request_headers_modified, + std::vector<const DNRRequestAction*>* matched_dnr_actions) { DCHECK(request_headers_modified); DCHECK(removed_headers->empty()); DCHECK(set_headers->empty()); + DCHECK(request.dnr_actions); + DCHECK(matched_dnr_actions); *request_headers_modified = false; - // Exhaustive subsets of |set_headers|. Split into a set for added headers and - // a set for overridden headers. - std::set<std::string> overridden_headers; - std::set<std::string> added_headers; + std::map<base::StringPiece, DNRHeaderAction> dnr_header_actions; + for (const auto& action : *request.dnr_actions) { + bool headers_modified_for_action = + ModifyRequestHeadersForAction(request_headers, action, removed_headers, + set_headers, &dnr_header_actions); + + *request_headers_modified |= headers_modified_for_action; + if (headers_modified_for_action) + matched_dnr_actions->push_back(&action); + } + + // A strict subset of |removed_headers| consisting of headers removed by the + // web request API. Used for metrics. + // TODO(crbug.com/1098945): Use base::StringPiece to avoid copying header + // names. + std::set<std::string> web_request_removed_headers; + + // Subsets of |set_headers| consisting of headers modified by the web request + // API. Split into a set for added headers and a set for overridden headers. + std::set<std::string> web_request_overridden_headers; + std::set<std::string> web_request_added_headers; // We assume here that the deltas are sorted in decreasing extension // precedence (i.e. decreasing extension installation time). @@ -951,16 +1132,20 @@ void MergeOnBeforeSendHeadersResponses( const std::string key = base::ToLowerASCII(modification.name()); const std::string& value = modification.value(); - // We must not modify anything that has been deleted before. - if (base::Contains(*removed_headers, key)) { + // We must not modify anything that was specified to be removed by the + // Declarative Net Request API. Note that the actual header + // modifications made by Declarative Net Request should be represented + // in |removed_headers| and |set_headers|. + auto iter = dnr_header_actions.find(key); + if (iter != dnr_header_actions.end() && + iter->second.header_info->operation == + dnr_api::HEADER_OPERATION_REMOVE) { extension_conflicts = true; break; } - // Prevent extensions from adding any header removed by the Declarative - // Net Request API. - DCHECK(request.dnr_actions); - if (HasMatchingRemovedDNRRequestHeader(request, key)) { + // We must not modify anything that has been deleted before. + if (base::Contains(*removed_headers, key)) { extension_conflicts = true; break; } @@ -998,11 +1183,11 @@ void MergeOnBeforeSendHeadersResponses( while (modification.GetNext()) { std::string key = base::ToLowerASCII(modification.name()); if (!request_headers->HasHeader(key)) { - added_headers.insert(key); - } else if (!base::Contains(added_headers, key)) { + web_request_added_headers.insert(key); + } else if (!base::Contains(web_request_added_headers, key)) { // Note: |key| will only be present in |added_headers| if this is an // identical edit. - overridden_headers.insert(key); + web_request_overridden_headers.insert(key); } set_headers->insert(key); @@ -1013,8 +1198,11 @@ void MergeOnBeforeSendHeadersResponses( // Perform all deletions and record which keys were deleted. { for (const auto& header : delta.deleted_request_headers) { + std::string lowercase_header = base::ToLowerASCII(header); + request_headers->RemoveHeader(header); - removed_headers->insert(base::ToLowerASCII(header)); + removed_headers->insert(lowercase_header); + web_request_removed_headers.insert(lowercase_header); } } *request_headers_modified = true; @@ -1039,23 +1227,24 @@ void MergeOnBeforeSendHeadersResponses( IsStringLowerCaseASCII)); DCHECK(std::all_of(set_headers->begin(), set_headers->end(), IsStringLowerCaseASCII)); - DCHECK(std::all_of(overridden_headers.begin(), overridden_headers.end(), - IsStringLowerCaseASCII)); - DCHECK(std::all_of(added_headers.begin(), added_headers.end(), - IsStringLowerCaseASCII)); - DCHECK(*set_headers == base::STLSetUnion<std::set<std::string>>( - added_headers, overridden_headers)); - DCHECK(base::STLSetIntersection<std::set<std::string>>(added_headers, - overridden_headers) + DCHECK(base::STLIncludes( + *set_headers, + base::STLSetUnion<std::set<std::string>>( + web_request_added_headers, web_request_overridden_headers))); + DCHECK(base::STLSetIntersection<std::set<std::string>>( + web_request_added_headers, web_request_overridden_headers) .empty()); DCHECK(base::STLSetIntersection<std::set<std::string>>(*removed_headers, *set_headers) .empty()); + DCHECK(base::STLIncludes(*removed_headers, web_request_removed_headers)); // Record request header removals, additions and modifications. - record_request_headers(*removed_headers, &RecordRequestHeaderRemoved); - record_request_headers(added_headers, &RecordRequestHeaderAdded); - record_request_headers(overridden_headers, &RecordRequestHeaderChanged); + record_request_headers(web_request_removed_headers, + &RecordRequestHeaderRemoved); + record_request_headers(web_request_added_headers, &RecordRequestHeaderAdded); + record_request_headers(web_request_overridden_headers, + &RecordRequestHeaderChanged); // Currently, conflicts are ignored while merging cookies. MergeCookiesInOnBeforeSendHeadersResponses(request.url, deltas, @@ -1290,10 +1479,25 @@ void MergeOnHeadersReceivedResponses( scoped_refptr<net::HttpResponseHeaders>* override_response_headers, GURL* preserve_fragment_on_redirect_url, IgnoredActions* ignored_actions, - bool* response_headers_modified) { + bool* response_headers_modified, + std::vector<const DNRRequestAction*>* matched_dnr_actions) { DCHECK(response_headers_modified); *response_headers_modified = false; + DCHECK(request.dnr_actions); + DCHECK(matched_dnr_actions); + + std::map<base::StringPiece, std::vector<DNRHeaderAction>> dnr_header_actions; + for (const auto& action : *request.dnr_actions) { + bool headers_modified_for_action = ModifyResponseHeadersForAction( + original_response_headers, override_response_headers, action, + &dnr_header_actions); + + *response_headers_modified |= headers_modified_for_action; + if (headers_modified_for_action) + matched_dnr_actions->push_back(&action); + } + // Here we collect which headers we have removed or added so far due to // extensions of higher precedence. Header keys are always stored as // lower case. @@ -1322,18 +1526,28 @@ void MergeOnHeadersReceivedResponses( // this takes care of precedence. bool extension_conflicts = false; for (const ResponseHeader& header : delta.deleted_response_headers) { - if (removed_headers.find(ToLowerCase(header)) != removed_headers.end()) { + ResponseHeader lowercase_header(ToLowerCase(header)); + if (base::Contains(removed_headers, lowercase_header) || + base::Contains(dnr_header_actions, lowercase_header.first)) { extension_conflicts = true; break; } } - // Prevent extensions from adding any response header which was removed by - // the Declarative Net Request API. - DCHECK(request.dnr_actions); + // Prevent extensions from adding any response header which was specified to + // be removed or set by the Declarative Net Request API. However, multiple + // appends are allowed. if (!extension_conflicts) { for (const ResponseHeader& header : delta.added_response_headers) { - if (HasMatchingRemovedDNRResponseHeader(request, header.first)) { + ResponseHeader lowercase_header(ToLowerCase(header)); + + auto it = dnr_header_actions.find(lowercase_header.first); + if (it == dnr_header_actions.end()) + continue; + + // Multiple appends are allowed. + if (it->second[0].header_info->operation != + dnr_api::HEADER_OPERATION_APPEND) { extension_conflicts = true; break; } @@ -1406,6 +1620,7 @@ void MergeOnHeadersReceivedResponses( std::set<base::StringPiece> modified_header_names; std::set<base::StringPiece> added_header_names; std::set<base::StringPiece> removed_header_names; + for (const ResponseHeader& header : added_headers) { // Skip logging this header if this was subsequently removed by an // extension. @@ -1467,8 +1682,8 @@ void ClearCacheOnNavigation() { if (content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) { ClearCacheOnNavigationOnUI(); } else { - base::PostTask(FROM_HERE, {content::BrowserThread::UI}, - base::BindOnce(&ClearCacheOnNavigationOnUI)); + content::GetUIThreadTaskRunner({})->PostTask( + FROM_HERE, base::BindOnce(&ClearCacheOnNavigationOnUI)); } } diff --git a/chromium/extensions/browser/api/web_request/web_request_api_helpers.h b/chromium/extensions/browser/api/web_request/web_request_api_helpers.h index a90a34b999b..cd7d01a62cc 100644 --- a/chromium/extensions/browser/api/web_request/web_request_api_helpers.h +++ b/chromium/extensions/browser/api/web_request/web_request_api_helpers.h @@ -38,6 +38,11 @@ class BrowserContext; namespace extensions { class Extension; struct WebRequestInfo; + +namespace declarative_net_request { +struct RequestAction; +} // namespace declarative_net_request + } // namespace extensions namespace extension_web_request_api_helpers { @@ -435,6 +440,8 @@ void MergeCookiesInOnBeforeSendHeadersResponses( // are tried to be resolved. // Stores in |request_headers_modified| whether the request headers were // modified. +// Any actions within |request.dnr_actions| which result in headers being +// modified are added to |matched_dnr_actions|. void MergeOnBeforeSendHeadersResponses( const extensions::WebRequestInfo& request, const EventResponseDeltas& deltas, @@ -442,7 +449,9 @@ void MergeOnBeforeSendHeadersResponses( IgnoredActions* ignored_actions, std::set<std::string>* removed_headers, std::set<std::string>* set_headers, - bool* request_headers_modified); + bool* request_headers_modified, + std::vector<const extensions::declarative_net_request::RequestAction*>* + matched_dnr_actions); // Modifies the "Set-Cookie" headers in |override_response_headers| according to // |deltas.response_cookie_modifications|. If |override_response_headers| is // NULL, a copy of |original_response_headers| is created. Conflicts are @@ -460,6 +469,8 @@ void MergeCookiesInOnHeadersReceivedResponses( // sure that the URL provided by the extension isn't modified by having its // fragment overwritten by that of the original URL). Stores in // |response_headers_modified| whether the response headers were modified. +// Any actions within |request.dnr_actions| which result in headers being +// modified are added to |matched_dnr_actions|. void MergeOnHeadersReceivedResponses( const extensions::WebRequestInfo& request, const EventResponseDeltas& deltas, @@ -467,7 +478,9 @@ void MergeOnHeadersReceivedResponses( scoped_refptr<net::HttpResponseHeaders>* override_response_headers, GURL* preserve_fragment_on_redirect_url, IgnoredActions* ignored_actions, - bool* response_headers_modified); + bool* response_headers_modified, + std::vector<const extensions::declarative_net_request::RequestAction*>* + matched_dnr_actions); // Merge the responses of blocked onAuthRequired handlers. The first // registered listener that supplies authentication credentials in a response, // if any, will have its authentication credentials used. |request| must be diff --git a/chromium/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc b/chromium/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc index dbb25b166a8..e26a21f26b6 100644 --- a/chromium/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc +++ b/chromium/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc @@ -10,6 +10,7 @@ #include "base/bind.h" #include "base/callback_helpers.h" #include "base/feature_list.h" +#include "base/metrics/histogram_macros.h" #include "base/strings/stringprintf.h" #include "base/task/post_task.h" #include "components/keyed_service/content/browser_context_keyed_service_shutdown_notifier_factory.h" @@ -21,12 +22,18 @@ #include "extensions/browser/api/web_request/permission_helper.h" #include "extensions/browser/extension_navigation_ui_data.h" #include "extensions/browser/extension_registry.h" +#include "extensions/common/extension_features.h" #include "extensions/common/manifest_handlers/web_accessible_resources_info.h" #include "net/base/completion_repeating_callback.h" +#include "net/http/http_response_headers.h" +#include "net/http/http_status_code.h" #include "net/http/http_util.h" #include "net/url_request/redirect_info.h" #include "net/url_request/redirect_util.h" #include "net/url_request/url_request.h" +#include "services/metrics/public/cpp/ukm_builders.h" +#include "services/metrics/public/cpp/ukm_recorder.h" +#include "services/metrics/public/cpp/ukm_source_id.h" #include "services/network/public/cpp/features.h" #include "third_party/blink/public/common/loader/throttling_url_loader.h" #include "third_party/blink/public/platform/resource_request_blocked_reason.h" @@ -91,6 +98,7 @@ WebRequestProxyingURLLoaderFactory::InProgressRequest::InProgressRequest( int32_t network_service_request_id, int32_t routing_id, uint32_t options, + ukm::SourceId ukm_source_id, const network::ResourceRequest& request, const net::MutableNetworkTrafficAnnotationTag& traffic_annotation, mojo::PendingReceiver<network::mojom::URLLoader> loader_receiver, @@ -102,6 +110,7 @@ WebRequestProxyingURLLoaderFactory::InProgressRequest::InProgressRequest( network_service_request_id_(network_service_request_id), routing_id_(routing_id), options_(options), + ukm_source_id_(ukm_source_id), traffic_annotation_(traffic_annotation), proxied_loader_receiver_(this, std::move(loader_receiver)), target_client_(std::move(client)), @@ -111,10 +120,10 @@ WebRequestProxyingURLLoaderFactory::InProgressRequest::InProgressRequest( ExtensionWebRequestEventRouter::GetInstance() ->HasAnyExtraHeadersListener(factory_->browser_context_)) { // If there is a client error, clean up the request. - target_client_.set_disconnect_handler(base::BindOnce( - &WebRequestProxyingURLLoaderFactory::InProgressRequest::OnRequestError, - weak_factory_.GetWeakPtr(), - network::URLLoaderCompletionStatus(net::ERR_ABORTED))); + target_client_.set_disconnect_handler( + base::BindOnce(&WebRequestProxyingURLLoaderFactory::InProgressRequest:: + OnClientDisconnected, + weak_factory_.GetWeakPtr())); proxied_loader_receiver_.set_disconnect_with_reason_handler( base::BindOnce(&WebRequestProxyingURLLoaderFactory::InProgressRequest:: OnLoaderDisconnected, @@ -129,6 +138,7 @@ WebRequestProxyingURLLoaderFactory::InProgressRequest::InProgressRequest( request_(request), original_initiator_(request.request_initiator), request_id_(request_id), + ukm_source_id_(ukm::kInvalidSourceId), proxied_loader_receiver_(this), for_cors_preflight_(true), has_any_extra_headers_listeners_( @@ -136,6 +146,18 @@ WebRequestProxyingURLLoaderFactory::InProgressRequest::InProgressRequest( ->HasAnyExtraHeadersListener(factory_->browser_context_)) {} WebRequestProxyingURLLoaderFactory::InProgressRequest::~InProgressRequest() { + DCHECK_NE(state_, State::kInvalid); + if (request_.keepalive && !for_cors_preflight_) { + UMA_HISTOGRAM_ENUMERATION("Extensions.WebRequest.KeepaliveRequestState", + state_); + if (base::FeatureList::IsEnabled( + extensions_features::kReportKeepaliveUkm)) { + ukm::builders::Extensions_WebRequest_KeepaliveRequestFinished( + ukm_source_id_) + .SetState(state_) + .Record(ukm::UkmRecorder::Get()); + } + } // This is important to ensure that no outstanding blocking requests continue // to reference state owned by this object. if (info_) { @@ -186,20 +208,23 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: void WebRequestProxyingURLLoaderFactory::InProgressRequest::RestartInternal() { DCHECK_EQ(info_->url, request_.url) << "UpdateRequestInfo must have been called first"; + is_header_client_receiver_paused_ = false; // If the header client will be used, we start the request immediately, and // OnBeforeSendHeaders and OnSendHeaders will be handled there. Otherwise, // send these events before the request starts. base::RepeatingCallback<void(int)> continuation; + const auto state_on_error = State::kRejectedByOnBeforeRequest; if (current_request_uses_header_client_) { - continuation = base::BindRepeating( - &InProgressRequest::ContinueToStartRequest, weak_factory_.GetWeakPtr()); + continuation = + base::BindRepeating(&InProgressRequest::ContinueToStartRequest, + weak_factory_.GetWeakPtr(), state_on_error); } else if (for_cors_preflight_) { // In this case we do nothing because extensions should see nothing. return; } else { continuation = base::BindRepeating(&InProgressRequest::ContinueToBeforeSendHeaders, - weak_factory_.GetWeakPtr()); + weak_factory_.GetWeakPtr(), state_on_error); } redirect_url_ = GURL(); bool should_collapse_initiator = false; @@ -214,7 +239,7 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest::RestartInternal() { status.extended_error_code = static_cast<int>( blink::ResourceRequestBlockedReason::kCollapsedByClient); } - OnRequestError(status); + OnRequestError(status, state_on_error); return; } @@ -229,8 +254,10 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest::RestartInternal() { // Pause the header client, since we want to wait until OnBeforeRequest has // finished before processing any future events. - if (header_client_receiver_.is_bound()) + if (header_client_receiver_.is_bound()) { header_client_receiver_.Pause(); + is_header_client_receiver_paused_ = true; + } return; } DCHECK_EQ(net::OK, result); @@ -318,7 +345,7 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest::OnReceiveRedirect( if (redirect_url_ != redirect_info.new_url && !IsRedirectSafe(request_.url, redirect_info.new_url, info_->is_navigation_request)) { - OnRequestError( + OnNetworkError( network::URLLoaderCompletionStatus(net::ERR_UNSAFE_REDIRECT)); return; } @@ -368,10 +395,11 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: void WebRequestProxyingURLLoaderFactory::InProgressRequest::OnComplete( const network::URLLoaderCompletionStatus& status) { if (status.error_code != net::OK) { - OnRequestError(status); + OnNetworkError(status); return; } + state_ = kCompleted; target_client_->OnComplete(status); ExtensionWebRequestEventRouter::GetInstance()->OnCompleted( factory_->browser_context_, &info_.value(), status.error_code); @@ -422,12 +450,14 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest::OnLoaderCreated( header_client_receiver_.reset(); header_client_receiver_.Bind(std::move(receiver)); + if (is_header_client_receiver_paused_) + header_client_receiver_.Pause(); if (for_cors_preflight_) { // In this case we don't have |target_loader_| and // |proxied_client_receiver_|, and |receiver| is the only connection to the // network service, so we observe mojo connection errors. header_client_receiver_.set_disconnect_handler(base::BindOnce( - &WebRequestProxyingURLLoaderFactory::InProgressRequest::OnRequestError, + &WebRequestProxyingURLLoaderFactory::InProgressRequest::OnNetworkError, weak_factory_.GetWeakPtr(), network::URLLoaderCompletionStatus(net::ERR_FAILED))); } @@ -443,7 +473,7 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest::OnBeforeSendHeaders( request_.headers = headers; on_before_send_headers_callback_ = std::move(callback); - ContinueToBeforeSendHeaders(net::OK); + ContinueToBeforeSendHeadersWithOk(); } void WebRequestProxyingURLLoaderFactory::InProgressRequest::OnHeadersReceived( @@ -539,9 +569,10 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: } void WebRequestProxyingURLLoaderFactory::InProgressRequest:: - ContinueToBeforeSendHeaders(int error_code) { + ContinueToBeforeSendHeaders(State state_on_error, int error_code) { if (error_code != net::OK) { - OnRequestError(network::URLLoaderCompletionStatus(error_code)); + OnRequestError(network::URLLoaderCompletionStatus(error_code), + state_on_error); return; } @@ -558,8 +589,10 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: // intuitive), |onBeforeSendHeaders| is only dispatched for HTTP and HTTPS // requests. - auto continuation = base::BindRepeating( - &InProgressRequest::ContinueToSendHeaders, weak_factory_.GetWeakPtr()); + const auto state_on_error = State::kRejectedByOnBeforeSendHeaders; + auto continuation = + base::BindRepeating(&InProgressRequest::ContinueToSendHeaders, + weak_factory_.GetWeakPtr(), state_on_error); int result = ExtensionWebRequestEventRouter::GetInstance()->OnBeforeSendHeaders( factory_->browser_context_, &info_.value(), continuation, @@ -568,7 +601,8 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: if (result == net::ERR_BLOCKED_BY_CLIENT) { // The request was cancelled synchronously. Dispatch an error notification // and terminate the request. - OnRequestError(network::URLLoaderCompletionStatus(result)); + OnRequestError(network::URLLoaderCompletionStatus(result), + state_on_error); return; } @@ -585,21 +619,26 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: DCHECK_EQ(net::OK, result); } - ContinueToSendHeaders(std::set<std::string>(), std::set<std::string>(), - net::OK); + ContinueToSendHeadersWithOk(std::set<std::string>(), std::set<std::string>()); } void WebRequestProxyingURLLoaderFactory::InProgressRequest:: - ContinueToStartRequest(int error_code) { + ContinueToBeforeSendHeadersWithOk() { + ContinueToBeforeSendHeaders(State::kInvalid, net::OK); +} +void WebRequestProxyingURLLoaderFactory::InProgressRequest:: + ContinueToStartRequest(State state_on_error, int error_code) { if (error_code != net::OK) { - OnRequestError(network::URLLoaderCompletionStatus(error_code)); + OnRequestError(network::URLLoaderCompletionStatus(error_code), + state_on_error); return; } if (current_request_uses_header_client_ && !redirect_url_.is_empty()) { if (for_cors_preflight_) { // CORS preflight doesn't support redirect. - OnRequestError(network::URLLoaderCompletionStatus(net::ERR_FAILED)); + OnRequestError(network::URLLoaderCompletionStatus(net::ERR_FAILED), + state_on_error); return; } HandleBeforeRequestRedirect(); @@ -609,8 +648,10 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: if (proxied_client_receiver_.is_bound()) proxied_client_receiver_.Resume(); - if (header_client_receiver_.is_bound()) + if (header_client_receiver_.is_bound()) { header_client_receiver_.Resume(); + is_header_client_receiver_paused_ = false; + } if (for_cors_preflight_) { // For CORS preflight requests, we have already started the request in @@ -640,11 +681,18 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: } void WebRequestProxyingURLLoaderFactory::InProgressRequest:: - ContinueToSendHeaders(const std::set<std::string>& removed_headers, + ContinueToStartRequestWithOk() { + ContinueToStartRequest(State::kInvalid, net::OK); +} + +void WebRequestProxyingURLLoaderFactory::InProgressRequest:: + ContinueToSendHeaders(State state_on_error, + const std::set<std::string>& removed_headers, const std::set<std::string>& set_headers, int error_code) { if (error_code != net::OK) { - OnRequestError(network::URLLoaderCompletionStatus(error_code)); + OnRequestError(network::URLLoaderCompletionStatus(error_code), + state_on_error); return; } @@ -690,7 +738,13 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: } if (!current_request_uses_header_client_) - ContinueToStartRequest(net::OK); + ContinueToStartRequestWithOk(); +} + +void WebRequestProxyingURLLoaderFactory::InProgressRequest:: + ContinueToSendHeadersWithOk(const std::set<std::string>& removed_headers, + const std::set<std::string>& set_headers) { + ContinueToSendHeaders(State::kInvalid, removed_headers, set_headers, net::OK); } void WebRequestProxyingURLLoaderFactory::InProgressRequest::ContinueAuthRequest( @@ -698,6 +752,8 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest::ContinueAuthRequest( WebRequestAPI::AuthRequestCallback callback, int error_code) { if (error_code != net::OK) { + // Here we come from an onHeaderReceived failure. + state_ = State::kRejectedByOnHeadersReceivedForAuth; base::SequencedTaskRunnerHandle::Get()->PostTask( FROM_HERE, base::BindOnce(std::move(callback), base::nullopt, true /* should_cancel */)); @@ -756,6 +812,7 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: AUTH_REQUIRED_RESPONSE_CANCEL_AUTH: completion = base::BindOnce(std::move(callback), base::nullopt, true /* should_cancel */); + state_ = State::kRejectedByOnAuthRequired; break; default: NOTREACHED(); @@ -770,7 +827,18 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: void WebRequestProxyingURLLoaderFactory::InProgressRequest:: ContinueToHandleOverrideHeaders(int error_code) { if (error_code != net::OK) { - OnRequestError(network::URLLoaderCompletionStatus(error_code)); + const int status_code = current_response_->headers + ? current_response_->headers->response_code() + : 0; + State state; + if (status_code == net::HTTP_UNAUTHORIZED) { + state = State::kRejectedByOnHeadersReceivedForAuth; + } else if (net::HttpResponseHeaders::IsRedirectResponseCode(status_code)) { + state = State::kRejectedByOnHeadersReceivedForRedirect; + } else { + state = State::kRejectedByOnHeadersReceivedForFinalResponse; + } + OnRequestError(network::URLLoaderCompletionStatus(error_code), state); return; } @@ -787,7 +855,8 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: } if (for_cors_preflight_ && !redirect_url_.is_empty()) { - OnRequestError(network::URLLoaderCompletionStatus(net::ERR_FAILED)); + OnRequestError(network::URLLoaderCompletionStatus(net::ERR_FAILED), + State::kRejectedByOnHeadersReceivedForRedirect); return; } @@ -816,10 +885,15 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: ContinueToResponseStarted(int error_code) { DCHECK(!for_cors_preflight_); if (error_code != net::OK) { - OnRequestError(network::URLLoaderCompletionStatus(error_code)); + OnRequestError(network::URLLoaderCompletionStatus(error_code), + State::kRejectedByOnHeadersReceivedForFinalResponse); return; } + if (state_ == State::kInProgress) { + state_ = State::kInProgressWithFinalResponseReceived; + } + DCHECK(!current_request_uses_header_client_ || !override_headers_); if (override_headers_) current_response_->headers = override_headers_; @@ -863,7 +937,8 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: ContinueToBeforeRedirect(const net::RedirectInfo& redirect_info, int error_code) { if (error_code != net::OK) { - OnRequestError(network::URLLoaderCompletionStatus(error_code)); + OnRequestError(network::URLLoaderCompletionStatus(error_code), + kRejectedByOnHeadersReceivedForRedirect); return; } @@ -907,7 +982,19 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: current_response_->headers.get(), &override_headers_, &redirect_url_); if (result == net::ERR_BLOCKED_BY_CLIENT) { - OnRequestError(network::URLLoaderCompletionStatus(result)); + const int status_code = current_response_->headers + ? current_response_->headers->response_code() + : 0; + State state; + if (status_code == net::HTTP_UNAUTHORIZED) { + state = State::kRejectedByOnHeadersReceivedForAuth; + } else if (net::HttpResponseHeaders::IsRedirectResponseCode( + status_code)) { + state = State::kRejectedByOnHeadersReceivedForRedirect; + } else { + state = State::kRejectedByOnHeadersReceivedForFinalResponse; + } + OnRequestError(network::URLLoaderCompletionStatus(result), state); return; } @@ -930,17 +1017,41 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: copyable_callback.Run(net::OK); } void WebRequestProxyingURLLoaderFactory::InProgressRequest::OnRequestError( - const network::URLLoaderCompletionStatus& status) { + const network::URLLoaderCompletionStatus& status, + State state) { if (target_client_) target_client_->OnComplete(status); ExtensionWebRequestEventRouter::GetInstance()->OnErrorOccurred( factory_->browser_context_, &info_.value(), true /* started */, status.error_code); + state_ = state; // Deletes |this|. factory_->RemoveRequest(network_service_request_id_, request_id_); } +void WebRequestProxyingURLLoaderFactory::InProgressRequest::OnNetworkError( + const network::URLLoaderCompletionStatus& status) { + State state = state_; + if (state_ == State::kInProgress) { + state = State::kRejectedByNetworkError; + } else if (state_ == State::kInProgressWithFinalResponseReceived) { + state = State::kRejectedByNetworkErrorAfterReceivingFinalResponse; + } + OnRequestError(status, state); +} + +void WebRequestProxyingURLLoaderFactory::InProgressRequest:: + OnClientDisconnected() { + State state = state_; + if (state_ == State::kInProgress) { + state = State::kDetachedFromClient; + } else if (state_ == State::kInProgressWithFinalResponseReceived) { + state = State::kDetachedFromClientAfterReceivingResponse; + } + OnRequestError(network::URLLoaderCompletionStatus(net::ERR_ABORTED), state); +} + void WebRequestProxyingURLLoaderFactory::InProgressRequest:: OnLoaderDisconnected(uint32_t custom_reason, const std::string& description) { @@ -953,10 +1064,11 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest:: factory_->request_id_generator_->SaveID( routing_id_, network_service_request_id_, request_id_); + state_ = State::kRedirectFollowedByAnotherInProgressRequest; // Deletes |this|. factory_->RemoveRequest(network_service_request_id_, request_id_); } else { - OnRequestError(network::URLLoaderCompletionStatus(net::ERR_ABORTED)); + OnNetworkError(network::URLLoaderCompletionStatus(net::ERR_ABORTED)); } } @@ -991,17 +1103,20 @@ WebRequestProxyingURLLoaderFactory::WebRequestProxyingURLLoaderFactory( mojo::PendingReceiver<network::mojom::TrustedURLLoaderHeaderClient> header_client_receiver, WebRequestAPI::ProxySet* proxies, - content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type) + content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type, + ukm::SourceId ukm_source_id) : browser_context_(browser_context), render_process_id_(render_process_id), request_id_generator_(request_id_generator), navigation_ui_data_(std::move(navigation_ui_data)), navigation_id_(std::move(navigation_id)), proxies_(proxies), - loader_factory_type_(loader_factory_type) { + loader_factory_type_(loader_factory_type), + ukm_source_id_(ukm_source_id) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); - // base::Unretained is safe here because the callback will be canceled when - // |shutdown_notifier_| is destroyed, and |proxies_| owns this. + // base::Unretained is safe here because the callback will be + // canceled when |shutdown_notifier_| is destroyed, and |proxies_| + // owns this. shutdown_notifier_ = ShutdownNotifierFactory::GetInstance() ->Get(browser_context) @@ -1032,14 +1147,16 @@ void WebRequestProxyingURLLoaderFactory::StartProxying( mojo::PendingReceiver<network::mojom::TrustedURLLoaderHeaderClient> header_client_receiver, WebRequestAPI::ProxySet* proxies, - content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type) { + content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type, + ukm::SourceId ukm_source_id) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); auto proxy = std::make_unique<WebRequestProxyingURLLoaderFactory>( browser_context, render_process_id, request_id_generator, std::move(navigation_ui_data), std::move(navigation_id), std::move(loader_receiver), std::move(target_factory_remote), - std::move(header_client_receiver), proxies, loader_factory_type); + std::move(header_client_receiver), proxies, loader_factory_type, + ukm_source_id); proxies->AddProxy(std::move(proxy)); } @@ -1054,35 +1171,36 @@ void WebRequestProxyingURLLoaderFactory::CreateLoaderAndStart( const net::MutableNetworkTrafficAnnotationTag& traffic_annotation) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); - // Make sure we are not proxying a browser initiated non-navigation request - // except for loading service worker scripts. + // Make sure we are not proxying a browser initiated non-navigation + // request except for loading service worker scripts. DCHECK(render_process_id_ != -1 || navigation_ui_data_ || IsForServiceWorkerScript()); - // The |web_request_id| doesn't really matter. It just needs to be unique - // per-BrowserContext so extensions can make sense of it. Note that - // |network_service_request_id_| by contrast is not necessarily unique, so we - // don't use it for identity here. This request ID may be the same as a - // previous request if the previous request was redirected to a URL that - // required a different loader. + // The |web_request_id| doesn't really matter. It just needs to be + // unique per-BrowserContext so extensions can make sense of it. + // Note that |network_service_request_id_| by contrast is not + // necessarily unique, so we don't use it for identity here. This + // request ID may be the same as a previous request if the previous + // request was redirected to a URL that required a different loader. const uint64_t web_request_id = request_id_generator_->Generate(routing_id, request_id); if (request_id) { - // Only requests with a non-zero request ID can have their proxy associated - // with said ID. This is necessary to support correlation against any auth - // events received by the browser. Requests with a request ID of 0 therefore - // do not support dispatching |WebRequest.onAuthRequired| events. + // Only requests with a non-zero request ID can have their proxy + // associated with said ID. This is necessary to support + // correlation against any auth events received by the browser. + // Requests with a request ID of 0 therefore do not support + // dispatching |WebRequest.onAuthRequired| events. proxies_->AssociateProxyWithRequestId( this, content::GlobalRequestID(render_process_id_, request_id)); network_request_id_to_web_request_id_.emplace(request_id, web_request_id); } auto result = requests_.emplace( - web_request_id, - std::make_unique<InProgressRequest>( - this, web_request_id, request_id, routing_id, options, request, - traffic_annotation, std::move(loader_receiver), std::move(client))); + web_request_id, std::make_unique<InProgressRequest>( + this, web_request_id, request_id, routing_id, options, + ukm_source_id_, request, traffic_annotation, + std::move(loader_receiver), std::move(client))); result.first->second->Restart(); } diff --git a/chromium/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h b/chromium/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h index 35c4bda6134..67e79302271 100644 --- a/chromium/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h +++ b/chromium/extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h @@ -25,6 +25,7 @@ #include "mojo/public/cpp/bindings/remote.h" #include "net/base/completion_once_callback.h" #include "net/traffic_annotation/network_traffic_annotation.h" +#include "services/metrics/public/cpp/ukm_source_id.h" #include "services/network/public/cpp/resource_request.h" #include "services/network/public/mojom/network_context.mojom.h" #include "services/network/public/mojom/url_loader.mojom.h" @@ -54,6 +55,7 @@ class WebRequestProxyingURLLoaderFactory int32_t routing_id, int32_t network_service_request_id, uint32_t options, + ukm::SourceId ukm_source_id, const network::ResourceRequest& request, const net::MutableNetworkTrafficAnnotationTag& traffic_annotation, mojo::PendingReceiver<network::mojom::URLLoader> loader_receiver, @@ -106,15 +108,41 @@ class WebRequestProxyingURLLoaderFactory OnHeadersReceivedCallback callback) override; private: + // The state of an InprogressRequest. This is reported via UMA and UKM + // at the end of the request, so do not change enum values. + enum State { + kInProgress = 0, + kInProgressWithFinalResponseReceived, + kInvalid, // This is an invalid state and must not be recorded. + kRedirectFollowedByAnotherInProgressRequest, + kRejectedByNetworkError, + kRejectedByNetworkErrorAfterReceivingFinalResponse, + kDetachedFromClient, + kDetachedFromClientAfterReceivingResponse, + kRejectedByOnBeforeRequest, + kRejectedByOnBeforeSendHeaders, + kRejectedByOnHeadersReceivedForFinalResponse, + kRejectedByOnHeadersReceivedForRedirect, + kRejectedByOnHeadersReceivedForAuth, + kRejectedByOnAuthRequired, + kCompleted, + kMaxValue = kCompleted, + }; // These two methods combined form the implementation of Restart(). void UpdateRequestInfo(); void RestartInternal(); - void ContinueToBeforeSendHeaders(int error_code); - void ContinueToSendHeaders(const std::set<std::string>& removed_headers, + void ContinueToBeforeSendHeaders(State state_on_error, int error_code); + void ContinueToBeforeSendHeadersWithOk(); + void ContinueToSendHeaders(State state_on_error, + const std::set<std::string>& removed_headers, const std::set<std::string>& set_headers, int error_code); - void ContinueToStartRequest(int error_code); + void ContinueToSendHeadersWithOk( + const std::set<std::string>& removed_headers, + const std::set<std::string>& set_headers); + void ContinueToStartRequest(State state_on_error, int error_code); + void ContinueToStartRequestWithOk(); void ContinueToHandleOverrideHeaders(int error_code); void ContinueToResponseStarted(int error_code); void ContinueAuthRequest(const net::AuthChallengeInfo& auth_info, @@ -127,7 +155,10 @@ class WebRequestProxyingURLLoaderFactory int error_code); void HandleResponseOrRedirectHeaders( net::CompletionOnceCallback continuation); - void OnRequestError(const network::URLLoaderCompletionStatus& status); + void OnRequestError(const network::URLLoaderCompletionStatus& status, + State state); + void OnNetworkError(const network::URLLoaderCompletionStatus& status); + void OnClientDisconnected(); void OnLoaderDisconnected(uint32_t custom_reason, const std::string& description); bool IsRedirectSafe(const GURL& from_url, @@ -142,6 +173,7 @@ class WebRequestProxyingURLLoaderFactory const int32_t network_service_request_id_ = 0; const int32_t routing_id_ = 0; const uint32_t options_ = 0; + const ukm::SourceId ukm_source_id_; const net::MutableNetworkTrafficAnnotationTag traffic_annotation_; mojo::Receiver<network::mojom::URLLoader> proxied_loader_receiver_; mojo::Remote<network::mojom::URLLoaderClient> target_client_; @@ -179,6 +211,7 @@ class WebRequestProxyingURLLoaderFactory OnHeadersReceivedCallback on_headers_received_callback_; mojo::Receiver<network::mojom::TrustedHeaderClient> header_client_receiver_{ this}; + bool is_header_client_receiver_paused_ = false; // If |has_any_extra_headers_listeners_| is set to false and a redirect is // in progress, this stores the parameters to FollowRedirect that came from @@ -195,6 +228,7 @@ class WebRequestProxyingURLLoaderFactory DISALLOW_COPY_AND_ASSIGN(FollowRedirectParams); }; std::unique_ptr<FollowRedirectParams> pending_follow_redirect_params_; + State state_ = State::kInProgress; base::WeakPtrFactory<InProgressRequest> weak_factory_{this}; @@ -213,7 +247,8 @@ class WebRequestProxyingURLLoaderFactory mojo::PendingReceiver<network::mojom::TrustedURLLoaderHeaderClient> header_client_receiver, WebRequestAPI::ProxySet* proxies, - content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type); + content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type, + ukm::SourceId ukm_source_id); ~WebRequestProxyingURLLoaderFactory() override; @@ -229,7 +264,8 @@ class WebRequestProxyingURLLoaderFactory mojo::PendingReceiver<network::mojom::TrustedURLLoaderHeaderClient> header_client_receiver, WebRequestAPI::ProxySet* proxies, - content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type); + content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type, + ukm::SourceId ukm_source_id); // network::mojom::URLLoaderFactory: void CreateLoaderAndStart( @@ -289,6 +325,9 @@ class WebRequestProxyingURLLoaderFactory const content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type_; + // A UKM source ID associated with the content::WebContents of the initiator + // frame. + const ukm::SourceId ukm_source_id_; // Mapping from our own internally generated request ID to an // InProgressRequest instance. |