Bug 1346757 - Change the downloadError callback timing.
MozReview-Commit-ID: JleLPltEBOw
--- a/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
@@ -352,17 +352,16 @@ nsUrlClassifierStreamUpdater::FetchNext(
nsresult rv = FetchUpdate(update.mUrl,
EmptyCString(),
true, // This method is for v2 and v2 is always a POST.
update.mTable);
if (NS_FAILED(rv)) {
LOG(("Error fetching update url: %s\n", update.mUrl.get()));
// We can commit the urls that we've applied so far. This is
// probably a transient server problem, so trigger backoff.
- mDownloadErrorCallback->HandleEvent(EmptyCString());
mDownloadError = true;
mDBService->FinishUpdate();
return rv;
}
mPendingUpdates.RemoveElementAt(0);
return NS_OK;
@@ -437,49 +436,55 @@ nsUrlClassifierStreamUpdater::UpdateSucc
{
LOG(("nsUrlClassifierStreamUpdater::UpdateSuccess [this=%p]", this));
if (mPendingUpdates.Length() != 0) {
NS_WARNING("Didn't fetch all safebrowsing update redirects");
}
// DownloadDone() clears mSuccessCallback, so we save it off here.
nsCOMPtr<nsIUrlClassifierCallback> successCallback = mDownloadError ? nullptr : mSuccessCallback.get();
+ nsCOMPtr<nsIUrlClassifierCallback> downloadErrorCallback = mDownloadError ? mDownloadErrorCallback.get() : nullptr;
DownloadDone();
nsAutoCString strTimeout;
strTimeout.AppendInt(requestedTimeout);
if (successCallback) {
LOG(("nsUrlClassifierStreamUpdater::UpdateSuccess callback [this=%p]",
this));
successCallback->HandleEvent(strTimeout);
- } else {
- LOG(("nsUrlClassifierStreamUpdater::UpdateSuccess skipping callback [this=%p]",
- this));
+ } else if (downloadErrorCallback) {
+ downloadErrorCallback->HandleEvent(mDownloadErrorStatusStr);
+ mDownloadErrorStatusStr = EmptyCString();
+ LOG(("Notify download error callback in UpdateSuccess [this=%p]", this));
}
// Now fetch the next request
LOG(("stream updater: calling into fetch next request"));
FetchNextRequest();
return NS_OK;
}
NS_IMETHODIMP
nsUrlClassifierStreamUpdater::UpdateError(nsresult result)
{
LOG(("nsUrlClassifierStreamUpdater::UpdateError [this=%p]", this));
// DownloadDone() clears mUpdateErrorCallback, so we save it off here.
nsCOMPtr<nsIUrlClassifierCallback> errorCallback = mDownloadError ? nullptr : mUpdateErrorCallback.get();
-
+ nsCOMPtr<nsIUrlClassifierCallback> downloadErrorCallback = mDownloadError ? mDownloadErrorCallback.get() : nullptr;
DownloadDone();
nsAutoCString strResult;
strResult.AppendInt(static_cast<uint32_t>(result));
if (errorCallback) {
errorCallback->HandleEvent(strResult);
+ } else if (downloadErrorCallback) {
+ LOG(("Notify download error callback in UpdateError [this=%p]", this));
+ downloadErrorCallback->HandleEvent(mDownloadErrorStatusStr);
+ mDownloadErrorStatusStr = EmptyCString();
}
return NS_OK;
}
nsresult
nsUrlClassifierStreamUpdater::AddRequestBody(const nsACString &aRequestBody)
{
@@ -700,23 +705,18 @@ nsUrlClassifierStreamUpdater::OnStartReq
strStatus.AppendInt(requestStatus);
downloadError = true;
}
}
}
if (downloadError) {
LOG(("nsUrlClassifierStreamUpdater::Download error [this=%p]", this));
-
- // It's possible for mDownloadErrorCallback to be null on shutdown.
- if (mDownloadErrorCallback) {
- mDownloadErrorCallback->HandleEvent(strStatus);
- }
-
mDownloadError = true;
+ mDownloadErrorStatusStr = strStatus;
status = NS_ERROR_ABORT;
} else if (NS_SUCCEEDED(status)) {
MOZ_ASSERT(mDownloadErrorCallback);
mBeganStream = true;
LOG(("nsUrlClassifierStreamUpdater::Beginning stream [this=%p]", this));
rv = mDBService->BeginStream(mStreamTable);
NS_ENSURE_SUCCESS(rv, rv);
}
--- a/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h
+++ b/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h
@@ -69,16 +69,18 @@ private:
nsresult FetchNextRequest();
bool mIsUpdating;
bool mInitialized;
bool mDownloadError;
bool mBeganStream;
+ nsCString mDownloadErrorStatusStr;
+
// Note that mStreamTable is only used by v2, it is empty for v4 update.
nsCString mStreamTable;
nsCOMPtr<nsIChannel> mChannel;
nsCOMPtr<nsIUrlClassifierDBService> mDBService;
nsCOMPtr<nsITimer> mTimer;
struct PendingRequest {
--- a/toolkit/components/url-classifier/tests/unit/test_streamupdater.js
+++ b/toolkit/components/url-classifier/tests/unit/test_streamupdater.js
@@ -97,17 +97,17 @@ function testInvalidUrlForward() {
var update = buildPhishingUpdate(
[{ "chunkNum" : 1,
"urls" : add1Urls }]);
update += "u:asdf://blah/blah\n"; // invalid URL scheme
// add1Urls is present, but that is an artifact of the way we do the test.
var assertions = {
- "tableData" : "",
+ "tableData" : "test-phish-simple;a:1",
"urlsExist" : add1Urls
};
doTest([update], assertions, true);
}
// A failed network request causes the update to fail.
function testErrorUrlForward() {
@@ -115,17 +115,17 @@ function testErrorUrlForward() {
var update = buildPhishingUpdate(
[{ "chunkNum" : 1,
"urls" : add1Urls }]);
update += "u:http://test.invalid/asdf/asdf\n"; // invalid URL scheme
// add1Urls is present, but that is an artifact of the way we do the test.
var assertions = {
- "tableData" : "",
+ "tableData" : "test-phish-simple;a:1",
"urlsExist" : add1Urls
};
doTest([update], assertions, true);
}
function testMultipleTables() {
var add1Urls = [ "foo-multiple.com/a", "bar-multiple.com/c" ];