Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1691543002:
[Downloads] Enforce state transition integrity and state invariants. (Closed)
Description[Downloads] Enforce state transition integrity and state invariants.
DownloadItemImpl is getting pretty complicated and it's hard to keep
track of possible races. This CL tries to make things a little easier by
constraining state transitions and also asserting the state of the
DownloadItem as each state is entered.
This CL introduces two new intermediate states to DownloadItemImpl:
- TARGET_PENDING_INTERNAL
- TARGET_RESOLVED_INTERNAL
Of these, only TARGET_RESOLVED_INTERNAL is allowed to transition to an
interrupted state. TARGET_PENDING_INTERNAL is entered when the download
target determination cascade begins and defers any interruptions until
TARGET_RESOLVED_INTERNAL. This state also blocks download completion.
Hence transitions to INTERRUPTED and COMPLETED states must now step over
the target determination state.
Also introduce a set of tests that are designed to exercise the various
permutations of events that can occur during the target determination
phase.
BUG=7648
Committed: https://crrev.com/45579ca4f8334797d90ea510c7a74dee41e96f3b
Cr-Commit-Position: refs/heads/master@{#375275}
Patch Set 1 #Patch Set 2 : Add a state transition map for SavePackage. #
Total comments: 10
Patch Set 3 : Clarify comments. #
Total comments: 48
Patch Set 4 : Address comments and fix DownloadsExtensionTest #
Total comments: 3
Patch Set 5 : #
Total comments: 4
Patch Set 6 : Address comments #
Dependent Patchsets: Messages
Total messages: 18 (6 generated)
asanka@chromium.org changed reviewers: + rdsmith@chromium.org
This is the CL that will block the state transition rework.
I think I'm going to send initial comments here and switch back to the other CL, since a reasonable amount of this CL is going to be analyzing the stated transition diagram and then validating it against the code, and I have a couple of questions about what transition diagram you *meant* to specify :-}. Not blocked, all my time are belong to you/these reviews, but context switching to maximize bandwidth. https://codereview.chromium.org/1691543002/diff/20001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/1691543002/diff/20001/content/browser/downloa... content/browser/download/download_item_impl.cc:1044: // updates in a non-in-progress state. This issue should be readdressed The existence of a phrase "This issue should be readdressed" makes me think that there should be a TODO somewhere. https://codereview.chromium.org/1691543002/diff/20001/content/browser/downloa... content/browser/download/download_item_impl.cc:1164: // download target determination process. I don't follow "Transition out to IN_PROGRESS"; this os going TARGET_RESOLVED -> INTERRUPTED, right? https://codereview.chromium.org/1691543002/diff/20001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/1691543002/diff/20001/content/browser/downloa... content/browser/download/download_item_impl.h:269: // Transitions from: 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? https://codereview.chromium.org/1691543002/diff/20001/content/browser/downloa... content/browser/download/download_item_impl.h:270: // <Initial creation>: Only for SavePackage downloads. Curses! 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.)
https://codereview.chromium.org/1691543002/diff/20001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/1691543002/diff/20001/content/browser/downloa... content/browser/download/download_item_impl.cc:1044: // updates in a non-in-progress state. This issue should be readdressed On 2016/02/11 17:19:18, Randy Smith - Not in Fridays wrote: > The existence of a phrase "This issue should be readdressed" makes me think that > there should be a TODO somewhere. Done. https://codereview.chromium.org/1691543002/diff/20001/content/browser/downloa... content/browser/download/download_item_impl.cc:1164: // download target determination process. On 2016/02/11 17:19:18, Randy Smith - Not in Fridays wrote: > I don't follow "Transition out to IN_PROGRESS"; this os going TARGET_RESOLVED -> > INTERRUPTED, right? Yup :) Done https://codereview.chromium.org/1691543002/diff/20001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/1691543002/diff/20001/content/browser/downloa... content/browser/download/download_item_impl.h:269: // Transitions from: On 2016/02/11 17:19:18, Randy Smith - Not in Fridays wrote: > 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? 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. https://codereview.chromium.org/1691543002/diff/20001/content/browser/downloa... content/browser/download/download_item_impl.h:270: // <Initial creation>: Only for SavePackage downloads. Curses! On 2016/02/11 17:19:18, Randy Smith - Not in Fridays wrote: > 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.) 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.
Ok, this looks basically good, though several comments below. But I presume I can stamp on the next round. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... File content/browser/download/download_item_impl.cc (left): https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:411: if (!is_save_package_download_ && download_file_) Interesting. Was the is_save_package_download_ check vestigial? I can't come up with a reason why it would have been needed before this CL, so I presume removing it was just a cleanup? https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:386: return; suggestion: DCHECK(!is_paused_)? https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:512: case COMPLETING_INTERNAL: Not in this CL (as not changed in this CL): I find this contradictory with the external state for this state being COMPLETE. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:864: // Cnly support resumption for HTTP(S). Interesting. This seems like a semantic change that isn't really part of this CL; what am I missing? https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:1009: state_ == TARGET_RESOLVED_INTERNAL || state_ == IN_PROGRESS_INTERNAL); DestinationCompleted comes in via post from the file thread, right? So why couldn't we be interrupted when it arrives? (I.e. I'm wondering if removing the conditional in DestinationCompleted() might be a bug.) https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:1076: if (state_ != IN_PROGRESS_INTERNAL) Confirming I understand (if this is true, I'm ok with it): For this CL, the paths being empty is equivalent to being in TARGET_PENDING/RESOLVED, but for the next CL resumption would have allowed those variables to be set even if target determination hadn't completed, therefore we need to actually base the conditional on the state variable? https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:1492: current_path_.clear(); Am I right in thinking that if this path has been taken, the download_file_ will be null, and thus the download file logic below won't be executed? If so, willing to put in a DCHECK? I spent a bit of time puzzling over this code trying to figure out how this PostTask and the below ReleaseDownloadFile() interacted with each other, and if they don't it would be nice to state that. I'm tempted to suggest you put all the download file handling in this routine in one place and cascade it to make it even clearer that only one of these paths can be taken, but I'm happy to go with your judgement as to whether or not that would be clearer. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:1603: void DownloadItemImpl::TransitionTo(DownloadInternalState new_state, Nice invariant checking. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:1688: bool is_done = nit, suggestion: I'm feeling tempted to suggest inclusive rather than exclusive state testing here (i.e. list the states that are final states, not the ones that aren't). https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:1855: #if DCHECK_IS_ON() nit, suggestion: I wince a bit at declaration a routine and not defining it (as will happen in release builds AIUTC); maybe either just null out the inside of these routines to always return true (or false) or put this #if in the .h file as well? https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:1872: return to == CANCELLED_INTERNAL || COMPLETE_INTERNAL; I think you were typing a little too fast here :-}. (i.e. ITYM "return (to == CANCELLED_INTERNAL) || (to == COMPLETE_INTERNAL);") https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.h:279: TARGET_RESOLVED_INTERNAL, 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. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... File content/browser/download/download_item_impl_unittest.cc (right): https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1377: namespace { Why? It seems inconsistent with the rest of the file. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1378: It might be useful to have a motivating comment before diving into the infrastructure. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1397: using EventList = std::deque<ObservationSet>; This is a bit confusing, given that the EventList has no way to specify what the event was. I presume that means that exactly what the event is is context dependent and specified by ordering within that context? https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1431: // * Each ObservationSet in each EventList contains a subrange of observations suggest: "... subrange (possibly empty) of observations ..." https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1439: // list of observations can be distributed into |event_count| events. I'm tempted to title this "Thor's hammer" except that hammer was very powerful, not immensely large :-}. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1450: for (const auto other_bins : subsequent_bins) { Reference? It's copied anyway below (IIUC). https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1487: all_observations.end(), 3); Where does event_count == 3 come from? Isn't that dependent on the caller? Or is there some link between the event count and the number of observations? ETA: Ah, looks like all your tests have the same three events. Maybe worthwhile defining that above and using a constant here? https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1554: TEST_P(DownloadItemDestinationUpdateRaceTest, DumpParameters) { Quick comment before each test sketching out what it does? https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1585: TEST_P(DownloadItemDestinationUpdateRaceTest, InitDownloadFileFails) { I'm sure there's something I'm missing, but is there a reason to run this test for each param? It looks like it does the same thing no matter what parameter it gets.
asanka@chromium.org changed reviewers: + benjhayden@chromium.org
+benjhayden: Could you look at downloads_api_browsertest.cc? https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... File content/browser/download/download_item_impl.cc (left): https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:411: if (!is_save_package_download_ && download_file_) On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > Interesting. Was the is_save_package_download_ check vestigial? I can't come > up with a reason why it would have been needed before this CL, so I presume > removing it was just a cleanup? It was cleanup. I don't know why it was there either, but in its current form, it wasn't doing anything since SavePackage downloads don't have a download_file_. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:386: return; On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > suggestion: DCHECK(!is_paused_)? Done. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:512: case COMPLETING_INTERNAL: On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > Not in this CL (as not changed in this CL): I find this contradictory with the > external state for this state being COMPLETE. Acknowledged. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:864: // Cnly support resumption for HTTP(S). On 2016/02/12 00:03:10, Randy Smith - Not in Fridays wrote: > Interesting. This seems like a semantic change that isn't really part of this > CL; what am I missing? I moved this check from CanResume() to here. Earlier even though GetResumeMode() returns a valid mode, CanResume() will block based on the URL scheme and whether resumption is enabled. Now GetResumeMode() will return RESUME_MODE_INVALID if the download is in a state where it cannot be resumed. Previously, we could leave a partial file on disk on the false assumption that it could be continued when it could not because the only gate for deciding whether to delete the partial file is the result of calling GetResumeMode(). The set of states where resumption is allowed and their effective modes should be the same. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:1009: state_ == TARGET_RESOLVED_INTERNAL || state_ == IN_PROGRESS_INTERNAL); On 2016/02/12 00:03:10, Randy Smith - Not in Fridays wrote: > DestinationCompleted comes in via post from the file thread, right? So why > couldn't we be interrupted when it arrives? (I.e. I'm wondering if removing the > conditional in DestinationCompleted() might be a bug.) This one is a bit subtle. Basically, if the DII transitions to interrupted or cancel or complete, we call ReleaseDownloadFile, which unconditionally invalidates the weak pointers. This effectively cuts off any destination observer callbacks at the point where we release the download file. I moved the DCHECKs to the destination observer methods along with a comment explaining why we don't expect callbacks in any other state. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:1076: if (state_ != IN_PROGRESS_INTERNAL) On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > Confirming I understand (if this is true, I'm ok with it): For this CL, the > paths being empty is equivalent to being in TARGET_PENDING/RESOLVED, but for the > next CL resumption would have allowed those variables to be set even if target > determination hadn't completed, therefore we need to actually base the > conditional on the state variable? Correct. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:1492: current_path_.clear(); On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > Am I right in thinking that if this path has been taken, the download_file_ will > be null, and thus the download file logic below won't be executed? If so, > willing to put in a DCHECK? I spent a bit of time puzzling over this code > trying to figure out how this PostTask and the below ReleaseDownloadFile() > interacted with each other, and if they don't it would be nice to state that. > I'm tempted to suggest you put all the download file handling in this routine in > one place and cascade it to make it even clearer that only one of these paths > can be taken, but I'm happy to go with your judgement as to whether or not that > would be clearer. I moved the download_file_ handling to one place, but then it wasn't quite clear what was going on. Instead I moved the 'if (download_file_) bit to the IN_PROGRESS branch above and removed the condition. The IN_PROGRESS states should always have the download_file_ intact. And then I left the above code as-is. That way it should be clear that the two code paths are mutually exclusive. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:1603: void DownloadItemImpl::TransitionTo(DownloadInternalState new_state, On 2016/02/12 00:03:10, Randy Smith - Not in Fridays wrote: > Nice invariant checking. Thanks! https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:1688: bool is_done = On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > nit, suggestion: I'm feeling tempted to suggest inclusive rather than exclusive > state testing here (i.e. list the states that are final states, not the ones > that aren't). Done. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:1855: #if DCHECK_IS_ON() On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > nit, suggestion: I wince a bit at declaration a routine and not defining it (as > will happen in release builds AIUTC); maybe either just null out the inside of > these routines to always return true (or false) or put this #if in the .h file > as well? Opted to put the #if in the .h file. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:1872: return to == CANCELLED_INTERNAL || COMPLETE_INTERNAL; On 2016/02/12 00:03:10, Randy Smith - Not in Fridays wrote: > I think you were typing a little too fast here :-}. (i.e. ITYM "return (to == > CANCELLED_INTERNAL) || (to == COMPLETE_INTERNAL);") Hehe :) Done. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.h:279: TARGET_RESOLVED_INTERNAL, On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > 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. 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. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... File content/browser/download/download_item_impl_unittest.cc (right): https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1377: namespace { On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > Why? It seems inconsistent with the rest of the file. Hmm? The namespace only covers the set of definitions below. I placed them here next to the test fixture so that it would be easier to follow. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1378: On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > It might be useful to have a motivating comment before diving into the > infrastructure. Done. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1397: using EventList = std::deque<ObservationSet>; On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > This is a bit confusing, given that the EventList has no way to specify what the > event was. I presume that means that exactly what the event is is context > dependent and specified by ordering within that context? Yeah. It's the ordering. I'll make a note of it. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1431: // * Each ObservationSet in each EventList contains a subrange of observations On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > suggest: "... subrange (possibly empty) of observations ..." Done. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1439: // list of observations can be distributed into |event_count| events. On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > I'm tempted to title this "Thor's hammer" except that hammer was very powerful, > not immensely large :-}. Hehe. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1450: for (const auto other_bins : subsequent_bins) { On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > Reference? It's copied anyway below (IIUC). Done. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1487: all_observations.end(), 3); On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > Where does event_count == 3 come from? Isn't that dependent on the caller? Or > is there some link between the event count and the number of observations? > > ETA: Ah, looks like all your tests have the same three events. Maybe worthwhile > defining that above and using a constant here? Gave it a name. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1554: TEST_P(DownloadItemDestinationUpdateRaceTest, DumpParameters) { On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > Quick comment before each test sketching out what it does? Done. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1585: TEST_P(DownloadItemDestinationUpdateRaceTest, InitDownloadFileFails) { On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > I'm sure there's something I'm missing, but is there a reason to run this test > for each param? It looks like it does the same thing no matter what parameter > it gets. Yeah, this one was wasteful. I moved it to a regular DownloadItemTest.
https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:1855: #if DCHECK_IS_ON() On 2016/02/12 05:04:26, asanka wrote: > On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > > nit, suggestion: I wince a bit at declaration a routine and not defining it > (as > > will happen in release builds AIUTC); maybe either just null out the inside of > > these routines to always return true (or false) or put this #if in the .h file > > as well? > > Opted to put the #if in the .h file. Well, that didn't. Apparently the symbol still needs to be declared for some reason I was too tired to figure out. Went with #if inside the definition. https://codereview.chromium.org/1691543002/diff/60001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/1691543002/diff/60001/content/browser/downloa... content/browser/download/download_item_impl.cc:1498: DCHECK_EQ(DOWNLOAD_INTERRUPT_REASON_USER_CANCELED, reason); SavePackage also hits this code path. I removed the DCHECK.
Description was changed from ========== [Downloads] Enforce state transition integrity and state invariants. DownloadItemImpl is getting pretty complicated and it's hard to keep track of possible races. This CL tries to make things a little easier by contraining state transitions and also asserting the state of the DownloadItem as each state is entered. This CL introduces two new intermediate states to DownloadItemImpl: - TARGET_PENDING_INTERNAL - TARGET_RESOLVED_INTERNAL Of these, only TARGET_RESOLVED_INTERNAL is allowed to transition to an interrupted state. TARGET_PENDING_INTERNAL is entered when the download target determination cascade begins and defers any interruptions until TARGET_RESOLVED_INTERNAL. This state also blocks download completion. Hence transitions to INTERRUPTED and COMPLETED states must now step over the target determination state. Also introduce a set of tests that are designed to exercise the various permutations of events that can occur during the target determination phase. BUG=7648 ========== to ========== [Downloads] Enforce state transition integrity and state invariants. DownloadItemImpl is getting pretty complicated and it's hard to keep track of possible races. This CL tries to make things a little easier by constraining state transitions and also asserting the state of the DownloadItem as each state is entered. This CL introduces two new intermediate states to DownloadItemImpl: - TARGET_PENDING_INTERNAL - TARGET_RESOLVED_INTERNAL Of these, only TARGET_RESOLVED_INTERNAL is allowed to transition to an interrupted state. TARGET_PENDING_INTERNAL is entered when the download target determination cascade begins and defers any interruptions until TARGET_RESOLVED_INTERNAL. This state also blocks download completion. Hence transitions to INTERRUPTED and COMPLETED states must now step over the target determination state. Also introduce a set of tests that are designed to exercise the various permutations of events that can occur during the target determination phase. BUG=7648 ==========
downloads_api_browsertest lgtm
LGTM; you can do what you want with all of my comments except that you have to spell "rename" correctly :-}. https://codereview.chromium.org/1691543002/diff/20001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/1691543002/diff/20001/content/browser/downloa... content/browser/download/download_item_impl.h:270: // <Initial creation>: Only for SavePackage downloads. Curses! On 2016/02/11 21:12:05, asanka wrote: > On 2016/02/11 17:19:18, Randy Smith - Not in Fridays wrote: > > 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.) > > 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. 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. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:1009: state_ == TARGET_RESOLVED_INTERNAL || state_ == IN_PROGRESS_INTERNAL); On 2016/02/12 05:04:26, asanka wrote: > On 2016/02/12 00:03:10, Randy Smith - Not in Fridays wrote: > > DestinationCompleted comes in via post from the file thread, right? So why > > couldn't we be interrupted when it arrives? (I.e. I'm wondering if removing > the > > conditional in DestinationCompleted() might be a bug.) > > This one is a bit subtle. Basically, if the DII transitions to interrupted or > cancel or complete, we call ReleaseDownloadFile, which unconditionally > invalidates the weak pointers. This effectively cuts off any destination > observer callbacks at the point where we release the download file. > > I moved the DCHECKs to the destination observer methods along with a comment > explaining why we don't expect callbacks in any other state. Ah, thank you. However, as I read the code, what you say isn't precisely true. In the Interrupt/Cancel case, we only call ReleaseDownloadFile download_file_ is not null. I don't think that's a problem, since the code only sets it to null in ReleaseDownloadFile(), but I wanted to mention the very slight imprecision in case you saw a place where it might be a problem. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.h:279: TARGET_RESOLVED_INTERNAL, On 2016/02/12 05:04:26, asanka wrote: > On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > > 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. > > 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. Acknowledged. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... File content/browser/download/download_item_impl_unittest.cc (right): https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1377: namespace { On 2016/02/12 05:04:27, asanka wrote: > On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > > Why? It seems inconsistent with the rest of the file. > > Hmm? The namespace only covers the set of definitions below. I placed them here > next to the test fixture so that it would be easier to follow. Ah, ignore me; it's a combination of a prejudice against anonymous namespaces other than at the top of the file and a bias towards having tests at file scope but not in an anonymous namespace, neither one of which I can really defend. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1378: On 2016/02/12 05:04:27, asanka wrote: > On 2016/02/12 00:03:11, Randy Smith - Not in Fridays wrote: > > It might be useful to have a motivating comment before diving into the > > infrastructure. > > Done. Nice, thank you. https://codereview.chromium.org/1691543002/diff/60001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/1691543002/diff/60001/content/browser/downloa... content/browser/download/download_item_impl.cc:1498: DCHECK_EQ(DOWNLOAD_INTERRUPT_REASON_USER_CANCELED, reason); On 2016/02/12 15:53:37, asanka wrote: > SavePackage also hits this code path. I removed the DCHECK. It's ok either way, but any reason not to just make the DCHECK dependent on SP as well? https://codereview.chromium.org/1691543002/diff/80001/content/browser/downloa... File content/browser/download/download_item_impl_unittest.cc (right): https://codereview.chromium.org/1691543002/diff/80001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1631: // during download target determination. nit: I could read this as meaning either "the user cancels through the shelf during DTD" or "the user cancels at a prompt thrown up by DTD". Maybe make the comment more specific? https://codereview.chromium.org/1691543002/diff/80001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1731: // determination and intermediate renam all succeed. nit: rename
Thanks Randy and Ben! (hey Ben! there's another one!) https://codereview.chromium.org/1691543002/diff/20001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/1691543002/diff/20001/content/browser/downloa... content/browser/download/download_item_impl.h:270: // <Initial creation>: Only for SavePackage downloads. Curses! On 2016/02/12 17:37:02, Randy Smith - Not in Fridays wrote: > On 2016/02/11 21:12:05, asanka wrote: > > On 2016/02/11 17:19:18, Randy Smith - Not in Fridays wrote: > > > 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.) > > > > 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. > > 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. Acknowledged. https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/1691543002/diff/40001/content/browser/downloa... content/browser/download/download_item_impl.cc:1009: state_ == TARGET_RESOLVED_INTERNAL || state_ == IN_PROGRESS_INTERNAL); On 2016/02/12 17:37:02, Randy Smith - Not in Fridays wrote: > On 2016/02/12 05:04:26, asanka wrote: > > On 2016/02/12 00:03:10, Randy Smith - Not in Fridays wrote: > > > DestinationCompleted comes in via post from the file thread, right? So why > > > couldn't we be interrupted when it arrives? (I.e. I'm wondering if removing > > the > > > conditional in DestinationCompleted() might be a bug.) > > > > This one is a bit subtle. Basically, if the DII transitions to interrupted or > > cancel or complete, we call ReleaseDownloadFile, which unconditionally > > invalidates the weak pointers. This effectively cuts off any destination > > observer callbacks at the point where we release the download file. > > > > I moved the DCHECKs to the destination observer methods along with a comment > > explaining why we don't expect callbacks in any other state. > > Ah, thank you. However, as I read the code, what you say isn't precisely true. > In the Interrupt/Cancel case, we only call ReleaseDownloadFile download_file_ is > not null. I don't think that's a problem, since the code only sets it to null > in ReleaseDownloadFile(), but I wanted to mention the very slight imprecision in > case you saw a place where it might be a problem. Yeah. We rely on download_file_ only being set to null in ReleaseDownloadFile. It's a scoped_ptr and needs to be destroyed on the FILE thread (enforced via DCHECKs). Setting download_file_ to null isn't trivial. I think it's fair to rely on ReleaseDownloadFile being the only place where download_file_ is discarded. https://codereview.chromium.org/1691543002/diff/60001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/1691543002/diff/60001/content/browser/downloa... content/browser/download/download_item_impl.cc:1498: DCHECK_EQ(DOWNLOAD_INTERRUPT_REASON_USER_CANCELED, reason); On 2016/02/12 17:37:02, Randy Smith - Not in Fridays wrote: > On 2016/02/12 15:53:37, asanka wrote: > > SavePackage also hits this code path. I removed the DCHECK. > > It's ok either way, but any reason not to just make the DCHECK dependent on SP > as well? It tipped the scales against the assertion being useful. I.e. it would be important to know if download_file_ is always non-null here, but once there's more than one reason why it wouldn't, then enforcing those causes via DCHECKs becomes of questionable utility. I opted to not enforce it. https://codereview.chromium.org/1691543002/diff/80001/content/browser/downloa... File content/browser/download/download_item_impl_unittest.cc (right): https://codereview.chromium.org/1691543002/diff/80001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1631: // during download target determination. On 2016/02/12 17:37:02, Randy Smith - Not in Fridays wrote: > nit: I could read this as meaning either "the user cancels through the shelf > during DTD" or "the user cancels at a prompt thrown up by DTD". Maybe make the > comment more specific? I made it say "the embedder cancels..." since this code doesn't have a direct relationship with the user. https://codereview.chromium.org/1691543002/diff/80001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1731: // determination and intermediate renam all succeed. On 2016/02/12 17:37:02, Randy Smith - Not in Fridays wrote: > nit: rename Done.
The CQ bit was checked by asanka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benjhayden@chromium.org, rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/1691543002/#ps100001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1691543002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1691543002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Downloads] Enforce state transition integrity and state invariants. DownloadItemImpl is getting pretty complicated and it's hard to keep track of possible races. This CL tries to make things a little easier by constraining state transitions and also asserting the state of the DownloadItem as each state is entered. This CL introduces two new intermediate states to DownloadItemImpl: - TARGET_PENDING_INTERNAL - TARGET_RESOLVED_INTERNAL Of these, only TARGET_RESOLVED_INTERNAL is allowed to transition to an interrupted state. TARGET_PENDING_INTERNAL is entered when the download target determination cascade begins and defers any interruptions until TARGET_RESOLVED_INTERNAL. This state also blocks download completion. Hence transitions to INTERRUPTED and COMPLETED states must now step over the target determination state. Also introduce a set of tests that are designed to exercise the various permutations of events that can occur during the target determination phase. BUG=7648 ========== to ========== [Downloads] Enforce state transition integrity and state invariants. DownloadItemImpl is getting pretty complicated and it's hard to keep track of possible races. This CL tries to make things a little easier by constraining state transitions and also asserting the state of the DownloadItem as each state is entered. This CL introduces two new intermediate states to DownloadItemImpl: - TARGET_PENDING_INTERNAL - TARGET_RESOLVED_INTERNAL Of these, only TARGET_RESOLVED_INTERNAL is allowed to transition to an interrupted state. TARGET_PENDING_INTERNAL is entered when the download target determination cascade begins and defers any interruptions until TARGET_RESOLVED_INTERNAL. This state also blocks download completion. Hence transitions to INTERRUPTED and COMPLETED states must now step over the target determination state. Also introduce a set of tests that are designed to exercise the various permutations of events that can occur during the target determination phase. BUG=7648 Committed: https://crrev.com/45579ca4f8334797d90ea510c7a74dee41e96f3b Cr-Commit-Position: refs/heads/master@{#375275} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/45579ca4f8334797d90ea510c7a74dee41e96f3b Cr-Commit-Position: refs/heads/master@{#375275} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
