While testing bug 1137909 it seemed to me that downloads with blocked data aren't persisted across sessions, leaving untracked ".part" files around.

Temporary reduction in priority to keep team in scope for release.

I confirm that downloads are not persisted and the part file is forgotten after a browser restart. At a minimum, we'll need to persist the download in "downloads.json" until the blocked data is removed or the download is unblocked, keeping the information about the verdict that will be added in bug 1254100. Since in the current design we don't persist the verdict indication once a decision is made to unblock the download, we don't need to store this additional information in the browser history if the download is unblocked. However, if the block is confirmed, we might have to display a different icon based on the verdict type, so we might still need to store the information to handle this case, or use the same icon for all downloads blocked in the past.

Review commit: https://reviewboard.mozilla.org/r/40461/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40461/

Note that this change requires manual testing, we don't have regression tests across restarts.

(In reply to :Paolo Amadini from comment #4) > Note that this change requires manual testing, we don't have regression > tests across restarts. well, ideally now we have marionette.

(In reply to Marco Bonardo [::mak] from comment #5) > (In reply to :Paolo Amadini from comment #4) > > Note that this change requires manual testing, we don't have regression > > tests across restarts. > > well, ideally now we have marionette. To be clear, make a list of things that should be tested across restarts, and send it to :whimboo. Since now firefox UI tests (able to restart the browser) are in-tree, we can start collecting things we should be testing in those paths, and figure out if we can improve the process of adding new "ui" tests.

Comment on attachment 8731245 [details] MozReview Request: Bug 1139914 - Downloads with blocked data should be persisted across sessions. r=mak https://reviewboard.mozilla.org/r/40461/#review37283 off-hand, you may write a test without a real restart. Just simulate a download (so that metadata is stored) clear the session data, at that point the history downloads view should load from the stored metadata. ::: browser/components/downloads/content/allDownloadsViewOverlay.js:83 (Diff revision 1) > ? { becauseBlockedByParentalControls: true } > : metaData.state == nsIDM.DOWNLOAD_DIRTY > - ? { becauseBlockedByReputationCheck: true } > + ? { becauseBlockedByReputationCheck: true, > + reputationCheckVerdict: > + metaData.reputationCheckVerdict || "" } > : null; sorry, this starts being unreadable... What about a plain if/else if/else? I think we also have a (currently disabled) eslint rule to avoid nesting conditional operators

(In reply to Marco Bonardo [::mak] from comment #6) > To be clear, make a list of things that should be tested across restarts, > and send it to :whimboo. Since now firefox UI tests (able to restart the > browser) are in-tree, we can start collecting things we should be testing in > those paths, and figure out if we can improve the process of adding new "ui" > tests. It would be good to share the code that sets up the fake download sources and use it during the automated tests, whether they test the actual UI or the JSON storage.

Comment on attachment 8731245 [details] MozReview Request: Bug 1139914 - Downloads with blocked data should be persisted across sessions. r=mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40461/diff/1-2/

(In reply to Marco Bonardo [::mak] from comment #6) > To be clear, make a list of things that should be tested across restarts, > and send it to :whimboo. I filed bug 1258194 for these end-to-end tests, but I don't see this happening very soon given the complexity of such tests. I filed bug 1258195 about doing something simpler without a real restart like you suggested. Something even simpler can be done in bug 1258192 as unit tests checking the DownloadStore and DownloadIntegration behavior, although this would only deal with downloads stored in "downloads.json", such as blocked downloads that can be unblocked, and would not apply to anything in the Places database.

I'm not sure how to test this. All I see is that if before the fix, "unblock" option for a blocked download was gone after a browser restart, now it persists.

(In reply to Paul Silaghi, QA [:pauly] from comment #13) > I'm not sure how to test this. > All I see is that if before the fix, "unblock" option for a blocked download > was gone after a browser restart, now it persists. Yes, this is what we need to test for now. When we'll add new types of blocked downloads, we should ensure that their state also persists across restarts.

Is it expected that blocked downloads are still listed in the downloads panel after restart?

(In reply to Paul Silaghi, QA [:pauly] from comment #15) > Is it expected that blocked downloads are still listed in the downloads > panel after restart? Yes. Actually, we should probably expire them and delete the temporary file on disk after some time automatically, which we currently don't.

One more observation: the selection rectangle is not displayed in the downloads panel for the blocked downloads - let me know if this is not expected. Verified fixed using http://testsafebrowsing.appspot.com/ on FX 48.0a1 (2016-04-07) Win7, Ubuntu 14.04, OS X 10.10.5.