Skip to content

Commit

Permalink
[loader] Add debugging code for FetcherCache crash (#444) (#462)
Browse files Browse the repository at this point in the history 
The CHECK for consistency in linked_hash_map::erase() fails in
production, where we are unable to find an obvious cause.

Now FetcherCache is ref counted to ensure that there isn't any life time
issue, and extra CHECKs are added so we can have more information about
the crash once released.

b/270993319

(cherry picked from commit dd3705f)

Co-authored-by: xiaomings <[email protected]>
  • Loading branch information
@cobalt-github-releaser-bot @xiaomings
cobalt-github-releaser-bot and xiaomings authored May 25, 2023
1 parent f114951 commit 7993ff6
Show file tree
Hide file tree
Showing 4 changed files with 103 additions and 41 deletions.
115 changes: 78 additions & 37 deletions cobalt/loader/fetcher_cache.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class CachedFetcherHandler : public Fetcher::Handler {
const std::string& url,
const scoped_refptr<net::HttpResponseHeaders>& headers,
const Origin& last_url_origin, bool did_fail_from_transient_error,
std::string* data)>
std::string data)>
SuccessCallback;

CachedFetcherHandler(const std::string& url, Fetcher::Handler* handler,
Expand All @@ -52,6 +52,7 @@ class CachedFetcherHandler : public Fetcher::Handler {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK(!wrapping_fetcher_);
DCHECK(wrapping_fetcher);

wrapping_fetcher_ = wrapping_fetcher;
}

Expand All @@ -63,6 +64,7 @@ class CachedFetcherHandler : public Fetcher::Handler {
// TODO: Respect HttpResponseHeaders::GetMaxAgeValue().
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK(wrapping_fetcher_);

auto response = handler_->OnResponseStarted(wrapping_fetcher_, headers);
if (response == kLoadResponseContinue && headers) {
headers_ = headers;
Expand All @@ -77,29 +79,33 @@ class CachedFetcherHandler : public Fetcher::Handler {
void OnReceived(Fetcher*, const char* data, size_t size) override {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK(wrapping_fetcher_);

data_.insert(data_.end(), data, data + size);
handler_->OnReceived(wrapping_fetcher_, data, size);
}

void OnReceivedPassed(Fetcher*, std::unique_ptr<std::string> data) override {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK(wrapping_fetcher_);

data_.insert(data_.end(), data->begin(), data->end());
handler_->OnReceivedPassed(wrapping_fetcher_, std::move(data));
}

void OnDone(Fetcher*) override {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK(wrapping_fetcher_);

handler_->OnDone(wrapping_fetcher_);
on_success_callback_.Run(
url_, headers_, wrapping_fetcher_->last_url_origin(),
wrapping_fetcher_->did_fail_from_transient_error(), &data_);
wrapping_fetcher_->did_fail_from_transient_error(), std::move(data_));
}

void OnError(Fetcher*, const std::string& error) override {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK(wrapping_fetcher_);

handler_->OnError(wrapping_fetcher_, error);
}

Expand Down Expand Up @@ -184,13 +190,12 @@ class FetcherCache::CacheEntry {
public:
CacheEntry(const scoped_refptr<net::HttpResponseHeaders>& headers,
const Origin& last_url_origin, bool did_fail_from_transient_error,
std::string* data)
std::string data)
: headers_(headers),
last_url_origin_(last_url_origin),
did_fail_from_transient_error_(did_fail_from_transient_error) {
DCHECK(data);
data_.swap(*data);
}
did_fail_from_transient_error_(did_fail_from_transient_error),
data_(std::move(data)),
capacity_(data_.capacity()) {}

const scoped_refptr<net::HttpResponseHeaders>& headers() const {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
Expand All @@ -213,16 +218,24 @@ class FetcherCache::CacheEntry {
}
size_t capacity() const {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
return data_.capacity();
CHECK_EQ(capacity_, data_.capacity());
return capacity_;
}

private:
CacheEntry(const CacheEntry&) = delete;
CacheEntry& operator=(const CacheEntry&) = delete;

THREAD_CHECKER(thread_checker_);

scoped_refptr<net::HttpResponseHeaders> headers_;
Origin last_url_origin_;
bool did_fail_from_transient_error_;
std::string data_;
const Origin last_url_origin_;
const bool did_fail_from_transient_error_;
const std::string data_;

// TODO(b/270993319): For debugging cache integrity issues in production only,
// remove after identifying the root cause.
const size_t capacity_;
};

FetcherCache::FetcherCache(const char* name, size_t capacity)
Expand All @@ -236,12 +249,15 @@ FetcherCache::FetcherCache(const char* name, size_t capacity)

FetcherCache::~FetcherCache() {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
CHECK_EQ(thread_id_, SbThreadGetId());
CHECK(destroy_soon_called_);

while (!cache_entries_.empty()) {
delete cache_entries_.begin()->second;
cache_entries_.erase(cache_entries_.begin());
cache_entries_.pop_front();
}

total_size_ = 0;
memory_size_in_bytes_ = 0;
count_resources_cached_ = 0;
}
Expand All @@ -251,12 +267,13 @@ Loader::FetcherCreator FetcherCache::GetFetcherCreator(
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK(!real_fetcher_creator.is_null());

return base::Bind(&FetcherCache::CreateCachedFetcher, base::Unretained(this),
url, real_fetcher_creator);
return base::Bind(&FetcherCache::CreateCachedFetcher, this, url,
real_fetcher_creator);
}

void FetcherCache::NotifyResourceRequested(const std::string& url) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
CHECK_EQ(thread_id_, SbThreadGetId());

auto iter = cache_entries_.find(url);
if (iter != cache_entries_.end()) {
Expand All @@ -266,13 +283,26 @@ void FetcherCache::NotifyResourceRequested(const std::string& url) {
}
}

void FetcherCache::DestroySoon() {
#if !defined(COBALT_BUILD_TYPE_GOLD)
CHECK(HasOneRef());
#endif //! defined(COBALT_BUILD_TYPE_GOLD)

destroy_soon_called_ = true;
}

std::unique_ptr<Fetcher> FetcherCache::CreateCachedFetcher(
const GURL& url, const Loader::FetcherCreator& real_fetcher_creator,
Fetcher::Handler* handler) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK(!real_fetcher_creator.is_null());
DCHECK(handler);

#if !defined(COBALT_BUILD_TYPE_GOLD)
CHECK(!destroy_soon_called_);
#endif // !defined(COBALT_BUILD_TYPE_GOLD)
CHECK_EQ(thread_id_, SbThreadGetId());

auto iterator = cache_entries_.find(url.spec());
if (iterator != cache_entries_.end()) {
auto entry = iterator->second;
Expand All @@ -285,8 +315,7 @@ std::unique_ptr<Fetcher> FetcherCache::CreateCachedFetcher(
}

std::unique_ptr<CachedFetcherHandler> cached_handler(new CachedFetcherHandler(
url.spec(), handler,
base::Bind(&FetcherCache::OnFetchSuccess, base::Unretained(this))));
url.spec(), handler, base::Bind(&FetcherCache::OnFetchSuccess, this)));
return std::unique_ptr<Fetcher>(
new OngoingFetcher(std::move(cached_handler), real_fetcher_creator));
}
Expand All @@ -295,32 +324,44 @@ void FetcherCache::OnFetchSuccess(
const std::string& url,
const scoped_refptr<net::HttpResponseHeaders>& headers,
const Origin& last_url_origin, bool did_fail_from_transient_error,
std::string* data) {
std::string data) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK(data);

if (data->size() <= capacity_) {
auto entry = new CacheEntry(headers, last_url_origin,
did_fail_from_transient_error, data);
#if !defined(COBALT_BUILD_TYPE_GOLD)
CHECK(!destroy_soon_called_);
#endif // !defined(COBALT_BUILD_TYPE_GOLD)
CHECK_EQ(thread_id_, SbThreadGetId());

bool inserted = cache_entries_.insert(std::make_pair(url, entry)).second;
if (!inserted) {
// The resource is already cached.
delete entry;
return;
}
if (data.capacity() > capacity_) {
return;
}

total_size_ += entry->capacity();
while (total_size_ > capacity_) {
DCHECK(!cache_entries_.empty());
total_size_ -= cache_entries_.begin()->second->capacity();
delete cache_entries_.begin()->second;
cache_entries_.erase(cache_entries_.begin());
--count_resources_cached_;
}
++count_resources_cached_;
memory_size_in_bytes_ = total_size_;
auto entry = new CacheEntry(headers, last_url_origin,
did_fail_from_transient_error, std::move(data));

bool inserted = cache_entries_.insert(std::make_pair(url, entry)).second;
if (!inserted) {
// The resource is already cached.
delete entry;
return;
}

total_size_ += entry->capacity();
++count_resources_cached_;

while (total_size_ > capacity_) {
// TODO(b/270993319): For debugging cache integrity issues in production
// only, remove after identifying the root cause.
CHECK(!cache_entries_.empty());
CHECK_GE(total_size_, cache_entries_.begin()->second->capacity());

total_size_ -= cache_entries_.begin()->second->capacity();
delete cache_entries_.begin()->second;
cache_entries_.pop_front();
--count_resources_cached_;
}

memory_size_in_bytes_ = total_size_;
}

} // namespace loader
Expand Down
18 changes: 16 additions & 2 deletions cobalt/loader/fetcher_cache.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#ifndef COBALT_LOADER_FETCHER_CACHE_H_
#define COBALT_LOADER_FETCHER_CACHE_H_

