Bug 1032414: Always return failure in OnStopRequest on network error (r=gcp)

This commit is contained in:
Monica Chew 2014-11-07 07:12:37 -08:00
parent 90fd4d6a3f
commit b9779d70ee
3 changed files with 50 additions and 28 deletions

View File

@ -340,14 +340,19 @@ NS_IMETHODIMP
nsUrlClassifierStreamUpdater::StreamFinished(nsresult status,
uint32_t requestedDelay)
{
// We are a service and may not be reset with Init between calls, so reset
// mBeganStream manually.
mBeganStream = false;
LOG(("nsUrlClassifierStreamUpdater::StreamFinished [%x, %d]", status, requestedDelay));
if (NS_FAILED(status) || mPendingUpdates.Length() == 0) {
// We're done.
LOG(("nsUrlClassifierStreamUpdater::Done [this=%p]", this));
mDBService->FinishUpdate();
return NS_OK;
}
// Wait the requested amount of time before starting a new stream.
// This appears to be a duplicate timer (see bug 1110891)
nsresult rv;
mTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
if (NS_SUCCEEDED(rv)) {
@ -378,7 +383,12 @@ nsUrlClassifierStreamUpdater::UpdateSuccess(uint32_t requestedTimeout)
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));
}
// Now fetch the next request
LOG(("stream updater: calling into fetch next request"));
@ -452,19 +462,20 @@ nsUrlClassifierStreamUpdater::OnStartRequest(nsIRequest *request,
nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(request);
if (httpChannel) {
rv = httpChannel->GetStatus(&status);
LOG(("nsUrlClassifierStreamUpdater::OnStartRequest (status=%x, this=%p)",
status, this));
NS_ENSURE_SUCCESS(rv, rv);
if (NS_ERROR_CONNECTION_REFUSED == status ||
NS_ERROR_NET_TIMEOUT == status) {
if (NS_FAILED(status)) {
// Assume we're overloading the server and trigger backoff.
downloadError = true;
}
if (NS_SUCCEEDED(status)) {
} else {
bool succeeded = false;
rv = httpChannel->GetRequestSucceeded(&succeeded);
NS_ENSURE_SUCCESS(rv, rv);
LOG(("nsUrlClassifierStreamUpdater::OnStartRequest (%s)", succeeded ?
"succeeded" : "failed"));
if (!succeeded) {
// 404 or other error, pass error status back
LOG(("HTTP request returned failure code."));
@ -481,11 +492,13 @@ nsUrlClassifierStreamUpdater::OnStartRequest(nsIRequest *request,
}
if (downloadError) {
LOG(("nsUrlClassifierStreamUpdater::Download error [this=%p]", this));
mDownloadErrorCallback->HandleEvent(strStatus);
mDownloadError = true;
status = NS_ERROR_ABORT;
} else if (NS_SUCCEEDED(status)) {
mBeganStream = true;
LOG(("nsUrlClassifierStreamUpdater::Beginning stream [this=%p]", this));
rv = mDBService->BeginStream(mStreamTable);
NS_ENSURE_SUCCESS(rv, rv);
}
@ -528,7 +541,8 @@ nsUrlClassifierStreamUpdater::OnStopRequest(nsIRequest *request, nsISupports* co
if (!mDBService)
return NS_ERROR_NOT_INITIALIZED;
LOG(("OnStopRequest (status %x)", aStatus));
LOG(("OnStopRequest (status %x, beganStream %s, this=%p)", aStatus,
mBeganStream ? "true" : "false", this));
nsresult rv;
@ -536,10 +550,12 @@ nsUrlClassifierStreamUpdater::OnStopRequest(nsIRequest *request, nsISupports* co
// Success, finish this stream and move on to the next.
rv = mDBService->FinishStream();
} else if (mBeganStream) {
LOG(("OnStopRequest::Canceling update [this=%p]", this));
// We began this stream and couldn't finish it. We have to cancel the
// update, it's not in a consistent state.
rv = mDBService->CancelUpdate();
} else {
LOG(("OnStopRequest::Finishing update [this=%p]", this));
// The fetch failed, but we didn't start the stream (probably a
// server or connection error). We can commit what we've applied
// so far, and request again later.
@ -548,7 +564,12 @@ nsUrlClassifierStreamUpdater::OnStopRequest(nsIRequest *request, nsISupports* co
mChannel = nullptr;
return rv;
// If the fetch failed, return the network status rather than NS_OK, the
// result of finishing a possibly-empty update
if (NS_SUCCEEDED(aStatus)) {
return rv;
}
return aStatus;
}
///////////////////////////////////////////////////////////////////////////////

View File

@ -176,8 +176,9 @@ function doErrorUpdate(tables, success, failure) {
function doStreamUpdate(updateText, success, failure, downloadFailure) {
var dataUpdate = "data:," + encodeURIComponent(updateText);
if (!downloadFailure)
if (!downloadFailure) {
downloadFailure = failure;
}
streamUpdater.downloadUpdates("test-phish-simple,test-malware-simple", "",
dataUpdate, success, failure, downloadFailure);

View File

@ -7,6 +7,8 @@ function doTest(updates, assertions, expectError)
}
}
// Never use the same URLs for multiple tests, because we aren't guaranteed
// to reset the database between tests.
function testFillDb() {
var add1Urls = [ "zaz.com/a", "yxz.com/c" ];
@ -27,9 +29,9 @@ function testFillDb() {
}
function testSimpleForward() {
var add1Urls = [ "foo.com/a", "bar.com/c" ];
var add2Urls = [ "foo.com/b" ];
var add3Urls = [ "bar.com/d" ];
var add1Urls = [ "foo-simple.com/a", "bar-simple.com/c" ];
var add2Urls = [ "foo-simple.com/b" ];
var add3Urls = [ "bar-simple.com/d" ];
var update = "n:1000\n";
update += "i:test-phish-simple\n";
@ -60,8 +62,8 @@ function testSimpleForward() {
// Make sure that a nested forward (a forward within a forward) causes
// the update to fail.
function testNestedForward() {
var add1Urls = [ "foo.com/a", "bar.com/c" ];
var add2Urls = [ "foo.com/b" ];
var add1Urls = [ "foo-nested.com/a", "bar-nested.com/c" ];
var add2Urls = [ "foo-nested.com/b" ];
var update = "n:1000\n";
update += "i:test-phish-simple\n";
@ -91,46 +93,44 @@ function testNestedForward() {
// An invalid URL forward causes the update to fail.
function testInvalidUrlForward() {
var add1Urls = [ "foo.com/a", "bar.com/c" ];
var add1Urls = [ "foo-invalid.com/a", "bar-invalid.com/c" ];
var update = buildPhishingUpdate(
[{ "chunkNum" : 1,
"urls" : add1Urls }]);
update += "u:asdf://blah/blah\n"; // invalid URL scheme
// The first part of the update should have succeeded.
// add1Urls is present, but that is an artifact of the way we do the test.
var assertions = {
"tableData" : "test-phish-simple;a:1",
"tableData" : "",
"urlsExist" : add1Urls
};
doTest([update], assertions, false);
doTest([update], assertions, true);
}
// A failed network request causes the update to fail.
function testErrorUrlForward() {
var add1Urls = [ "foo.com/a", "bar.com/c" ];
var add1Urls = [ "foo-forward.com/a", "bar-forward.com/c" ];
var update = buildPhishingUpdate(
[{ "chunkNum" : 1,
"urls" : add1Urls }]);
update += "u:http://test.invalid/asdf/asdf\n"; // invalid URL scheme
// The first part of the update should have succeeded
// add1Urls is present, but that is an artifact of the way we do the test.
var assertions = {
"tableData" : "test-phish-simple;a:1",
"tableData" : "",
"urlsExist" : add1Urls
};
doTest([update], assertions, false);
doTest([update], assertions, true);
}
function testMultipleTables() {
var add1Urls = [ "foo.com/a", "bar.com/c" ];
var add2Urls = [ "foo.com/b" ];
var add3Urls = [ "bar.com/d" ];
var add1Urls = [ "foo-multiple.com/a", "bar-multiple.com/c" ];
var add2Urls = [ "foo-multiple.com/b" ];
var add3Urls = [ "bar-multiple.com/d" ];
var update = "n:1000\n";
update += "i:test-phish-simple\n";
@ -179,7 +179,7 @@ QueryInterface: function(iid)
// Tests a database reset request.
function testReset() {
var addUrls1 = [ "foo.com/a", "foo.com/b" ];
var addUrls1 = [ "foo-reset.com/a", "foo-reset.com/b" ];
var update1 = buildPhishingUpdate(
[
{ "chunkNum" : 1,
@ -188,7 +188,7 @@ function testReset() {
var update2 = "n:1000\nr:pleasereset\n";
var addUrls3 = [ "bar.com/a", "bar.com/b" ];
var addUrls3 = [ "bar-reset.com/a", "bar-reset.com/b" ];
var update3 = buildPhishingUpdate(
[
{ "chunkNum" : 3,