Bug 1434206 - Keep CacheResult objects in smart pointers. r?gcp
Some of the objects were kept in UniquePtr and nsAutoPtr but that
seemed unnecessary complexity given that we can simply use RefPtr
everywhere.
It's also possible to make all of the CacheResult arrays const
since we don't ever modify the elements once they are added.
MozReview-Commit-ID: 5OlcbkQLrGb
--- a/toolkit/components/url-classifier/LookupCache.h
+++ b/toolkit/components/url-classifier/LookupCache.h
@@ -84,31 +84,34 @@ public:
bool mProtocolV2;
};
typedef nsTArray<LookupResult> LookupResultArray;
class CacheResult {
public:
+ NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CacheResult);
+
enum { V2, V4 };
virtual int Ver() const = 0;
virtual bool findCompletion(const Completion& aCompletion) const = 0;
- virtual ~CacheResult() {}
-
template<typename T>
static const T* Cast(const CacheResult* aThat) {
return ((aThat && T::VER == aThat->Ver()) ?
reinterpret_cast<const T*>(aThat) : nullptr);
}
nsCString table;
Prefix prefix;
+
+protected:
+ virtual ~CacheResult() {}
};
class CacheResultV2 final : public CacheResult
{
public:
static const int VER;
// True when 'prefix' in CacheResult indicates a prefix that
@@ -156,17 +159,17 @@ public:
nsDependentCSubstring completion(
reinterpret_cast<const char*>(aCompletion.buf), COMPLETE_SIZE);
return response.fullHashes.Contains(completion);
}
virtual int Ver() const override { return VER; }
};
-typedef nsTArray<UniquePtr<CacheResult>> CacheResultArray;
+typedef nsTArray<RefPtr<const CacheResult>> ConstCacheResultArray;
class LookupCache {
public:
// Check for a canonicalized IP address.
static bool IsCanonicalizedIP(const nsACString& aHost);
// take a lookup string (www.hostname.com/path/to/resource.html) and
// expand it into the set of fragments that should be searched for in an
--- a/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ -804,19 +804,17 @@ nsUrlClassifierDBServiceWorker::CloseDb(
{
if (mClassifier) {
mClassifier->Close();
mClassifier = nullptr;
}
// Clear last completion result when close db so we will still cache completion
// result next time we re-open it.
- if (mLastResults) {
- mLastResults->Clear();
- }
+ mLastResults.Clear();
LOG(("urlclassifier db closed\n"));
return NS_OK;
}
nsresult
nsUrlClassifierDBServiceWorker::PreShutdown()
@@ -828,35 +826,32 @@ nsUrlClassifierDBServiceWorker::PreShutd
}
// WARNING: nothing we put here should affect an ongoing update thread. When in doubt,
// put things in Shutdown() instead.
return NS_OK;
}
nsresult
-nsUrlClassifierDBServiceWorker::CacheCompletions(CacheResultArray *results)
+nsUrlClassifierDBServiceWorker::CacheCompletions(const ConstCacheResultArray& aResults)
{
if (gShuttingDownThread) {
return NS_ERROR_ABORT;
}
LOG(("nsUrlClassifierDBServiceWorker::CacheCompletions [%p]", this));
if (!mClassifier) {
return NS_OK;
}
- // Ownership is transferred in to us
- nsAutoPtr<CacheResultArray> resultsPtr(results);
-
- if (resultsPtr->Length() == 0) {
+ if (aResults.Length() == 0) {
return NS_OK;
}
- if (IsSameAsLastResults(*resultsPtr)) {
+ if (IsSameAsLastResults(aResults)) {
LOG(("Skipping completions that have just been cached already."));
return NS_OK;
}
// Only cache results for tables that we have, don't take
// in tables we might accidentally have hit during a completion.
// This happens due to goog vs googpub lists existing.
nsTArray<nsCString> tables;
@@ -870,19 +865,18 @@ nsUrlClassifierDBServiceWorker::CacheCom
}
s += tables[i];
}
LOG(("Active tables: %s", s.get()));
}
ConstTableUpdateArray updates;
- for (uint32_t i = 0; i < resultsPtr->Length(); i++) {
+ for (const auto result : aResults) {
bool activeTable = false;
- CacheResult* result = resultsPtr->ElementAt(i).get();
for (uint32_t table = 0; table < tables.Length(); table++) {
if (tables[table].Equals(result->table)) {
activeTable = true;
break;
}
}
if (activeTable) {
@@ -904,28 +898,28 @@ nsUrlClassifierDBServiceWorker::CacheCom
} else {
LOG(("Completion received, but table %s is not active, so not caching.",
result->table.get()));
}
}
rv = mClassifier->ApplyFullHashes(updates);
if (NS_SUCCEEDED(rv)) {
- mLastResults = std::move(resultsPtr);
+ mLastResults = aResults;
}
return rv;
}
nsresult
-nsUrlClassifierDBServiceWorker::CacheResultToTableUpdate(const CacheResult* aCacheResult,
+nsUrlClassifierDBServiceWorker::CacheResultToTableUpdate(RefPtr<const CacheResult> aCacheResult,
RefPtr<TableUpdate> aUpdate)
{
RefPtr<TableUpdateV2> tuV2 = TableUpdate::Cast<TableUpdateV2>(aUpdate);
if (tuV2) {
- const CacheResultV2* result = CacheResult::Cast<const CacheResultV2>(aCacheResult);
+ RefPtr<const CacheResultV2> result = CacheResult::Cast<const CacheResultV2>(aCacheResult);
MOZ_ASSERT(result);
if (result->miss) {
return tuV2->NewMissPrefix(result->prefix);
} else {
LOG(("CacheCompletion hash %X, Addchunk %d", result->completion.ToUint32(),
result->addChunk));
@@ -934,17 +928,17 @@ nsUrlClassifierDBServiceWorker::CacheRes
return rv;
}
return tuV2->NewAddChunk(result->addChunk);
}
}
RefPtr<TableUpdateV4> tuV4 = TableUpdate::Cast<TableUpdateV4>(aUpdate);
if (tuV4) {
- const CacheResultV4* result = CacheResult::Cast<const CacheResultV4>(aCacheResult);
+ RefPtr<const CacheResultV4> result = CacheResult::Cast<const CacheResultV4>(aCacheResult);
MOZ_ASSERT(result);
if (LOG_ENABLED()) {
const FullHashExpiryCache& fullHashes = result->response.fullHashes;
for (auto iter = fullHashes.ConstIter(); !iter.Done(); iter.Next()) {
Completion completion;
completion.Assign(iter.Key());
LOG(("CacheCompletion(v4) hash %X, CacheExpireTime %" PRId64,
@@ -986,19 +980,17 @@ nsUrlClassifierDBServiceWorker::OpenDb()
return NS_OK;
}
NS_IMETHODIMP
nsUrlClassifierDBServiceWorker::ClearLastResults()
{
MOZ_ASSERT(!NS_IsMainThread(), "Must be on the background thread");
- if (mLastResults) {
- mLastResults->Clear();
- }
+ mLastResults.Clear();
return NS_OK;
}
nsresult
nsUrlClassifierDBServiceWorker::GetCacheInfo(const nsACString& aTable,
nsIUrlClassifierCacheInfo** aCache)
{
MOZ_ASSERT(!NS_IsMainThread(), "Must be on the background thread");
@@ -1006,26 +998,26 @@ nsUrlClassifierDBServiceWorker::GetCache
return NS_ERROR_NOT_AVAILABLE;
}
mClassifier->GetCacheInfo(aTable, aCache);
return NS_OK;
}
bool
-nsUrlClassifierDBServiceWorker::IsSameAsLastResults(const CacheResultArray& aResult) const
+nsUrlClassifierDBServiceWorker::IsSameAsLastResults(const ConstCacheResultArray& aResult) const
{
- if (!mLastResults || mLastResults->Length() != aResult.Length()) {
+ if (mLastResults.Length() != aResult.Length()) {
return false;
}
bool equal = true;
- for (uint32_t i = 0; i < mLastResults->Length() && equal; i++) {
- CacheResult* lhs = mLastResults->ElementAt(i).get();
- CacheResult* rhs = aResult[i].get();
+ for (uint32_t i = 0; i < mLastResults.Length() && equal; i++) {
+ RefPtr<const CacheResult> lhs = mLastResults[i];
+ RefPtr<const CacheResult> rhs = aResult[i];
if (lhs->Ver() != rhs->Ver()) {
return false;
}
if (lhs->Ver() == CacheResult::V2) {
equal = *(CacheResult::Cast<const CacheResultV2>(lhs)) ==
*(CacheResult::Cast<const CacheResultV2>(rhs));
@@ -1060,24 +1052,24 @@ public:
, mPendingCompletions(0)
, mCallback(c)
{}
private:
~nsUrlClassifierLookupCallback();
nsresult HandleResults();
- nsresult ProcessComplete(CacheResult* aCacheResult);
+ nsresult ProcessComplete(RefPtr<CacheResult> aCacheResult);
nsresult CacheMisses();
RefPtr<nsUrlClassifierDBService> mDBService;
nsAutoPtr<LookupResultArray> mResults;
// Completed results to send back to the worker for caching.
- nsAutoPtr<CacheResultArray> mCacheResults;
+ ConstCacheResultArray mCacheResults;
uint32_t mPendingCompletions;
nsCOMPtr<nsIUrlClassifierCallback> mCallback;
};
NS_IMPL_ISUPPORTS(nsUrlClassifierLookupCallback,
nsIUrlClassifierLookupCallback,
nsIUrlClassifierHashCompleterCallback)
@@ -1184,24 +1176,24 @@ nsUrlClassifierLookupCallback::Completio
const nsACString& aTableName,
uint32_t aChunkId)
{
LOG(("nsUrlClassifierLookupCallback::Completion [%p, %s, %d]",
this, PromiseFlatCString(aTableName).get(), aChunkId));
MOZ_ASSERT(!StringEndsWith(aTableName, NS_LITERAL_CSTRING("-proto")));
- nsAutoPtr<CacheResultV2> result(new CacheResultV2);
+ RefPtr<CacheResultV2> result = new CacheResultV2();
result->table = aTableName;
result->prefix.Assign(aCompleteHash);
result->completion.Assign(aCompleteHash);
result->addChunk = aChunkId;
- return ProcessComplete(result.forget());
+ return ProcessComplete(result);
}
NS_IMETHODIMP
nsUrlClassifierLookupCallback::CompletionV4(const nsACString& aPartialHash,
const nsACString& aTableName,
uint32_t aNegativeCacheDuration,
nsIArray* aFullHashes)
{
@@ -1215,17 +1207,17 @@ nsUrlClassifierLookupCallback::Completio
}
if (aNegativeCacheDuration > MAXIMUM_NEGATIVE_CACHE_DURATION_SEC) {
LOG(("Negative cache duration too large, clamping it down to"
"a reasonable value."));
aNegativeCacheDuration = MAXIMUM_NEGATIVE_CACHE_DURATION_SEC;
}
- nsAutoPtr<CacheResultV4> result(new CacheResultV4);
+ RefPtr<CacheResultV4> result = new CacheResultV4();
int64_t nowSec = PR_Now() / PR_USEC_PER_SEC;
result->table = aTableName;
result->prefix.Assign(aPartialHash);
result->response.negativeCacheExpirySec = nowSec + aNegativeCacheDuration;
// Fill in positive cache entries.
@@ -1242,32 +1234,24 @@ nsUrlClassifierLookupCallback::Completio
match->GetFullHash(fullHash);
uint32_t duration;
match->GetCacheDuration(&duration);
result->response.fullHashes.Put(fullHash, nowSec + duration);
}
- return ProcessComplete(result.forget());
+ return ProcessComplete(result);
}
nsresult
-nsUrlClassifierLookupCallback::ProcessComplete(CacheResult* aCacheResult)
+nsUrlClassifierLookupCallback::ProcessComplete(RefPtr<CacheResult> aCacheResult)
{
- // Send this completion to the store for caching.
- if (!mCacheResults) {
- mCacheResults = new (fallible) CacheResultArray();
- if (!mCacheResults) {
- return NS_ERROR_OUT_OF_MEMORY;
- }
- }
-
// OK if this fails, we just won't cache the item.
- mCacheResults->AppendElement(aCacheResult, fallible);
+ mCacheResults.AppendElement(aCacheResult, fallible);
// Check if this matched any of our results.
for (uint32_t i = 0; i < mResults->Length(); i++) {
LookupResult& result = mResults->ElementAt(i);
// Now, see if it verifies a lookup
if (!result.mNoise
&& result.mTableName.Equals(aCacheResult->table)
@@ -1329,21 +1313,20 @@ nsUrlClassifierLookupCallback::HandleRes
classifyCallback->HandleResult(result.mTableName, fullHashString);
}
}
// Some parts of this gethash request generated no hits at all.
// Save the prefixes we checked to prevent repeated requests.
CacheMisses();
- if (mCacheResults) {
- // This hands ownership of the cache results array back to the worker
- // thread.
- mDBService->CacheCompletions(mCacheResults.forget());
- }
+ // This hands ownership of the cache results array back to the worker
+ // thread.
+ mDBService->CacheCompletions(mCacheResults);
+ mCacheResults.Clear();
nsAutoCString tableStr;
for (uint32_t i = 0; i < tables.Length(); i++) {
if (i != 0)
tableStr.Append(',');
tableStr.Append(tables[i]);
}
@@ -1356,29 +1339,22 @@ nsUrlClassifierLookupCallback::CacheMiss
for (uint32_t i = 0; i < mResults->Length(); i++) {
const LookupResult &result = mResults->ElementAt(i);
// Skip V4 because cache information is already included in the
// fullhash response so we don't need to manually add it here.
if (!result.mProtocolV2 || result.Confirmed() || result.mNoise) {
continue;
}
- if (!mCacheResults) {
- mCacheResults = new (fallible) CacheResultArray();
- if (!mCacheResults) {
- return NS_ERROR_OUT_OF_MEMORY;
- }
- }
-
- auto cacheResult = new CacheResultV2;
+ RefPtr<CacheResultV2> cacheResult = new CacheResultV2();
cacheResult->table = result.mTableName;
cacheResult->prefix = result.hash.fixedLengthPrefix;
cacheResult->miss = true;
- if (!mCacheResults->AppendElement(cacheResult, fallible)) {
+ if (!mCacheResults.AppendElement(cacheResult, fallible)) {
return NS_ERROR_OUT_OF_MEMORY;
}
}
return NS_OK;
}
struct Provider {
nsCString name;
@@ -2426,17 +2402,17 @@ nsUrlClassifierDBService::GetCacheInfo(c
{
NS_ENSURE_TRUE(gDbBackgroundThread, NS_ERROR_NOT_INITIALIZED);
return mWorkerProxy->GetCacheInfo(aTable, aCallback);
return NS_OK;
}
nsresult
-nsUrlClassifierDBService::CacheCompletions(CacheResultArray *results)
+nsUrlClassifierDBService::CacheCompletions(const ConstCacheResultArray& results)
{
NS_ENSURE_TRUE(gDbBackgroundThread, NS_ERROR_NOT_INITIALIZED);
return mWorkerProxy->CacheCompletions(results);
}
bool
nsUrlClassifierDBService::CanComplete(const nsACString &aTableName)
--- a/toolkit/components/url-classifier/nsUrlClassifierDBService.h
+++ b/toolkit/components/url-classifier/nsUrlClassifierDBService.h
@@ -102,17 +102,17 @@ public:
NS_DECL_NSIURLCLASSIFIERDBSERVICE
NS_DECL_NSIURICLASSIFIER
NS_DECL_NSIURLCLASSIFIERINFO
NS_DECL_NSIOBSERVER
bool CanComplete(const nsACString &tableName);
bool GetCompleter(const nsACString& tableName,
nsIUrlClassifierHashCompleter** completer);
- nsresult CacheCompletions(mozilla::safebrowsing::CacheResultArray *results);
+ nsresult CacheCompletions(const mozilla::safebrowsing::ConstCacheResultArray& results);
static nsIThread* BackgroundThread();
static bool ShutdownHasStarted();
private:
const nsTArray<nsCString> kObservedPrefs = {
@@ -216,17 +216,17 @@ public:
// Open the DB connection
nsresult GCC_MANGLING_WORKAROUND OpenDb();
// Provide a way to forcibly close the db connection.
nsresult GCC_MANGLING_WORKAROUND CloseDb();
nsresult GCC_MANGLING_WORKAROUND PreShutdown();
- nsresult CacheCompletions(CacheResultArray * aEntries);
+ nsresult CacheCompletions(const ConstCacheResultArray& aEntries);
// Used to probe the state of the worker thread. When the update begins,
// mUpdateObserver will be set. When the update finished, mUpdateObserver
// will be nulled out in NotifyUpdateObserver.
bool IsBusyUpdating() const { return !!mUpdateObserver; }
// Check the DB ready state of the worker thread
bool IsDBOpened() const { return !!mClassifier; }
@@ -261,36 +261,36 @@ private:
const nsACString& tables,
nsIUrlClassifierLookupCallback* c);
nsresult AddNoise(const Prefix aPrefix,
const nsCString tableName,
uint32_t aCount,
LookupResultArray& results);
- nsresult CacheResultToTableUpdate(const CacheResult* aCacheResult,
+ nsresult CacheResultToTableUpdate(RefPtr<const CacheResult> aCacheResult,
RefPtr<TableUpdate> aUpdate);
- bool IsSameAsLastResults(const CacheResultArray& aResult) const;
+ bool IsSameAsLastResults(const ConstCacheResultArray& aResult) const;
nsAutoPtr<mozilla::safebrowsing::Classifier> mClassifier;
// The class that actually parses the update chunks.
nsAutoPtr<ProtocolParser> mProtocolParser;
// Directory where to store the SB databases.
nsCOMPtr<nsIFile> mCacheDir;
RefPtr<nsUrlClassifierDBService> mDBService;
TableUpdateArray mTableUpdates;
uint32_t mUpdateWaitSec;
// Stores the last results that triggered a table update.
- nsAutoPtr<CacheResultArray> mLastResults;
+ ConstCacheResultArray mLastResults;
nsresult mUpdateStatus;
nsTArray<nsCString> mUpdateTables;
nsCOMPtr<nsIUrlClassifierUpdateObserver> mUpdateObserver;
bool mInStream;
// The number of noise entries to add to the set of lookup results.
--- a/toolkit/components/url-classifier/nsUrlClassifierProxies.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierProxies.cpp
@@ -220,17 +220,17 @@ UrlClassifierDBServiceWorkerProxy::PreSh
nsCOMPtr<nsIRunnable> r =
NewRunnableMethod("nsUrlClassifierDBServiceWorker::PreShutdown",
mTarget,
&nsUrlClassifierDBServiceWorker::PreShutdown);
return DispatchToWorkerThread(r);
}
nsresult
-UrlClassifierDBServiceWorkerProxy::CacheCompletions(CacheResultArray * aEntries)
+UrlClassifierDBServiceWorkerProxy::CacheCompletions(const ConstCacheResultArray& aEntries)
{
nsCOMPtr<nsIRunnable> r = new CacheCompletionsRunnable(mTarget, aEntries);
return DispatchToWorkerThread(r);
}
NS_IMETHODIMP
UrlClassifierDBServiceWorkerProxy::CacheCompletionsRunnable::Run()
{
--- a/toolkit/components/url-classifier/nsUrlClassifierProxies.h
+++ b/toolkit/components/url-classifier/nsUrlClassifierProxies.h
@@ -125,28 +125,28 @@ public:
RefPtr<nsUrlClassifierDBServiceWorker> mTarget;
nsCString mUpdateChunk;
};
class CacheCompletionsRunnable : public mozilla::Runnable
{
public:
CacheCompletionsRunnable(nsUrlClassifierDBServiceWorker* aTarget,
- mozilla::safebrowsing::CacheResultArray* aEntries)
+ const mozilla::safebrowsing::ConstCacheResultArray& aEntries)
: mozilla::Runnable(
"UrlClassifierDBServiceWorkerProxy::CacheCompletionsRunnable")
, mTarget(aTarget)
, mEntries(aEntries)
{ }
NS_DECL_NSIRUNNABLE
private:
RefPtr<nsUrlClassifierDBServiceWorker> mTarget;
- mozilla::safebrowsing::CacheResultArray *mEntries;
+ mozilla::safebrowsing::ConstCacheResultArray mEntries;
};
class DoLocalLookupRunnable : public mozilla::Runnable
{
public:
DoLocalLookupRunnable(nsUrlClassifierDBServiceWorker* aTarget,
const nsACString& spec,
const nsACString& tables,
@@ -226,17 +226,17 @@ public:
nsresult DoLocalLookup(const nsACString& spec,
const nsACString& tables,
mozilla::safebrowsing::LookupResultArray* results);
nsresult OpenDb();
nsresult CloseDb();
nsresult PreShutdown();
- nsresult CacheCompletions(mozilla::safebrowsing::CacheResultArray * aEntries);
+ nsresult CacheCompletions(const mozilla::safebrowsing::ConstCacheResultArray& aEntries);
nsresult GetCacheInfo(const nsACString& aTable,
nsIUrlClassifierGetCacheCallback* aCallback);
private:
~UrlClassifierDBServiceWorkerProxy() {}
RefPtr<nsUrlClassifierDBServiceWorker> mTarget;
};