Open Bug 1621678 Opened 4 years ago Updated 21 days ago

Warning pop-up is not displayed while trying to close Firefox during an ongoing download process if the browser console is open

Categories

(Toolkit :: Downloads API, defect, P3)

defect

Tracking

()

Tracking Status
firefox-esr68 --- wontfix
firefox73 --- wontfix
firefox74 --- wontfix
firefox75 --- wontfix
firefox76 --- affected
firefox79 --- affected
firefox80 --- affected
firefox81 --- affected

People

(Reporter: emilghitta, Unassigned, Mentored)

References

Details

(Whiteboard: [outreachy-2021-downloads])

Attachments

(2 files)

Affected versions

  • Firefox 76.0a1 (BuildId:20200311041149)
  • Firefox 75.0b2 (BuildId:20200310192828)
  • Firefox 74.0 (BuildId:20200309095159)
  • Firefox 68.6.0esr (BuildId:20200305175243)

Affected platforms

  • Windows 10 64bit.
  • Windows 7 64bit.
  • Ubuntu 18.04 64bit

Unaffected platforms

  • macOS 10.14

Steps to reproduce

  1. Launch Firefox.
  2. Access the following link.
  3. Start a file download that has >= 200 MB (Doesn’t really matter the size but gives time to encounter this behavior).
  4. Launch the browser console.
  5. Close all Firefox tabs or close Firefox via the close (x) button.
  6. Close the browser console.

Expected result

  • A warning pop-up informs the user that by closing the browser the downloads are cancelled.

Actual result

  • No warning pop-up is displayed, Firefox closes and the download fails.

Regression Range

  • This goes way back to 2013 builds.

Notes

  • This issue is not reproducible with the old “Error Console”. With older builds that had the “Error Console”, the warning pop-up was displayed after trying to close the console (if Firefox main window was closed beforehand).
  • This doesn’t seem reproducible on Private Browsing mode. Trying to close the Firefox private browsing mode (while a download is initiated from it) displays the warning pop-up even though the browser console is open.

comment #0 suggests macOS is affected, but on mac after step 6 the process is not closed so I'd expect downloads to be uninterrupted. Unless step 6 is really mean to be "quit" ?

Flags: needinfo?(emil.ghitta)
Attached image windows.gif

Hi Gijs,

Confirmed that this is indeed not reproducible on macOS since the process doesn't close after clicking the X button or after closing all tabs. Closing Firefox on macOS via "Quit" successfully displays the warning message.

Updating the comment 0 and adding a note in step 5 that this is reproducible by closing all tabs or via the x button. Trying to close Firefox via the hamburger menu successfully displays the message in both Windows and Ubuntu platforms.

Sorry for the confusion, this may have slipped somehow.

Attaching a screencast with this behavior on Windows.

Flags: needinfo?(emil.ghitta)

The priority flag is not set for this bug.
:mak, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(mak)
Flags: needinfo?(mak) → needinfo?(gijskruitbosch+bugs)
Priority: -- → P3

So as filed, this is not a particularly severe bug. However... this affects other non-browser windows, too, not just the browser console (e.g. the library / "downloads window" !)

The reason is that browser windows have onclose event handlers that call WindowIsClosing which calls closeWindow, which checks with the window enumerator if we're the last window alive, and if so, fires the quit-application-requested observer notification on all but macOS. If anything from that notification complains, we don't close the window.

This notification is what the Downloads code hooks to check for in-progress downloads.

