[Downloads] Enforce state transition integrity and state invariants.

content/browser/download/download_item_impl.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_IMPL_H_
 #define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_IMPL_H_
 
 #include <stdint.h>
 
 #include <string>
@@ skipping 199 matching lines @@
   virtual void MarkAsComplete();
 
   // DownloadDestinationObserver
   void DestinationUpdate(int64_t bytes_so_far,
                          int64_t bytes_per_sec,
                          const std::string& hash_state) override;
   void DestinationError(DownloadInterruptReason reason) override;
   void DestinationCompleted(const std::string& final_hash) override;
 
  private:
-  // Fine grained states of a download. Note that active downloads are created
-  // in IN_PROGRESS_INTERNAL state. However, downloads creates via history can
-  // be created in COMPLETE_INTERNAL, CANCELLED_INTERNAL and
-  // INTERRUPTED_INTERNAL.
+  // Fine grained states of a download.
+  //
+  // New downloads can be created in the following states:
+  //
+  //     INITIAL_INTERNAL:        All active new downloads.
+  //
+  //     COMPLETE_INTERNAL:       Downloads restored from persisted state.
+  //     CANCELLED_INTERNAL:      - do -
+  //     INTERRUPTED_INTERNAL:    - do -
+  //
+  //     IN_PROGRESS_INTERNAL:    SavePackage downloads.
+  //
+  // On debug builds, state transitions can be verified via
+  // IsValidStateTransition() and IsValidSavePackageStateTransition(). Allowed
+  // state transitions are described below, both for normal downloads and
+  // SavePackage downloads.
+  enum DownloadInternalState {
+    // Initial state. Regular downloads are created in this state until the
+    // Start() call is received.
+    //
+    // Transitions to (regular):
+    //   TARGET_PENDING_INTERNAL: After a successful Start() call.
+    //   INTERRUPTED_INTERNAL:    Afater a failed Start() call.
+    //
+    // Transitions to (SavePackage):
+    //   <n/a>                    SavePackage downloads never reach this state.
+    INITIAL_INTERNAL,
 
-  enum DownloadInternalState {
-    // Includes both before and after file name determination, and paused
-    // downloads.
-    // TODO(rdsmith): Put in state variable for file name determination.
-    // Transitions from:
-    //   <Initial creation>    Active downloads are created in this state.
-    //   RESUMING_INTERNAL
-    // Transitions to:
-    //   COMPLETING_INTERNAL   On final rename completion.
-    //   CANCELLED_INTERNAL    On cancel.
-    //   INTERRUPTED_INTERNAL  On interrupt.
-    //   COMPLETE_INTERNAL     On SavePackage download completion.
+    // Embedder is in the process of determining the target of the download.
+    // Since the embedder is sensitive to state transitions during this time,
+    // any DestinationError/DestinationCompleted events are deferred until
+    // TARGET_RESOLVED_INTERNAL.
+    //
+    // Transitions to (regular):
+    //   TARGET_RESOLVED_INTERNAL: Once the embedder invokes the callback.
+    //   CANCELLED_INTERNAL:      Cancelled.
+    //
+    // Transitions to (SavePackage):
+    //   <n/a>                    SavePackage downloads never reach this state.
+    TARGET_PENDING_INTERNAL,
+
+    // Embedder has completed target determination. It is now safe to resolve
+    // the download target as well as process deferred DestinationError events.
+    // This state is differs from TARGET_PENDING_INTERNAL due to it being
+    // allowed to transition to INTERRUPTED_INTERNAL, and it's different from
+    // IN_PROGRESS_INTERNAL in that entering this state doesn't require having
+    // a valid target.
+    //
+    // Transitions to (regular):
+    //   IN_PROGRESS_INTERNAL:    Target successfully determined. The incoming
+    //                            data stream can now be written to the target.
+    //   INTERRUPTED_INTERNAL:    Either the target determination or one of the
+    //                            deferred signals indicated that the download
+    //                            should be interrupted.
+    //   CANCELLED_INTERNAL:      User cancelled the download or there was a
+    //                            deferred Cancel() call.
+    //
+    // Transitions to (SavePackage):
+    //   <n/a>                    SavePackage downloads never reach this state.
+    TARGET_RESOLVED_INTERNAL,

[Randy Smith (Not in Mondays), 2016/02/12 00:03:11]

I said it on the other CL, but I think it's more appropriate in this CL: It
looks to me as if TARGET_RESOLVED_INTERNAL isn't really a state, in that it's
only the DII state transiently while DII code is executing on the UI thread.  If
having it makes reasoning about the code easier, I don't object (though I'd
sorta like to have it documented as transient here).  But if the object is never
left in the state while object code isn't running, that's not quite what I think
of as a state.

[asanka, 2016/02/12 05:04:26]

I believe the need for this state was established. Basically in its absence we'd
need to allow a transition from TARGET_PENDING to INTERRUPTED. The latter would
mean that we could introduce a regression where the download is interrupted
during target determination.

I added a comment about this state being transient.

[Randy Smith (Not in Mondays), 2016/02/12 17:37:02]

Acknowledged.

+
+    // Download target is known and the data can be transferred from our source
+    // to our sink.
+    //
+    // Transitions to (regular):
+    //   COMPLETING_INTERNAL:     On final rename completion.
+    //   CANCELLED_INTERNAL:      On cancel.
+    //   INTERRUPTED_INTERNAL:    On interrupt.
+    //
+    // Transitions to (SavePackage):
+    //   COMPLETE_INTERNAL:       On completion.
+    //   CANCELLED_INTERNAL:      On cancel.
     IN_PROGRESS_INTERNAL,
 
     // Between commit point (dispatch of download file release) and completed.
     // Embedder may be opening the file in this state.
-    // Transitions from:
-    //   IN_PROGRESS_INTERNAL
-    // Transitions to:
-    //   COMPLETE_INTERNAL     On successful completion.
+    //
+    // Transitions to (regular):
+    //   COMPLETE_INTERNAL:       On successful completion.
+    //
+    // Transitions to (SavePackage):
+    //   <n/a>                    SavePackage downloads never reach this state.
     COMPLETING_INTERNAL,
 
     // After embedder has had a chance to auto-open.  User may now open
     // or auto-open based on extension.
-    // Transitions from:
-    //   COMPLETING_INTERNAL
-    //   IN_PROGRESS_INTERNAL  SavePackage only.
-    //   <Initial creation>    Completed persisted downloads.
-    // Transitions to:
-    //   <none>                Terminal state.
+    //
+    // Transitions to (regular):
+    //   <none>                   Terminal state.
+    //
+    // Transitions to (SavePackage):
+    //   <none>                   Terminal state.
     COMPLETE_INTERNAL,
 
-    // User has cancelled the download.
-    // Transitions from:
-    //   IN_PROGRESS_INTERNAL
-    //   INTERRUPTED_INTERNAL
-    //   RESUMING_INTERNAL
-    //   <Initial creation>    Canceleld persisted downloads.
-    // Transitions to:
-    //   <none>                Terminal state.
-    CANCELLED_INTERNAL,
-
     // An error has interrupted the download.
-    // Transitions from:
-    //   IN_PROGRESS_INTERNAL
-    //   RESUMING_INTERNAL
-    //   <Initial creation>    Interrupted persisted downloads.
-    // Transitions to:
-    //   RESUMING_INTERNAL     On resumption.
+    //
+    // Transitions to (regular):
+    //   RESUMING_INTERNAL:       On resumption.
+    //   CANCELLED_INTERNAL:      On cancel.
+    //
+    // Transitions to (SavePackage):
+    //   <n/a>                    SavePackage downloads never reach this state.
     INTERRUPTED_INTERNAL,
 
     // A request to resume this interrupted download is in progress.
-    // Transitions from:
-    //   INTERRUPTED_INTERNAL
-    // Transitions to:
-    //   IN_PROGRESS_INTERNAL  Once a server response is received from a
-    //                         resumption.
-    //   INTERRUPTED_INTERNAL  If the resumption request fails.
-    //   CANCELLED_INTERNAL    On cancel.
+    //
+    // Transitions to (regular):
+    //   TARGET_PENDING_INTERNAL: Once a server response is received from a
+    //                            resumption.
+    //   CANCELLED_INTERNAL:      On cancel.
+    //
+    // Transitions to (SavePackage):
+    //   <n/a>                    SavePackage downloads never reach this state.
     RESUMING_INTERNAL,
 
+    // User has cancelled the download.
+    // TODO(asanka): Merge interrupted and cancelled states.
+    //
+    // Transitions to (regular):
+    //   <none>                   Terminal state.
+    //
+    // Transitions to (SavePackage):
+    //   <none>                   Terminal state.
+    CANCELLED_INTERNAL,
+
     MAX_DOWNLOAD_INTERNAL_STATE,
   };
 
   // Used with TransitionTo() to indicate whether or not to call
   // UpdateObservers() after the state transition.
   enum ShouldUpdateObservers {
     UPDATE_OBSERVERS,
     DONT_UPDATE_OBSERVERS
   };
 
@@ skipping 83 matching lines @@
   void ResumeInterruptedDownload();
 
   static DownloadState InternalToExternalState(
       DownloadInternalState internal_state);
   static DownloadInternalState ExternalToInternalState(
       DownloadState external_state);
 
   // Debugging routines --------------------------------------------------------
   static const char* DebugDownloadStateString(DownloadInternalState state);
   static const char* DebugResumeModeString(ResumeMode mode);
+  static bool IsValidSavePackageStateTransition(DownloadInternalState from,
+                                                DownloadInternalState to);
+  static bool IsValidStateTransition(DownloadInternalState from,
+                                     DownloadInternalState to);
 
   // Will be false for save package downloads retrieved from the history.
   // TODO(rdsmith): Replace with a generalized enum for "download source".
   const bool is_save_package_download_;
 
   // The handle to the request information.  Used for operations outside the
   // download system.
   scoped_ptr<DownloadRequestHandleInterface> request_handle_;
 
   uint32_t download_id_;
@@ skipping 154 matching lines @@
   const net::BoundNetLog bound_net_log_;
 
   base::WeakPtrFactory<DownloadItemImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadItemImpl);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_IMPL_H_