Fix a bug that -[CWVDownloadTask cancel] causes an assertion failure.

The issue was that this condition is true even when the task is
cancelled (state kCancelled):
https://cs.chromium.org/chromium/src/ios/web_view/internal/cwv_download_task.mm?l=135&rcl=584d37cf04ac09d05ecacdf946edf11bdeeb0198
So it unintentionally tries to call DisownFile(), which fails.

Also introduce CWVDownloadErrorFailed and CWVDownloadErrorAborted
because clients probably want to distinguish failure and cancel.

Change-Id: I1637aabf8843685ccee01cf272a5f5acee85e005
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1504486
Auto-Submit: Hiroshi Ichikawa <ichikawa@chromium.org>
Reviewed-by: John Wu <jzw@chromium.org>
Commit-Queue: Hiroshi Ichikawa <ichikawa@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638431}

ios/web_view/internal/cwv_download_task.mm

@@ -25,7 +25,8 @@ int64_t const CWVDownloadSizeUnknown = -1;
 NSErrorDomain const CWVDownloadErrorDomain =
     @"org.chromium.chromewebview.DownloadErrorDomain";
 
-NSInteger const CWVDownloadErrorUnknown = -100;
+NSInteger const CWVDownloadErrorFailed = -100;
+NSInteger const CWVDownloadErrorAborted = -101;
 
 @interface CWVDownloadTask ()
 
