Private downloads get lost due to "cooldown" firefox restart

VERIFIED FIXED in Firefox 41

Status

()

--
major
VERIFIED FIXED
3 years ago
a year ago

People

(Reporter: nightgeorgy, Assigned: Gijs)

Tracking

({dataloss, regression})

40 Branch
Firefox 42
dataloss, regression
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox39 unaffected, firefox40+ wontfix, firefox41+ verified, firefox42+ verified, relnote-firefox 40+)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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
Severity: normal → critical
Status: UNCONFIRMED → NEW
status-firefox39: --- → unaffected
status-firefox40: --- → affected
status-firefox41: --- → affected
status-firefox42: --- → affected
Ever confirmed: true
Keywords: dataloss, regression, regressionwindow-wanted
[Tracking Requested - why for this release]: Regression
tracking-firefox40: --- → ?
tracking-firefox41: --- → ?
tracking-firefox42: --- → ?
(Reporter)

Comment 2

3 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

3 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
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.
tracking-firefox40: ? → +
tracking-firefox41: ? → +
tracking-firefox42: ? → +
(Reporter)

Comment 6

3 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

3 years ago
Flags: needinfo?(gijskruitbosch+bugs)
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)]:
status-firefox40: affected → wontfix
relnote-firefox: --- → ?
Flags: needinfo?(nightgeorgy)
(Reporter)

Comment 8

3 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)
Severity: critical → major
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.
relnote-firefox: ? → 40+
(Assignee)

Comment 10

3 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)
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

3 years ago
Created attachment 8643781 [details]
MozReview Request: Bug 1185294 - dispatch quit-application-requested from BrowserUtils.jsm, patch by George Malamas, r?Mossop

Bug 1185294 - dispatch quit-application-requested from BrowserUtils.jsm, patch by George Malamas, r?Mossop
Attachment #8643781 - Flags: review?(dtownsend)
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
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
(Assignee)

Comment 16

3 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?
Flags: qe-verify+
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
status-firefox41: fixed → verified
status-firefox42: fixed → verified
QA Contact: cornel.ionce
Duplicate of this bug: 1151539

Updated

a year ago
Flags: needinfo?(developer)
You need to log in before you can comment on or make changes to this bug.