Closed
Bug 1185294
Opened 9 years ago
Closed 9 years ago
Private downloads get lost due to "cooldown" firefox restart
Categories
(Firefox :: General, defect)
Tracking
()
VERIFIED
FIXED
Firefox 42
People
(Reporter: nightgeorgy, Assigned: Gijs)
References
Details
(Keywords: dataloss, regression)
Attachments
(1 file)
40 bytes,
text/x-review-board-request
|
mossop
:
review+
ritu
:
approval-mozilla-beta+
|
Details |
Hi all, This issue is relevant to firefox 40 and newer. If an extension which needs browser restart is installed, firefox shows the relevant pop-up notification, prompting the user to perform the restart. This is normal. However, in case there is an active or paused private download(s), if the user proceed to restart firefox via this pop-up notification, the private download(s) will be lost after restart. This is due the lack of the warning dialog (like in firefox 39 and some earlier versions), which notifies/reminds about the private download, giving the chance to the user to cancel the restart attempt. How to reproduce: 1. Open a private window and start a download in it. 2. Before this download is finished, install an extension which needs browser restart. 3. Attempt to perform the restart via the pop-up notification. Actual result: Firefox gets restarted immediately, without any warning. As a result, the private download will have been lost after restart. Expected result: On attempt to restart, firefox should show the warning dialog about the private download(s), giving the chance for restart cancellation, like firefox 39 and some earlier versions do. Taking a look "under the hood", i found that the responsible code for this resides in "BrowserUtils.jsm", lines 29 - 38: > restartApplication: function() { > let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"] > .getService(Ci.nsIAppStartup); > //if already in safe mode restart in safe mode > if (Services.appinfo.inSafeMode) { > appStartup.restartInSafeMode(Ci.nsIAppStartup.eAttemptQuit | Ci.nsIAppStartup.eRestart); > return; > } > appStartup.quit(Ci.nsIAppStartup.eAttemptQuit | Ci.nsIAppStartup.eRestart); > }, My proposing patch: Modifying the original code above as follows, this issue is gone, restoring the warning dialog: > restartApplication: function() { > let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"] > .getService(Ci.nsIAppStartup); > + let cancelQuit = Cc["@mozilla.org/supports-PRBool;1"] > + .createInstance(Ci.nsISupportsPRBool); > + Services.obs.notifyObservers(cancelQuit, "quit-application-requested", "restart"); > + if(cancelQuit.data) { // The quit request has been canceled. > + return false; > + }; > //if already in safe mode restart in safe mode > if (Services.appinfo.inSafeMode) { > appStartup.restartInSafeMode(Ci.nsIAppStartup.eAttemptQuit | Ci.nsIAppStartup.eRestart); > return; > } > appStartup.quit(Ci.nsIAppStartup.eAttemptQuit | Ci.nsIAppStartup.eRestart); > }, I hadn't the time to investigate further, but most likely this restart function in "BrowserUtils.jsm" is called also in other cases too. Please note that, after the installation of the extension, if the browser restart is performed via the extension manager, we have the same behavior like firefox 39: in case there is a private download, firefox 40+ shows the warning dialog. In my opinion, a "cooldown" browser restart in not a good practice, because there are cases where some pending/active operations must not be interrupted before their completion. So, since an "accidental" restart is always possible, we need a warning dialog when it is necessary. Thanks
Updated•9 years ago
|
Severity: normal → critical
Status: UNCONFIRMED → NEW
status-firefox39:
--- → unaffected
status-firefox40:
--- → affected
status-firefox41:
--- → affected
status-firefox42:
--- → affected
Ever confirmed: true
Comment 1•9 years ago
|
||
[Tracking Requested - why for this release]: Regression
Reporter | ||
Comment 2•9 years ago
|
||
I just noticed this issue recently and i checked what versions are affected. This is not a regression because firefox 40 is still in beta stage, which means it can be improved furthermore, before its official release to the public. Also, as i mention in the beginning of my first post, this issue affects firefox 40, 41 and 42. The patch i proposed can be applied unchanged in all these affected versions of firefox.
Comment 3•9 years ago
|
||
Regression window: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=2c5b0df8a9d8&tochange=61da0ee2db1f REGRESSED BY: bc45ac6b031e Sushrut Girdhari — Bug 989307 - Make FUEL warn deprecation to the console on first usage. r=jaws
Blocks: 989307
Component: Private Browsing → General
Flags: needinfo?(developer)
Keywords: regressionwindow-wanted
Comment 4•9 years ago
|
||
The fix you propose comes from the original restart() method in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/exthelper/extApplication.js#695 I'm not sure why we didn't port that part of the code to the new method, we should. Do you want to make an actual patch for this issue?
Tracking as its a regression that leads to data loss.
Reporter | ||
Comment 6•9 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #4) > Do you want to make an actual patch for this issue? I'm not sure if i can make an actual patch, i haven't did it again. If it is necessary for this to be done by me, i could try but it will take some time until i find out how to do it. The code i suggest here as a patch, isn't enough for the developers to address this issue? As a complement to this issue, i would like also to note that, for the sake of order and better code structure, on every restart request should be called the same restart function. For example, as i mention in the beginning, if the browser restart after the installation of a non restartless extension is initiated via the extension manager, is called a different restart function (which works correctly in this case, notifying first the observers properly).
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Comment 7•9 years ago
|
||
This is clearly too late for 40. George, this should probably mentioned in the release notes, don't you think? If you agree, could you fill the following? Thanks Release Note Request (optional, but appreciated) [Why is this notable]: [Suggested wording]: [Links (documentation, blog post, etc)]:
Reporter | ||
Comment 8•9 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #7) > This is clearly too late for 40. > George, this should probably mentioned in the release notes, don't you think? > If you agree, could you fill the following? Thanks > > Release Note Request (optional, but appreciated) > [Why is this notable]: > [Suggested wording]: > [Links (documentation, blog post, etc)]: Hi Sylvestre, In my opinion, because this issue causes data loss under specific conditions as i describe above, if it should be mentioned in release notes of firefox 40, then it should be listed in "Known issues" section. However, for firefox 41 and later, it should be fixed for the same reason (data loss). It's very easy as i point out above. Actually, this small part of code i suggest, or something similar with the same functionality, has been omitted in firefox 40 by mistake ( i think).
Flags: needinfo?(nightgeorgy)
Updated•9 years ago
|
Severity: critical → major
Comment 9•9 years ago
|
||
Added to the release notes for 40 with "If a restart happens during a private download, the file could be lost" as wording. Don't hesitate to propose a better wording if you have one.
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #9) > Added to the release notes for 40 with "If a restart happens during a > private download, the file could be lost" as wording. Don't hesitate to > propose a better wording if you have one. "If you restart Firefox from an add-on install notification, we restart immediately without the usual checks for e.g. pending private browsing downloads." (I realize this is quite long, but I would like to be clear that (a) this only affects the add-on notification (I checked), and (b) there could be other ramifications than private downloads - basically, none of the observers for this shutdown notification thing are called, and so nobody can warn you or cancel shutdown or whatever.)
Assignee: nobody → gijskruitbosch+bugs
Flags: needinfo?(gijskruitbosch+bugs)
Comment 11•9 years ago
|
||
Gijs, I think we should remain on a description for users. They don't need to know about the usual checks being skipped. What about: "If Firefox is restarted from an add-on install notification, on-going private browsing downloads might be canceled without warning" I know I am only talking about the private downloads, I will update the release notes if we see more ramifications.
Assignee | ||
Comment 12•9 years ago
|
||
Bug 1185294 - dispatch quit-application-requested from BrowserUtils.jsm, patch by George Malamas, r?Mossop
Attachment #8643781 -
Flags: review?(dtownsend)
Comment 13•9 years ago
|
||
Comment on attachment 8643781 [details] MozReview Request: Bug 1185294 - dispatch quit-application-requested from BrowserUtils.jsm, patch by George Malamas, r?Mossop https://reviewboard.mozilla.org/r/15141/#review13599 Ship It!
Attachment #8643781 -
Flags: review?(dtownsend) → review+
https://hg.mozilla.org/mozilla-central/rev/0f220ddcc60b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Assignee | ||
Comment 16•9 years ago
|
||
Comment on attachment 8643781 [details] MozReview Request: Bug 1185294 - dispatch quit-application-requested from BrowserUtils.jsm, patch by George Malamas, r?Mossop Approval Request Comment [Feature/regressing bug #]: bug 989307 [User impact if declined]: we don't prompt for various things before you restart the browser [Describe test coverage new/current, TreeHerder]: no [Risks and why]: low, just putting some code back in that was omitted [String/UUID change made/needed]: no (asking for beta/aurora to get this in *41* depending on when I get approvals)
Attachment #8643781 -
Flags: approval-mozilla-beta?
Attachment #8643781 -
Flags: approval-mozilla-aurora?
Comment on attachment 8643781 [details] MozReview Request: Bug 1185294 - dispatch quit-application-requested from BrowserUtils.jsm, patch by George Malamas, r?Mossop Approved for uplift to Beta41 as the fix has been in Nightly for a few days so should be safe.
Attachment #8643781 -
Flags: approval-mozilla-beta?
Attachment #8643781 -
Flags: approval-mozilla-beta+
Attachment #8643781 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Flags: qe-verify+
Comment 19•9 years ago
|
||
Confirming that this issue is now fixed using: *Firefox 41.0b2, build ID: 20150817163452 *latest 42.0a2 Aurora, build ID: 20150818004007 A prompt is now displayed when the user is in Private mode and wants to restart the browser while he has an active download. The prompt is the following: http://i.imgur.com/cmOanNe.png Selecting "Don't Exit" will close the pop-up and selecting "Cancel x Donwnload(s)" will close the browser.
Status: RESOLVED → VERIFIED
QA Contact: cornel.ionce
Updated•7 years ago
|
Flags: needinfo?(developer)
You need to log in
before you can comment on or make changes to this bug.
Description
•