All users were logged out of Bugzilla on October 13th, 2018

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

VERIFIED FIXED in Firefox 22

Status

P1
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: josh.tumath+bugzilla, Assigned: bbondy)

Tracking

Trunk
Firefox 22
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=change c=Opening_and_closing u=metro_firefox_user p=3 status=verified)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.

Updated

6 years ago
Blocks: 841214
(Assignee)

Comment 1

6 years ago
I always see this and it bothers me, confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

6 years ago
I'd not seen this before. Are the steps outlined the only way to trigger this bug?
(Reporter)

Comment 3

6 years ago
(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.
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
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.
(Assignee)

Updated

6 years ago
Whiteboard: p=3

Comment 6

6 years ago
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.
Blocks: 831888
No longer blocks: 841214
Whiteboard: p=3 → feature=change p=3

Updated

6 years ago
Blocks: 842108
Priority: -- → P1
Summary: New window opens in desktop Firefox when closing Metro Firefox → Change - New window opens in desktop Firefox when closing Metro Firefox
Whiteboard: feature=change p=3 → feature=change c=Opening_and_closing u=metro_firefox_user p=3
(Assignee)

Updated

6 years ago
Assignee: nobody → netzen
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Comment 8

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

Updated

6 years ago
Status: NEW → ASSIGNED

Updated

6 years ago
Component: General → Shell
(Assignee)

Comment 9

6 years ago
Created attachment 727837 [details] [diff] [review]
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.
Attachment #727837 - Flags: review?(jmathies)
(Assignee)

Comment 10

6 years ago
Created attachment 727846 [details] [diff] [review]
Patch v1.

Sorry was missing a closing parentheses on a last minute addition
Attachment #727837 - Attachment is obsolete: true
Attachment #727837 - Flags: review?(jmathies)
(Assignee)

Comment 11

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

ack I forgot to mark r? on the newly uploaded patch.
Attachment #727846 - Flags: review?(jmathies)

Comment 12

6 years ago
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.
Attachment #727846 - Flags: review?(jmathies) → review+
(Assignee)

Comment 13

6 years ago
(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!
https://hg.mozilla.org/mozilla-central/rev/ea59a801aabf
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Flags: needinfo?(jbecerra)

Updated

6 years ago
QA Contact: jbecerra
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jbecerra)
Whiteboard: feature=change c=Opening_and_closing u=metro_firefox_user p=3 → feature=change c=Opening_and_closing u=metro_firefox_user p=3 status=verified

Updated

6 years ago
Depends on: 856097
(Assignee)

Updated

6 years ago
No longer depends on: 856097
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
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.
You need to log in before you can comment on or make changes to this bug.