From 13bd8b32e9068a27dd5de9a366dba7f8fa7abd70 Mon Sep 17 00:00:00 2001 From: Monica Chew Date: Fri, 7 Nov 2014 07:12:37 -0800 Subject: [PATCH] Bug 1032414: Always return failure in OnStopRequest on network error (r=gcp) --- .../nsUrlClassifierStreamUpdater.cpp | 35 ++++++++++++---- .../tests/unit/head_urlclassifier.js | 3 +- .../tests/unit/test_streamupdater.js | 40 +++++++++---------- 3 files changed, 50 insertions(+), 28 deletions(-) diff --git a/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp b/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp index 19493857ba1f..0a903360f544 100644 --- a/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp +++ b/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp @@ -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 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; } /////////////////////////////////////////////////////////////////////////////// diff --git a/toolkit/components/url-classifier/tests/unit/head_urlclassifier.js b/toolkit/components/url-classifier/tests/unit/head_urlclassifier.js index 69c249640bec..69ecf2fda691 100644 --- a/toolkit/components/url-classifier/tests/unit/head_urlclassifier.js +++ b/toolkit/components/url-classifier/tests/unit/head_urlclassifier.js @@ -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); diff --git a/toolkit/components/url-classifier/tests/unit/test_streamupdater.js b/toolkit/components/url-classifier/tests/unit/test_streamupdater.js index c377a8c67e58..9ba73a17e053 100644 --- a/toolkit/components/url-classifier/tests/unit/test_streamupdater.js +++ b/toolkit/components/url-classifier/tests/unit/test_streamupdater.js @@ -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,