The browser console and places window do not have a close event handler and do not fire this notification. I haven't checked other windows that aren't modal dialogs (there probably exist a few that I'm forgetting about right now? Maybe same-site browsers, if/when they share a process? Definitely the FXR window, right now.).
This also means that other consumers of quit-application-requested are missing out on being told we're quitting, which could have other unfortunate consequences, depending on what those observers are doing.

I'm not sure how to proceed. The simple fix is to just add onclose handlers on those other windows, too, but that seems far too stopgap and will just fail the next time we come up with some other window.

There's no global way to listen for these close events except via the window watcher + adding/removing close handlers on every window... We could try that, I guess? Or we could add an observer notification fired by the code that fires the event. Dave, what do you think?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dtownsend)

This whole quit-application-requested thing has always been messy and there are probably a bunch of places where we still don't send it out as it is entirely optional. By default the platform "helpfully" initiates shutdown as soon as the last non-hidden window goes away. When a XUL window is destroyed we decrement the count of open windows and if it is zero initiate shutdown, with the first notification "quit-application-granted".

We'd be in a much better place if an attempt to close that last window first dispatched quit-application-granted automatically and cancelled the close if the request was denied. We could do that from frontend as you say, but the actual count of the number of opened windows is inaccessible in nsAppStartup and I'd be worried about getting out of sync somehow.

The obvious approach would be to have the window close path (https://searchfox.org/mozilla-central/rev/064b0f9501ad76802853b43f18e33d8713fd54d3/dom/base/nsGlobalWindowOuter.cpp#6111), which already does some checks about whether to close or not, ask nsAppStartup if the window should close which can send out an appropriate notification requesting cancellations.

The tricky bit is that we don't want to send out that notification when the last window closes if we've already requested shutdown correctly. Hmm.

Flags: needinfo?(dtownsend)

(In reply to Dave Townsend [:mossop] (he/him) from comment #5)

The tricky bit is that we don't want to send out that notification when the last window closes if we've already requested shutdown correctly. Hmm.

At that point AppStartup's shuttingDown / mShuttingDown should already be true right?

Flags: needinfo?(dtownsend)

(In reply to :Gijs (he/him) from comment #6)

(In reply to Dave Townsend [:mossop] (he/him) from comment #5)

The tricky bit is that we don't want to send out that notification when the last window closes if we've already requested shutdown correctly. Hmm.

At that point AppStartup's shuttingDown / mShuttingDown should already be true right?

Oh yes! So, we could make nsIAppStartup.Quit send out the quit-application-requested and then start the real shutdown. While closing the last window would send out quit-application-requested if mShuttingDown isn't true and block closing the window if shutdown is cancelled. Then we can remove all the quit-application-requested dispatch code we have in various places.

Flags: needinfo?(dtownsend)

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression
Keywords: regression

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is P3 (Backlog,) indicating it has been triaged, the bug's Severity is being updated to S3 (normal.)

Severity: normal → S3
QA Contact: emil.ghitta
See Also: → 1463704

This isn't a trivial or good first bug, but we can mentor it - between comment 4 and comment 7 there's a reasonably straightforward path to make this happen.

Mentor: mak, gijskruitbosch+bugs
Whiteboard: [outreachy-2021-downloads]

Falguni has said they'll work on this.

Assignee: nobody → falgunimst95

Ok thank you I am starting working on this bug

Attachment #9213855 - Attachment description: WIP: Bug 1621678: fixed Warning pop-up is not displayed while trying to close Firefox during an ongoing download process if the browser console is open → WIP: Bug 1621678: fixed warning pop-up is not displayed while trying to close Firefox during an ongoing download process if the browser console is open

So to re-summarize: to fix this bug, you'll need a full (non-artifact) build.

You'll need to update https://searchfox.org/mozilla-central/rev/be413c29deeb86be6cdac22445e0d0b035cb9e04/toolkit/components/startup/nsAppStartup.cpp#341 to call into the observer service and send out the quit-application-requested topic, with an nsISupportsPRBool subject. See https://searchfox.org/mozilla-central/rev/be413c29deeb86be6cdac22445e0d0b035cb9e04/widget/windows/nsWindow.cpp#5197-5206 for an example of how to do this. Do this in an if condition that checks for ferocity == eAttemptQuit.

You'll want to add a method on nsIAppStartup called something like canCloseWindow that takes no arguments and returns a boolean. To implement it, check mConsiderQuitStopper. If it's >=2, always return true. If mShuttingDown is true, also immediately return true. Otherwise, fire the quit-application-requested topic as above, and return the result value of the nsISupportsPRBool.

You'll want to call that method from https://searchfox.org/mozilla-central/rev/be413c29deeb86be6cdac22445e0d0b035cb9e04/dom/base/nsGlobalWindowOuter.cpp#6142 and return the result value. To do this you can get a reference to the app startup object using code like this: https://searchfox.org/mozilla-central/rev/be413c29deeb86be6cdac22445e0d0b035cb9e04/toolkit/xre/nsAppRunner.cpp#2926-2927 and then call the CanCloseWindow() method on it that you created in the previous step.

Then, you'll want to check the places in this searchfox query: https://searchfox.org/mozilla-central/search?q=quit-application-requested where we're calling notifyObservers with quit-application-requested and remove them. (Just the places calling notifyObservers, not the ones where we add/remove observer listeners for this same topic!)

Attachment #9213855 - Attachment description: WIP: Bug 1621678: fixed warning pop-up is not displayed while trying to close Firefox during an ongoing download process if the browser console is open → Bug 1621678: fixed warning pop-up is not displayed while trying to close firefox during an ongoing download process if the browser console is open
Attachment #9213855 - Attachment description: Bug 1621678: fixed warning pop-up is not displayed while trying to close firefox during an ongoing download process if the browser console is open → Bug 1621678: fix warning pop-up is not displayed while trying to close firefox during an ongoing download process if the browser console is open
Attachment #9213855 - Attachment description: Bug 1621678: fix warning pop-up is not displayed while trying to close firefox during an ongoing download process if the browser console is open → Bug 1621678: fix Warning pop-up is not displayed while trying to close firefox during an ongoing download process if the browser console is open

Hello Gijs! From comment 14, it seems that you have clarified almost everything that is expected. I would like to work on this bug. Can you assign it to me?

Hi @Gijs.
I have been following this bug and have done some work following your instructions from comment 14. I have some questions. Can I post them here?
Thanks.

(In reply to Vaidehi from comment #16)

Hi @Gijs.
I have been following this bug and have done some work following your instructions from comment 14. I have some questions. Can I post them here?
Thanks.

Well, Falguni was working on this previously, so I'd like to make sure they are not still working on it before passing it to someone else. Falguni?

Flags: needinfo?(falgunimst95)

Going to assume that Falguni is no longer working on this. :-)

Assignee: falgunimst95 → nobody
Flags: needinfo?(falgunimst95)

hello Gijs, I am an outreachy applicant, i request to work on this please. can it be assigned to me please

Flags: needinfo?(gijskruitbosch+bugs)

actually same with Kali linux, no warning message displayed about the abortion of the downloading files

(In reply to briannapatricia7 from comment #19)

hello Gijs, I am an outreachy applicant, i request to work on this please. can it be assigned to me please

We normally assign bugs when patches go up, but you're free to start work on this, sure. comment 14 has a list of steps to address this bug. Do ask if you have further questions.

Flags: needinfo?(gijskruitbosch+bugs)
You need to log in before you can comment on or make changes to this bug.