[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 200 matching lines @@
 
   // 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.
+  // in INITIAL_INTERNAL state. However, downloads creates via history can be
+  // created in COMPLETE_INTERNAL, CANCELLED_INTERNAL and INTERRUPTED_INTERNAL.
+  //
+  // On debug builds, state transitions can be verified via
+  // IsValidStateTransition().
 
   enum DownloadInternalState {
-    // Includes both before and after file name determination, and paused
-    // downloads.
-    // TODO(rdsmith): Put in state variable for file name determination.
+    // Initial state. Regular downloads are created in this state until the
+    // Start() call is received.
+    //
     // Transitions from:
-    //   <Initial creation>    Active downloads are created in this state.
-    //   RESUMING_INTERNAL
+    //   <Initial creation>:      Active downloads are created in this state.
+    //
     // Transitions to:
-    //   COMPLETING_INTERNAL   On final rename completion.
-    //   CANCELLED_INTERNAL    On cancel.
-    //   INTERRUPTED_INTERNAL  On interrupt.
-    //   COMPLETE_INTERNAL     On SavePackage download completion.
+    //   TARGET_PENDING_INTERNAL: After a successful Start() call.
+    //   INTERRUPTED_INTERNAL:    Afater a failed Start() call.
+    INITIAL_INTERNAL,
+
+    // 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:
+    //   TARGET_RESOLVED_INTERNAL: Once the embedder invokes the callback.
+    //   CANCELLED_INTERNAL:       Cancelled.
+    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:
+    //   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.
+    TARGET_RESOLVED_INTERNAL,
+
+    // Download target is known and the data can be transferred from our source
+    // to our sink.
+    //
+    // Transitions from:

[Randy Smith (Not in Mondays), 2016/02/11 17:19:18]

I'm not sure how you're using this comment slot.  Naively, I'd think it would be
the dual of the "Transitions to:" slot, but that's not what's going on (e.g.
TARGET_RESOLVED_INTERNAL transitions here but isn't listed in this slow).  So
what does "Transitions from:" mean?

[asanka, 2016/02/11 21:12:05]

Yeah. That was confusing. I removed the "Transitions from" and explicitly called
out initial states at the top rather than next to each potential candidate. I
also broke out the SavePackage state transitions in to a separate section.

+    //   <Initial creation>:       Only for SavePackage downloads. Curses!

[Randy Smith (Not in Mondays), 2016/02/11 17:19:18]

I'm not clear whether or not the transitions described here are supposed to
include SP or not.  Specifically, the old transition diagram had an
IN_PROGRESS->COMPLETED line (which I think was SP) and the new one doesn't
(based on the Transitions to: lists).  Probably worth a comment if you're
intentionally excluding SP, or maybe two separate transition descriptions.

(BTW, I've got .dot for old and new diagrams that I'm happy to share; I'm
assuming you do as well, so not just sending.)

[asanka, 2016/02/11 21:12:05]

I clarified the SavePackage transitions in the comments. I failed to update the
comments, as you can see :). There has to be an easier way to manage .dot files,
or rather some better abstraction for state machines such that we can extract it
automatically.

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

I was musing about suggesting commenting the meanings of all of the states (as
you have) and representing the allowed transitions in .dot form, which I think
is actually a reasonably concise, clear textual way to represent state
transitions.  But I'm not sure how I feel about it, so I'll just mention that
here to think about for some future CL.

[asanka, 2016/02/12 20:21:24]

Acknowledged.

+    //
+    // Transitions to:
+    //   COMPLETING_INTERNAL:      On final rename completion.
+    //   CANCELLED_INTERNAL:       On cancel.
+    //   INTERRUPTED_INTERNAL:     On interrupt.
     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.
+    //   COMPLETE_INTERNAL:        On successful completion.
     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.
+    //   <Initial creation>        Completed persisted downloads.
+    //
     // Transitions to:
-    //   <none>                Terminal state.
+    //   <none>                    Terminal state.
     COMPLETE_INTERNAL,
 
+    // An error has interrupted the download.
+    //
+    // Transitions from:
+    //   <Initial creation>        Interrupted persisted downloads.
+    //
+    // Transitions to:
+    //   RESUMING_INTERNAL:        On resumption.
+    INTERRUPTED_INTERNAL,
+
+    // A request to resume this interrupted download is in progress.
+    //
+    // Transitions to:
+    //   TARGET_PENDING_INTERNAL:  Once a server response is received from a
+    //                             resumption.
+    //   CANCELLED_INTERNAL:       On cancel.
+    RESUMING_INTERNAL,
+
     // User has cancelled the download.
+    // TODO(asanka): Merge interrupted and cancelled states.
+    //
     // Transitions from:
-    //   IN_PROGRESS_INTERNAL
-    //   INTERRUPTED_INTERNAL
-    //   RESUMING_INTERNAL
-    //   <Initial creation>    Canceleld persisted downloads.
+    //   <Initial creation>    Cancelled 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.
-    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.
-    RESUMING_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_