#include <atomic>
#include <memory>
#include <string>

Expand All @@ -25,14 +26,15 @@
#include "cobalt/loader/loader.h"
#include "net/base/linked_hash_map.h"
#include "net/http/http_response_headers.h"
#include "starboard/thread.h"
#include "url/gurl.h"
#include "url/origin.h"

namespace cobalt {
namespace loader {

// Manages a cache for data fetched by Fetchers.
class FetcherCache {
class FetcherCache : public base::RefCountedThreadSafe<FetcherCache> {
public:
FetcherCache(const char* name, size_t capacity);
~FetcherCache();
Expand All @@ -41,6 +43,13 @@ class FetcherCache {
const GURL& url, const Loader::FetcherCreator& real_fetcher_creator);
void NotifyResourceRequested(const std::string& url);

// To signal the imminent destruction of this object. If everything is
// working as expected, there shouldn't be any other reference of this object,
// and all usages of this object should be completed.
// TODO(b/270993319): For debugging cache integrity issues in production only,
// remove after identifying the root cause.
void DestroySoon();

private:
class CacheEntry;

Expand All @@ -50,10 +59,15 @@ class FetcherCache {
void OnFetchSuccess(const std::string& url,
const scoped_refptr<net::HttpResponseHeaders>& headers,
const Origin& last_url_origin,
bool did_fail_from_transient_error, std::string* data);
bool did_fail_from_transient_error, std::string data);

THREAD_CHECKER(thread_checker_);

// TODO(b/270993319): For debugging cache integrity issues in production only,
// remove after identifying the root cause.
const SbThreadId thread_id_ = SbThreadGetId();
std::atomic_bool destroy_soon_called_{false};

const size_t capacity_;
size_t total_size_ = 0;

Expand Down
8 changes: 7 additions & 1 deletion cobalt/loader/loader_factory.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,13 @@ LoaderFactory::LoaderFactory(const char* name, FetcherFactory* fetcher_factory,
debugger_hooks_(debugger_hooks),
resource_provider_(resource_provider) {
if (encoded_image_cache_capacity > 0) {
fetcher_cache_.reset(new FetcherCache(name, encoded_image_cache_capacity));
fetcher_cache_ = new FetcherCache(name, encoded_image_cache_capacity);
}
}

LoaderFactory::~LoaderFactory() {
if (fetcher_cache_) {
fetcher_cache_->DestroySoon();
}
}

Expand Down
3 changes: 2 additions & 1 deletion cobalt/loader/loader_factory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ class LoaderFactory : public ScriptLoaderFactory {
const base::DebuggerHooks& debugger_hooks,
size_t encoded_image_cache_capacity,
base::ThreadPriority loader_thread_priority);
~LoaderFactory();

// Creates a loader that fetches and decodes an image.
std::unique_ptr<Loader> CreateImageLoader(
Expand Down Expand Up @@ -108,7 +109,7 @@ class LoaderFactory : public ScriptLoaderFactory {
// Used to cache the fetched raw data. Note that currently the cache is only
// used to cache Image data. We may introduce more caches once we want to
// cache fetched data for other resource types.
std::unique_ptr<FetcherCache> fetcher_cache_;
scoped_refptr<FetcherCache> fetcher_cache_;

// Used with CLOG to report errors with the image source.
const base::DebuggerHooks& debugger_hooks_;
Expand Down

0 comments on commit 7993ff6

Please sign in to comment.