[Downloads] Enforce state transition integrity and state invariants.

content/browser/download/download_item_impl.h

Index: content/browser/download/download_item_impl.h
diff --git a/content/browser/download/download_item_impl.h b/content/browser/download/download_item_impl.h
index c527861287f8acf546e5822f8ae31c54ebae8f3a..3b9b76055d1181fe7e01afb91e1b9f595bbee184 100644
--- a/content/browser/download/download_item_impl.h
+++ b/content/browser/download/download_item_impl.h
@@ -218,71 +218,107 @@ class CONTENT_EXPORT DownloadItemImpl
 
  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,
 
-    // 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.
+    //   <Initial creation>        Interrupted persisted downloads.
+    //
     // Transitions to:
-    //   RESUMING_INTERNAL     On resumption.
+    //   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.
+    //   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:
+    //   <Initial creation>    Cancelled persisted downloads.
+    //
+    // Transitions to:
+    //   <none>                Terminal state.
+    CANCELLED_INTERNAL,
+
     MAX_DOWNLOAD_INTERNAL_STATE,
   };
 
@@ -386,6 +422,10 @@ class CONTENT_EXPORT DownloadItemImpl
   // 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".