Skip to content

Commit

Permalink
feedbackPrivate: Allow multiple handles to a log source from an exten…
Browse files Browse the repository at this point in the history 
…sion

Remove the rule that there can be only one source reader handle opened from a particular extension.

There will still be rate limiting mechanisms in place:
- 1 second between reads from one reader handle
- max of 10 open readers per source, regardless of extension.

BUG=768967

Change-Id: I00cb6af27e8a9bdb4da68e015e65fd7ca3c5422b
Reviewed-on: https://chromium-review.googlesource.com/685577
Commit-Queue: Simon Que <sque@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508677}
  • Loading branch information
@simonque
simonque authored and Commit Bot committed Oct 13, 2017
1 parent 95cfea6 commit 57f07de
Show file tree
Hide file tree
Showing 6 changed files with 240 additions and 144 deletions.
10 changes: 6 additions & 4 deletions extensions/browser/api/feedback_private/feedback_private_api_chromeos_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,11 +203,12 @@ TEST_F(FeedbackPrivateApiUnittest, ReadLogSourceMultipleSources) {
// Store the reader ID back into the params to set up for the next call.
params_1st_read.reader_id = std::make_unique<int>(result_reader_id);

// Cannot create a second reader from the same log source.
// Create a second reader from the same log source.
ReadLogSourceParams params_1st_read_repeated;
params_1st_read_repeated.source = api::feedback_private::LOG_SOURCE_MESSAGES;
params_1st_read_repeated.incremental = true;
EXPECT_NE("", RunReadLogSourceFunctionWithError(params_1st_read_repeated));
EXPECT_TRUE(RunReadLogSourceFunction(params_1st_read_repeated,
&result_reader_id, &result_string));

// Attempt to open LOG_SOURCE_UI_LATEST twice.
ReadLogSourceParams params_2nd_read;
Expand All @@ -221,11 +222,12 @@ TEST_F(FeedbackPrivateApiUnittest, ReadLogSourceMultipleSources) {
// Store the reader ID back into the params to set up for the next call.
params_2nd_read.reader_id = std::make_unique<int>(result_reader_id);

// Cannot create a second reader from the same log source.
// Create a second reader from the same log source.
ReadLogSourceParams params_2nd_read_repeated;
params_2nd_read_repeated.source = api::feedback_private::LOG_SOURCE_UILATEST;
params_2nd_read_repeated.incremental = true;
EXPECT_NE("", RunReadLogSourceFunctionWithError(params_2nd_read_repeated));
EXPECT_TRUE(RunReadLogSourceFunction(params_2nd_read_repeated,
&result_reader_id, &result_string));

// Close the two open log source readers, and make sure new ones can be
// opened.
Expand Down
173 changes: 96 additions & 77 deletions extensions/browser/api/feedback_private/log_source_access_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,22 @@ namespace extensions {

namespace {

namespace feedback_private = api::feedback_private;

using LogSource = api::feedback_private::LogSource;
using ReadLogSourceParams = api::feedback_private::ReadLogSourceParams;
using ReadLogSourceResult = api::feedback_private::ReadLogSourceResult;
using SystemLogsResponse = system_logs::SystemLogsResponse;

const int kMaxReadersPerSource = 10;

// The minimum time between consecutive reads of a log source by a particular
// extension.
const int kDefaultRateLimitingTimeoutMs = 1000;
constexpr base::TimeDelta kDefaultRateLimitingTimeout =
base::TimeDelta::FromMilliseconds(1000);

// If this is null, then |kDefaultRateLimitingTimeoutMs| is used as the timeout.
const base::TimeDelta* g_rate_limiting_timeout = nullptr;

base::TimeDelta GetMinTimeBetweenReads() {
return g_rate_limiting_timeout
? *g_rate_limiting_timeout
: base::TimeDelta::FromMilliseconds(kDefaultRateLimitingTimeoutMs);
return g_rate_limiting_timeout ? *g_rate_limiting_timeout
: kDefaultRateLimitingTimeout;
}

// SystemLogsResponse is a map of strings -> strings. The map value has the
Expand Down Expand Up @@ -69,108 +68,134 @@ void LogSourceAccessManager::SetRateLimitingTimeoutForTesting(
}

bool LogSourceAccessManager::FetchFromSource(
const feedback_private::ReadLogSourceParams& params,
const ReadLogSourceParams& params,
const std::string& extension_id,
const ReadLogSourceCallback& callback) {
SourceAndExtension key = SourceAndExtension(params.source, extension_id);
int requested_resource_id = params.reader_id ? *params.reader_id : 0;
int resource_id =
requested_resource_id > 0 ? requested_resource_id : CreateResource(key);
if (resource_id <= 0)
return false;

int requested_resource_id =
params.reader_id ? *params.reader_id : kInvalidResourceId;
ApiResourceManager<LogSourceResource>* resource_manager =
ApiResourceManager<LogSourceResource>::Get(context_);

const ResourceId resource_id =
requested_resource_id != kInvalidResourceId
? requested_resource_id
: CreateResource(params.source, extension_id);
if (resource_id == kInvalidResourceId)
return false;

LogSourceResource* resource =
resource_manager->Get(extension_id, resource_id);
if (!resource)
return false;

// Enforce the rules: rate-limit access to the source from the current
// extension. If not enough time has elapsed since the last access, do not
// read from the source, but instead return an empty response. From the
// caller's perspective, there is no new data. There is no need for the caller
// to keep track of the time since last access.
if (!UpdateSourceAccessTime(key)) {
feedback_private::ReadLogSourceResult empty_result;
callback.Run(empty_result);
// Enforce the rules: rate-limit access to the source from the current reader
// handle. If not enough time has elapsed since the last access, do not read
// from the source, but instead return an empty response. From the caller's
// perspective, there is no new data. There is no need for the caller to keep
// track of the time since last access.
if (!UpdateSourceAccessTime(resource_id)) {
callback.Run({});
return true;
}

// If the API call requested a non-incremental access, clean up the
// SingleLogSource by removing its API resource. Even if the existing source
// were originally created as incremental, passing in incremental=false on a
// later access indicates that the source should be closed afterwards.
bool delete_resource_when_done = !params.incremental;
const bool delete_resource_when_done = !params.incremental;

resource->GetLogSource()->Fetch(base::Bind(
&LogSourceAccessManager::OnFetchComplete, weak_factory_.GetWeakPtr(), key,
delete_resource_when_done, callback));
&LogSourceAccessManager::OnFetchComplete, weak_factory_.GetWeakPtr(),
extension_id, resource_id, delete_resource_when_done, callback));
return true;
}

void LogSourceAccessManager::OnFetchComplete(
const SourceAndExtension& key,
const std::string& extension_id,
ResourceId resource_id,
bool delete_resource,
const ReadLogSourceCallback& callback,
SystemLogsResponse* response) {
int resource_id = 0;
const auto iter = sources_.find(key);
if (iter != sources_.end())
resource_id = iter->second;

feedback_private::ReadLogSourceResult result;
// Always return reader_id=0 if there is a cleanup.
result.reader_id = delete_resource ? 0 : resource_id;
ReadLogSourceResult result;
// Always return invalid resource ID if there is a cleanup.
result.reader_id = delete_resource ? kInvalidResourceId : resource_id;

GetLogLinesFromSystemLogsResponse(*response, &result.log_lines);
if (delete_resource) {
// This should also remove the entry from |sources_|.
ApiResourceManager<LogSourceResource>::Get(context_)->Remove(
key.extension_id, resource_id);
ApiResourceManager<LogSourceResource>::Get(context_)->Remove(extension_id,
resource_id);
}

callback.Run(result);
}

void LogSourceAccessManager::RemoveSource(const SourceAndExtension& key) {
sources_.erase(key);
void LogSourceAccessManager::RemoveHandle(ResourceId id) {
auto iter = open_handles_.find(id);
if (iter == open_handles_.end())
return;
const SourceAndExtension& source_and_extension = *iter->second;
const LogSource source = source_and_extension.source;
if (num_readers_per_source_.find(source) != num_readers_per_source_.end())
num_readers_per_source_[source]--;

open_handles_.erase(id);
}

LogSourceAccessManager::SourceAndExtension::SourceAndExtension(
feedback_private::LogSource source,
LogSource source,
const std::string& extension_id)
: source(source), extension_id(extension_id) {}

int LogSourceAccessManager::CreateResource(const SourceAndExtension& key) {
// Enforce the rules: Do not create a new SingleLogSource if there was already
// one created for |key|.
if (sources_.find(key) != sources_.end())
return 0;

LogSourceAccessManager::ResourceId LogSourceAccessManager::CreateResource(
LogSource source,
const std::string& extension_id) {
// Enforce the rules: Do not create too many SingleLogSource objects to read
// from a source, even if they are from different extensions.
if (GetNumActiveResourcesForSource(key.source) >= kMaxReadersPerSource)
return 0;

std::unique_ptr<LogSourceResource> new_resource =
std::make_unique<LogSourceResource>(
key.extension_id,
ExtensionsAPIClient::Get()
->GetFeedbackPrivateDelegate()
->CreateSingleLogSource(key.source),
base::Bind(&LogSourceAccessManager::RemoveSource,
weak_factory_.GetWeakPtr(), key));

int id = ApiResourceManager<LogSourceResource>::Get(context_)->Add(
new_resource.release());
sources_[key] = id;

return id;
if (GetNumActiveResourcesForSource(source) >= kMaxReadersPerSource)
return kInvalidResourceId;

auto new_resource = std::make_unique<LogSourceResource>(
extension_id, ExtensionsAPIClient::Get()
->GetFeedbackPrivateDelegate()
->CreateSingleLogSource(source));

auto* resource_manager = ApiResourceManager<LogSourceResource>::Get(context_);
// Create an ID for the resource using the API Resource Manager, but don't
// release ownership of it until a valid ID has been secured.
ResourceId resource_id = resource_manager->Add(new_resource.get());
if (resource_id == kInvalidResourceId)
return kInvalidResourceId;

if (open_handles_.find(resource_id) != open_handles_.end())
return kInvalidResourceId;

// Now that |resource_id| has been determined to be valid, release ownership
// of the LogSourceResource, which is now owned by the API resource manager.
new_resource.release();

// The resource ID isn't known until |new_resource| is added to the API
// Resource Manager, but it needs to be passed into the resource afterward, so
// that the resource can unregister itself from LogSourceAccessManager. It is
// passed in as part of a callback.
resource_manager->Get(extension_id, resource_id)
->set_unregister_callback(
base::Bind(&LogSourceAccessManager::RemoveHandle,
weak_factory_.GetWeakPtr(), resource_id));

open_handles_.emplace(
resource_id, std::make_unique<SourceAndExtension>(source, extension_id));
num_readers_per_source_[source]++;

return resource_id;
}

bool LogSourceAccessManager::UpdateSourceAccessTime(
const SourceAndExtension& key) {
bool LogSourceAccessManager::UpdateSourceAccessTime(ResourceId id) {
auto iter = open_handles_.find(id);
if (iter == open_handles_.end())
return false;

const SourceAndExtension& key = *iter->second;
if (rate_limiters_.find(key) == rate_limiters_.end()) {
rate_limiters_.emplace(
key, std::make_unique<AccessRateLimiter>(1, GetMinTimeBetweenReads(),
Expand All @@ -180,17 +205,11 @@ bool LogSourceAccessManager::UpdateSourceAccessTime(
}

size_t LogSourceAccessManager::GetNumActiveResourcesForSource(
feedback_private::LogSource source) const {
size_t count = 0;
// The stored entries are sorted first by source type, then by extension ID.
// We can take advantage of this fact to avoid iterating over all elements.
// Instead start from the first element that matches |source|, and end at the
// first element that does not match |source| anymore.
for (auto iter = sources_.lower_bound(SourceAndExtension(source, ""));
iter != sources_.end() && iter->first.source == source; ++iter) {
++count;
}
return count;
LogSource source) const {
auto iter = num_readers_per_source_.find(source);
if (iter == num_readers_per_source_.end())
return 0;
return iter->second;
}

} // namespace extensions
83 changes: 51 additions & 32 deletions extensions/browser/api/feedback_private/log_source_access_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,15 @@ class LogSourceAccessManager {
const std::string& extension_id,
const ReadLogSourceCallback& callback);

// Each log source may not have more than this number of readers accessing it,
// regardless of extension.
static constexpr int kMaxReadersPerSource = 10;

private:
FRIEND_TEST_ALL_PREFIXES(LogSourceAccessManagerTest,
MaxNumberOfOpenLogSources);
MaxNumberOfOpenLogSourcesSameExtension);
FRIEND_TEST_ALL_PREFIXES(LogSourceAccessManagerTest,
MaxNumberOfOpenLogSourcesDifferentExtensions);

// Contains a source/extension pair.
struct SourceAndExtension {
Expand All @@ -66,53 +72,59 @@ class LogSourceAccessManager {
std::make_pair(other.source, other.extension_id);
}

// The log source that this handle is accessing.
api::feedback_private::LogSource source;
// ID of the extension that opened this handle.
std::string extension_id;
};

using ResourceId = int;

// Returned when there was an error creating a new resource or looking for an
// existing resource.
static constexpr ResourceId kInvalidResourceId = 0;

// Creates a new LogSourceResource for the source and extension indicated by
// |key|. Stores the new resource in the API Resource Manager and stores the
// resource ID in |sources_| as a new entry. Returns the nonzero ID of the
// newly created resource, or 0 if there was already an existing resource for
// |key|.
int CreateResource(const SourceAndExtension& key);
// |source| and |extension_id|. Stores the new resource in the API Resource
// Manager, and uses the resource ID as a key for a new entry in
// |open_handles_|, with value being a SourceAndExtension containing |source|
// and |extension_id|.
//
// Returns the nonzero ID of the newly created LogSourceResource, or
// |kInvalidResourceId| if a new resource could not be created.
ResourceId CreateResource(api::feedback_private::LogSource source,
const std::string& extension_id);

// Callback that is passed to the log source from FetchFromSource.
// Arguments:
// - key: The source that was read, and the extension requesting the read.
// - delete_source: Set this if the source indicated by |key| should be
// removed from both the API Resource Manager and from |sources_|.
// - response_callback: Callback for sending the response as a
// ReadLogSourceResult struct.
void OnFetchComplete(const SourceAndExtension& key,
// - extension_id: ID of extension that opened the log source.
// - resource_id: Resource ID provided by API Resource Manager for the reader.
// - delete_source: Set this if the source opened by |handle| should be
// removed from both the API Resource Manager and from |open_handles_|.
// - callback: Callback for sending the response as a ReadLogSourceResult
// struct.
// - response: Contains the result from an operation to fetch from system
// log(s).
void OnFetchComplete(const std::string& extension_id,
ResourceId resource_id,
bool delete_source,
const ReadLogSourceCallback& callback,
system_logs::SystemLogsResponse* response);

// Removes an existing log source indicated by |key| from both the API
// Resource Manager and |sources_|.
void RemoveSource(const SourceAndExtension& key);
// Removes an existing log source handle indicated by |id| from
// |open_handles_|.
void RemoveHandle(ResourceId id);

// Attempts to update the entry for |key| in |last_access_times_| to the
// current time, to record that the source is being accessed by the extension
// right now. If less than |min_time_between_reads_| has elapsed since the
// last successful read, do not update the timestamp in |last_access_times_|,
// and instead return false. Otherwise returns true.
//
// Creates a new entry in |last_access_times_| if it doesn't exist. Will not
// delete from |last_access_times_|.
bool UpdateSourceAccessTime(const SourceAndExtension& key);

// Returns the number of entries in |sources_| with source=|source|.
// Returns the number of entries in |open_handles_| with source=|source|.
size_t GetNumActiveResourcesForSource(
api::feedback_private::LogSource source) const;

// Every SourceAndExtension is linked to a unique SingleLogSource.
//
// Keys: SourceAndExtension for which a SingleLogSource has been created
// and not yet destroyed. (i.e. currently in use).
// Values: ID of the API Resource containing the SingleLogSource.
std::map<SourceAndExtension, int> sources_;
// Attempts to update the |last_access_time| field for the SourceAndExtension
// |open_handles_[id]|, to record that the source is being accessed by the
// handle right now. If less than |min_time_between_reads_| has elapsed since
// the last successful read, does not update |last_access_times|, and instead
// returns false. Otherwise returns true.
bool UpdateSourceAccessTime(ResourceId id);

// Keeps track of the last time each source was accessed by each extension.
// Each time FetchFromSource() is called, the timestamp gets updated.
Expand All @@ -123,6 +135,13 @@ class LogSourceAccessManager {
std::map<SourceAndExtension, std::unique_ptr<AccessRateLimiter>>
rate_limiters_;

// Contains all open handles, each uniquely identified by a ResourceId and
// additionally described by a SourceAndExtension struct.
std::map<ResourceId, std::unique_ptr<SourceAndExtension>> open_handles_;

// Keep count of the number of reader handles (resources) for each source.
std::map<api::feedback_private::LogSource, size_t> num_readers_per_source_;

// For fetching browser resources like ApiResourceManager.
content::BrowserContext* context_;

Expand Down
Loading

0 comments on commit 57f07de

Please sign in to comment.