@@ -132,18 +133,30 @@ class DownloadTaskObserverBridge : public web::DownloadTaskObserver {
 }
 
 - (void)downloadWasUpdated {
-  if (_internalTask->IsDone()) {
-    int errorCode = _internalTask->GetErrorCode();
-    if (errorCode == net::OK) {
-      // The writer deletes the file on its destructor by default. This prevents
-      // the deletion.
-      _internalTask->GetResponseWriter()->AsFileWriter()->DisownFile();
+  switch (_internalTask->GetState()) {
+    case web::DownloadTask::State::kInProgress: {
+      if ([_delegate
+              respondsToSelector:@selector(downloadTaskProgressDidChange:)]) {
+        [_delegate downloadTaskProgressDidChange:self];
+      }
+      break;
     }
-    [self notifyFinishWithErrorCode:errorCode];
-  } else {
-    if ([_delegate
-            respondsToSelector:@selector(downloadTaskProgressDidChange:)]) {
-      [_delegate downloadTaskProgressDidChange:self];
+    case web::DownloadTask::State::kComplete: {
+      int errorCode = _internalTask->GetErrorCode();
+      if (errorCode == net::OK) {
+        // The writer deletes the file on its destructor by default. This
+        // prevents the deletion.
+        _internalTask->GetResponseWriter()->AsFileWriter()->DisownFile();
+      }
+      [self notifyFinishWithErrorCode:errorCode];
+      break;
+    }
+    case web::DownloadTask::State::kNotStarted:
+    case web::DownloadTask::State::kCancelled: {
+      // Nothing to be done in these states.
+      // Note that state kCancelled is immediately followed by state kComplete
+      // with error code net::ERR_ABORTED, which is handled above.
+      break;
     }
   }
 }
@@ -151,14 +164,18 @@ class DownloadTaskObserverBridge : public web::DownloadTaskObserver {
 - (void)notifyFinishWithErrorCode:(int)errorCode {
   NSError* error = nil;
   if (errorCode != net::OK) {
+    // Use CWVDownloadErrorFailed for any errors other than net::ERR_ABORTED
+    // because a detailed error code is likely not very useful. Text
+    // representation of the error is still available via
+    // error.localizedDescription.
+    NSInteger cwvErrorCode = errorCode == net::ERR_ABORTED
+                                 ? CWVDownloadErrorAborted
+                                 : CWVDownloadErrorFailed;
     NSString* errorDescription =
         base::SysUTF8ToNSString(net::ErrorToShortString(errorCode));
-    // Always use CWVDownloadErrorUnknown so far because a detailed error code
-    // is likely not very useful. Text representation of the error is still
-    // available via error.localizedDescription.
     error = [NSError
         errorWithDomain:CWVDownloadErrorDomain
-                   code:CWVDownloadErrorUnknown
+                   code:cwvErrorCode
                userInfo:@{NSLocalizedDescriptionKey : errorDescription}];
   }
   if ([_delegate

ios/web_view/internal/cwv_download_task_unittest.mm

@@ -101,8 +101,11 @@ TEST_F(CWVDownloadTaskTest, FailedFlow) {
                                                 valid_local_file_path_)];
   ASSERT_TRUE(WaitUntilTaskStarts());
 
-  OCMExpect([mock_delegate_ downloadTask:cwv_task_
-                      didFinishWithError:[OCMArg isNotNil]]);
+  OCMExpect([mock_delegate_
+            downloadTask:cwv_task_
+      didFinishWithError:[OCMArg checkWithBlock:^(NSError* error) {
+        return error.code == CWVDownloadErrorFailed;
+      }]]);
   ASSERT_TRUE(FinishResponseWriter());
   fake_internal_task_->SetErrorCode(net::ERR_FAILED);
   fake_internal_task_->SetDone(true);
@@ -115,10 +118,23 @@ TEST_F(CWVDownloadTaskTest, CancelledFlow) {
                                                 valid_local_file_path_)];
   ASSERT_TRUE(WaitUntilTaskStarts());
 
+  OCMExpect([mock_delegate_
+            downloadTask:cwv_task_
+      didFinishWithError:[OCMArg checkWithBlock:^(NSError* error) {
+        return error.code == CWVDownloadErrorAborted;
+      }]]);
+
   ASSERT_TRUE(FinishResponseWriter());
   [cwv_task_ cancel];
   EXPECT_EQ(web::DownloadTask::State::kCancelled,
             fake_internal_task_->GetState());
+
+  // Simulate behavior of a real web::DownloadTask which transition to state
+  // kComplete with error code net::ERR_ABORTED when cancelled.
+  fake_internal_task_->SetErrorCode(net::ERR_ABORTED);
+  fake_internal_task_->SetDone(true);
+
+  EXPECT_OCMOCK_VERIFY((id)mock_delegate_);
 }
 
 // Tests a case when it fails to write to the specified local file path.

ios/web_view/public/cwv_download_task.h

@@ -19,8 +19,12 @@ FOUNDATION_EXPORT CWV_EXPORT int64_t const CWVDownloadSizeUnknown;
 // The error domain for download errors.
 FOUNDATION_EXPORT CWV_EXPORT NSErrorDomain const CWVDownloadErrorDomain;
 
-// An error code for CWVDownloadErrorDomain which doesn't indicate the cause.
-FOUNDATION_EXPORT CWV_EXPORT NSInteger const CWVDownloadErrorUnknown;
+// An error code for CWVDownloadErrorDomain for generic failure.
+FOUNDATION_EXPORT CWV_EXPORT NSInteger const CWVDownloadErrorFailed;
+
+// An error code for CWVDownloadErrorDomain when the task is aborted
+// (cancelled).
+FOUNDATION_EXPORT CWV_EXPORT NSInteger const CWVDownloadErrorAborted;
 
 // Represents a single browser download task.
 CWV_EXPORT
@@ -58,8 +62,11 @@ CWV_EXPORT
 // called if the task is not in progress.
 - (void)startDownloadToLocalFileAtPath:(NSString*)path;
 
-// Cancels the download. Cancelled download can be restarted by calling
-// -startDownloadToLocalFileAtPath:
+// Cancels the download.
+//
+// It triggers a delegate method -downloadTask:didFinishWithError: with
+// |error.code| CWVDownloadErrorAborted. Cancelled download can be restarted by
+// calling -startDownloadToLocalFileAtPath:
 - (void)cancel;
 
 @end
@@ -70,8 +77,9 @@ CWV_EXPORT
 
 // Called when the download has finished. |error| is nil when it has completed
 // successfully. |error| represents the error when the download has failed
-// e.g., due to network errors. |error| contains a description which describes
-// the type of an error.
+// (e.g., due to network errors) or has been cancelled. |error.code| is either
+// CWVDownloadErrorFailed or CWVDownloadErrorAborted. |error| also contains a
+// description which describes the type of an error.
 - (void)downloadTask:(CWVDownloadTask*)downloadTask
     didFinishWithError:(nullable NSError*)error;
 

ios/web_view/shell/shell_view_controller.m

@@ -376,6 +376,13 @@ NSString* const kWebViewShellJavaScriptDialogTextFieldAccessibiltyIdentifier =
                                              .usesSyncAndWalletSandbox ^= YES;
                                        }]];
 
+  [alertController
+      addAction:[UIAlertAction actionWithTitle:@"Cancel download"
+                                         style:UIAlertActionStyleDefault
+                                       handler:^(UIAlertAction* action) {
+                                         [weakSelf.downloadTask cancel];
+                                       }]];
+
   [self presentViewController:alertController animated:YES completion:nil];
 }