846365 - Change - New window opens in desktop Firefox when closing Metro Firefox

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Shell, defect, P1)

Description

[Josh Tumath]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:22.0) Gecko/20130228 Firefox/22.0
Build ID: 20130228030852

Steps to reproduce:

1. Open desktop Firefox.
2. Browse to any Web site.
3. Open Metro Firefox.
4. Browse to any Web site.
5. Go back to the desktop with the desktop Firefox window still active.
6. Close Metro Firefox from the app list by doing the following:
  a. Move your mouse to the very top left corner of the screen.
  b. Slide the mouse down the edge of the screen.
  c. Right click on the Metro Firefox app in the app list.
  d. Click on "Close" in the context menu.


Actual results:

A new window appeared in Firefox with just an empty tab.

Comment 1

[Brian R. Bondy [:bbondy]]

I always see this and it bothers me, confirming.

Comment 2

[Asa Dotzler [:asa]]

I'd not seen this before. Are the steps outlined the only way to trigger this bug?

Comment 3

[Josh Tumath]

(In reply to Asa Dotzler [:asa] from comment #2)
> I'd not seen this before. Are the steps outlined the only way to trigger
> this bug?

As far as I'm aware, yes. I can only reproduce it by closing the app via the app list.

Comment 4

[Brian R. Bondy [:bbondy]]

I get this when I bring up the application list on the right windows edge (the bar that comes up when you window + shift + tab) and then right click on the app and close.  This is how I mostly close applications in win8 and this bug really bothers me. 

I can fix it in it4 if you'd like as a defect.

Comment 5

[Brian R. Bondy [:bbondy]]

Also closing from the application bar leads to a unclean shutdown so it always brings up the same tabs as you last used from session restore.

Comment 6

[Asa Dotzler [:asa]]

OK. Yes. I think we need to fix this. Marking it as a *change* to Bug 831888 - Story - Closing Firefox because we didn't have that form of application closing covered in that story.

Comment 7

[Brian R. Bondy [:bbondy]]

Found the reason for this but have not fixed yet.  Basically if you don't close the application while it's activated we don't get the normal shutdown sequence.  So Windows restarts our process with a special command line to allow us to do any normal cleanup at that point.  .... so strange that they do this...

> Closing background sessions and cleaning up private browsing data
> There are a few user affordances that allow for a user to close a new 
> experience browsing session without first bringing the browser window 
> to the foreground. 
>•	Pulling the thumbnail of the browser from the switch list to the 
> bottom of the screen.
> •	Right-clicking the thumbnail of the browser from the switch list 
> and choosing ‘close’. 
> •	Pulling the browser from the back stack using the left edge 
> and dragging the window to the bottom of the screen.
> In such instances, the browser can’t run code before the app is closed. 
> The browser’s window isn’t sent a “DefaultBrowserClosing” message. 
> Instead, Process Lifetime Management closes the browser and immediately 
> reactivates it as a desktop process with the -BackgroundSessionClosed 
> command-line argument, e.g. “examplebrowser.exe -BackgroundSessionClosed”. Use 
> this opportunity to clean up data from the previous browsing session,
> particularly sensitive user data that may have been persisted to disk during a 
> private browsing session. Don’t present a window or run other tasks. Exit as 
> soon as the cleanup is complete. If no data cleanup tasks are required (for 
> instance, if the browser doesn’t offer the private browsing feature), handle the
>  activation argument and exit.

Comment 8

[Brian R. Bondy [:bbondy]]

I think in our case we only really want to clear the session history.

Comment 9

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

See Comment 7 before reviewing.  This adds support for background sessions being closed which re-opens Firefox with a special commandline to do cleanup.  Basically I route that command line to the WinRT widget code and do a partial initialization which is enough to get the profile directory.  Then clear the session store and exit silently.

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

Sorry was missing a closing parentheses on a last minute addition

Comment 11

[Brian R. Bondy [:bbondy]]

Comment on attachment 727846 [details] [diff] [review]
Patch v1.

ack I forgot to mark r? on the newly uploaded patch.

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 727846 [details] [diff] [review]
Patch v1.

Review of attachment 727846 [details] [diff] [review]:
-----------------------------------------------------------------

Is there anything we can do in here about those cache problems we were having?

::: widget/windows/winrt/MetroApp.cpp
@@ +235,5 @@
> +
> +  // Perform any cleanup for unclean shutdowns here, such as when the background session
> +  // is closed via the appbar on the left when outside of Metro.  Windows restarts the
> +  // process solely for cleanup reasons.
> +  if (IsBackgroundSessionClosedStartup() && SUCCEEDED(XRE_metroStartup(false))) {

Should we check the result of XRE_metroStartup? I think we want to return from within this bock no matter what.

Comment 13

[Brian R. Bondy [:bbondy]]

(In reply to Jim Mathies [:jimm] from comment #12)
> Comment on attachment 727846 [details] [diff] [review]
> Patch v1.
> 
> Review of attachment 727846 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there anything we can do in here about those cache problems we were
> having?


Unfortunately no. The way the disk cache is designed, if the program crashes or closes without notification, we have the potential of a header to files on disk mismatch that we can't detect.  Luckily though, the disk cache is being redesigned as we speak in this project: https://wiki.mozilla.org/Necko/Cache/Plans

> 
> ::: widget/windows/winrt/MetroApp.cpp
> @@ +235,5 @@
> > +
> > +  // Perform any cleanup for unclean shutdowns here, such as when the background session
> > +  // is closed via the appbar on the left when outside of Metro.  Windows restarts the
> > +  // process solely for cleanup reasons.
> > +  if (IsBackgroundSessionClosedStartup() && SUCCEEDED(XRE_metroStartup(false))) {
> 
> Should we check the result of XRE_metroStartup? I think we want to return
> from within this bock no matter what.

We may want to do that if we have anything in that block in the future that doesn't require initialization, but at the moment there is no need to ignore the result. The call that it will fall through to "hr = sCoreApp->Run(sMetroApp.Get());" will simply fail right away with a COM error of element not found, and the program will exit.  I think because it tries to find a CoreWindow but none exists.  For that reason I'll just leave this as is.

Thanks for the weekend review!

Comment 14

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea59a801aabf

Comment 15

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/ea59a801aabf

Comment 16

[juan becerra [:juanb]]

Tested on 2013-03-29 on a nightly built from http://hg.mozilla.org/mozilla-central/rev/8693d1d4c86d
- Tested with the steps in comment #0 when there was only desktop and metro Firefox were the only two applications running as well as when there were other apps running.
- Reopening metro Firefox in Metro mode opens cleanly with a Start page if I have set the preference to open with a start page, or it opens with the previous tabs from last time if I have that preference set.
- I couldn't close desktop Firefox using the same steps, but maybe that's intended? Filed a defect bug 856097.

Comment 17

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Change" for iteration #7 testing without any issues. Used the following build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-05-25-06-25-25-mozilla-central/

- Went through the original steps outlined in Comment 0 without any issues
- Went through the test cases added in Comment 16 without any issues

Comment 18

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130825030201
Built from http://hg.mozilla.org/mozilla-central/rev/01576441bdc6

WFM
Tested on windows 8 using latest nightly for iteration-12. Followed steps provided in comment0 and got expected result.