732631 - Selecting to a open a new window within an app, then exiting the app does not shut down the application

Status: VERIFIED FIXED

(Firefox Graveyard :: Webapp Runtime, defect)

Description

[Jason Smith [:jsmith]]

Related to bug 732628, filing as a separate issue, as its specific to Mac OS X only.

Steps:

1. Follow the steps in bug 732628
2. Close the app

Expected:

The app should shutdown and stop running.

Actual:

The app continues to run upon close, even though the window was disposed. Trying to start up the app again while its running does nothing. If I explicitly kill the application by going to the file menu and selecting quit, then I can successfully re-launch the app.

Comment 1

[Jason Smith [:jsmith]]

Dan - When I close a native application in Mac OS X, should the application "quit" (stop running, not shown on the bottom toolbar), or should the application continue to run? With the majority of the apps, I've seen the first option ("quit") occur, but this one appears to be different I click the report bug link. Any ideas on what is going on here?

Comment 2

[Dan Walkowski]

I'm not sure exactly what the issue is you're describing (I assume you hand-made a native app), but I see two issues with it when run in Firefox.  It opens an additional window, and also goes to a different domain.  Opening additional windows is a complicated issue, but in general I think it should be avoided by webapps. Opening popup windows to other domains should probably be frowned on as well.  

Can you demonstrate your test app for me?  I suspect that clicking the report bug button is opening an invisible window somehow.

I use Dave's Galaxy as my testing app, and it has all sorts of non-app behavior.  There are help pages and such that you can click-through to, with no way to return to the game. (no back button) 
Website developers are going to have to change their content to make it feel more app-like.

Comment 3

[Jason Smith [:jsmith]]

(In reply to Dan Walkowski from comment #2)
> I'm not sure exactly what the issue is you're describing (I assume you
> hand-made a native app), but I see two issues with it when run in Firefox. 
> It opens an additional window, and also goes to a different domain.  Opening
> additional windows is a complicated issue, but in general I think it should
> be avoided by webapps. Opening popup windows to other domains should
> probably be frowned on as well.  
> 
> Can you demonstrate your test app for me?  I suspect that clicking the
> report bug button is opening an invisible window somehow.

Guangcong Luo built this open web application temporarily. I'll contact him about this.

Guangcong, could you by chance enable a hidden URL on your http://pokemonshowdown.com/ to allow us to test your application in native app mode? We would like to investigate the root cause of this bug if possible.

> 
> I use Dave's Galaxy as my testing app, and it has all sorts of non-app
> behavior.  There are help pages and such that you can click-through to, with
> no way to return to the game. (no back button) 
> Website developers are going to have to change their content to make it feel
> more app-like.

Agreed. A recent discussion with Ron, Ragavan, and others concluded that we need to build a "recommended practices" document for developers and QA to ensure that apps follow "app-like" qualities. On the QA side, we're going to start working on this as of today. I'll be sure to put this out for feedback to the apps team (including you) to ensure that the app-like qualities are well specified.

Comment 4

[Guangcong Luo]

(In reply to Jason Smith from comment #3)
> Guangcong Luo built this open web application temporarily. I'll contact him
> about this.
> 
> Guangcong, could you by chance enable a hidden URL on your
> http://pokemonshowdown.com/ to allow us to test your application in native
> app mode? We would like to investigate the root cause of this bug if
> possible.

Sure.

http://pokemonshowdown.com/fxdev.html

This is just my home page with the "Install Firefox app" button no longer commented out.

Some notes:

The "Report Bug" area isn't supposed to be a part of the app itself; presumably, when installed as a web app, it would open the bug report thread in the default browser. It has target="_blank" to reflect that.

The "Report Bug" button, interestingly enough, works differently than if you paste that exact same link (which will also append target="_blank") into a chat area (please use a private chat area, since Pokemon Showdown is a production webapp).

Pokemon Showdown UI code is a bit messy; I apologize. I'm planning on refactoring it, I promise. ;)

Comment 5

[Tony Chung [:tchung]]

qawanted is a keyword, not a whiteboard.   use that one instead for bugs that require more testing.

Comment 6

[Jason Smith [:jsmith]]

Just saw this behavior occur with GIS Cloud with the new Mac implementation in the try build. Will get a reproducible test case for this.

Comment 7

[Jason Smith [:jsmith]]

Tested this with the pokemon showdown game as well. This looks like this is still happening with new implementation. Removing qawanted.

Comment 8

[Jason Smith [:jsmith]]

The only way to work-around this issue if it occurs is to kill the application process. Trying restart the application will not start the window again. Moving severity to critical.

Comment 9

[Jason Smith [:jsmith]]

Also, saw this happen when running Sinuous.

Comment 10

[Ed Lee :Mardak]

How are you closing the app? If I click the red X or use cmd-q after clicking "Report Bug", it closes fine for me.

Comment 11

[Jason Smith [:jsmith]]

(In reply to Edward Lee :Mardak from comment #10)
> How are you closing the app? If I click the red X or use cmd-q after
> clicking "Report Bug", it closes fine for me.

I'm close the app with the red X. It does not look like it happens every time you do the steps to reproduce. I did one click on report bug and closed right now on OS X 10.7 and saw the app shutdown. However, when I did multiple clicks on report bug and then shutdown, the app did not shutdown.

Comment 12

[Jason Smith [:jsmith]]

(In reply to Jason Smith from comment #11)
> (In reply to Edward Lee :Mardak from comment #10)
> > How are you closing the app? If I click the red X or use cmd-q after
> > clicking "Report Bug", it closes fine for me.
> 
> I'm close the app with the red X. It does not look like it happens every
> time you do the steps to reproduce. I did one click on report bug and closed
> right now on OS X 10.7 and saw the app shutdown. However, when I did
> multiple clicks on report bug and then shutdown, the app did not shutdown.

Badly worded. Let me try again. I closed the app using the X button. It does not appear to happen every time you do the steps to reproduce. If I did one click on the report bug link and closed the app, then the app did shutdown on OS X 10.7. If I did multiple clicks on report bug and closed the app, then the app did not shutdown.

Comment 13

[Jason Smith [:jsmith]]

Note - When this happens, the CPU and memory spikes way high:

- CPU Usage went as high as 65% on OS X 10.7
- Memory up to 260 MB

The underlying issue that could be the cause is that the above site using target="_blank" in a link, which would normally open a new tab in desktop firefox. The current implementation only supports a single window. What might be happening is the window is getting created after clicking that link, but not visible to the user. When they close the app, the other window still remains open, leaving the app still active.

What's strange is when this happens, you cannot restart the application. You can't kill it through the menu. The only thing you can do is kill the process.

Comment 14

[Jason Smith [:jsmith]]

Also happens on Win 7 64-bit, although since the cmd prompt window exists due to an existing bug, if you close the cmd prompt that comes up, that app will shutdown. The severity of this bug is much more prominent on Mac than windows.

Comment 15

[Jason Smith [:jsmith]]

Reduced test case - Create an app that uses target="_blank" in a link:

<a target="_blank" href="http://www.creativesocialapps.com/petoosnowball/">Fancy Box Test</a>

Comment 16

[Ed Lee :Mardak]

Does the high CPU also happen in that reduced test case? (High CPU could just be the pokemon showdown app.)

Comment 17

[Ed Lee :Mardak]

With a testcase that uses window.open, I see that the OS X app does not quit when clicking the red x. But quitting with cmd-q correctly quits.

Perhaps something with the platform thinking a window is open (or failed to show the window), so it doesn't quit until all windows are closed.

Comment 18

[Jason Smith [:jsmith]]

(In reply to Edward Lee :Mardak from comment #17)
> With a testcase that uses window.open, I see that the OS X app does not quit
> when clicking the red x. But quitting with cmd-q correctly quits.

Confirmed. It only appears to happen when clicking the red x, but not cmd-q (same thing for Mac & Windows).

Comment 19

[Jason Smith [:jsmith]]

(In reply to Edward Lee :Mardak from comment #16)
> Does the high CPU also happen in that reduced test case? (High CPU could
> just be the pokemon showdown app.)

On Windows, no, the CPU does not spike. Could be a Mac issue though if we get this to happen consistently. Digging into this now.

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Clicking the "test open" link in Ed's test app <http://ed.agadak.net/app/> against a debug build dumps these messages to the log:

++DOCSHELL 0x103964c00 == 4 [id = 4]
++DOMWINDOW == 7 (0x103965c78) [serial = 9] [outer = 0x0]
++DOMWINDOW == 8 (0x103967478) [serial = 10] [outer = 0x103965c00]
++DOCSHELL 0x103806800 == 5 [id = 5]
++DOMWINDOW == 9 (0x103807478) [serial = 11] [outer = 0x0]
++DOMWINDOW == 10 (0x10380dc78) [serial = 12] [outer = 0x103807400]
++DOMWINDOW == 11 (0x10cf22078) [serial = 13] [outer = 0x103807400]
WARNING: NS_ENSURE_TRUE(xulWin->mPrimaryContentShell) failed: file /Users/myk/Dropbox/central-git/xpfe/appshell/src/nsXULWindow.cpp, line 1810
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /Users/myk/Dropbox/central-git/dom/base/nsGlobalWindow.cpp, line 8823
++DOMWINDOW == 12 (0x10e135078) [serial = 14] [outer = 0x103807400]
--DOMWINDOW == 11 (0x10cf22078) [serial = 13] [outer = 0x103807400] [url = about:blank]
--DOMWINDOW == 10 (0x10380dc78) [serial = 12] [outer = 0x103807400] [url = about:blank]

Comment 21

[Ed Lee :Mardak]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #20)
> Clicking the "test open" link in Ed's test app <http://ed.agadak.net/app/>
> against a debug build dumps these messages to the log:
> 
> ++DOCSHELL 0x103964c00 == 4 [id = 4]
> ++DOMWINDOW == 7 (0x103965c78) [serial = 9] [outer = 0x0]
> ++DOMWINDOW == 8 (0x103967478) [serial = 10] [outer = 0x103965c00]
> ++DOCSHELL 0x103806800 == 5 [id = 5]
> ++DOMWINDOW == 9 (0x103807478) [serial = 11] [outer = 0x0]
> ++DOMWINDOW == 10 (0x10380dc78) [serial = 12] [outer = 0x103807400]
> ++DOMWINDOW == 11 (0x10cf22078) [serial = 13] [outer = 0x103807400]
> WARNING: NS_ENSURE_TRUE(xulWin->mPrimaryContentShell) failed: file
> /Users/myk/Dropbox/central-git/xpfe/appshell/src/nsXULWindow.cpp, line 1810
> WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file
> /Users/myk/Dropbox/central-git/dom/base/nsGlobalWindow.cpp, line 8823
> ++DOMWINDOW == 12 (0x10e135078) [serial = 14] [outer = 0x103807400]
> --DOMWINDOW == 11 (0x10cf22078) [serial = 13] [outer = 0x103807400] [url =
> about:blank]
> --DOMWINDOW == 10 (0x10380dc78) [serial = 12] [outer = 0x103807400] [url =
> about:blank]

There's 2 main things happening here that I've noticed when investigating:

1) not all dom windows are -- removed (click open or _blank many times and when you quit the app, you'll see a ton of windows getting destroyed)

2) the warning is the same that I noted in bug 746217 c2 and is how I traced back through the code to find "content-primary". That's how the patch in that bug (type=content -> type=content-primary) avoids the warning and then allows windows to open.

Comment 22

[Ed Lee :Mardak]

Attachment: v1

Switch type from "content" to "content-primary" to avoid zombie windows because the platform code is expecting to find "content-primary" when adding a content shell.

Comment 23

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 618756 [details] [diff] [review]
v1

>diff --git a/webapprt/content/webapp.js b/webapprt/content/webapp.js

>+  // Only load the app on the initially launched main window
>+  if (!("arguments" in window))
>+    return;
>+
>   // Load the webapp's launch path.
>   let url = Services.io.newURI(installRecord.origin, null, null);
>   if (manifest.launch_path)
>     url = Services.io.newURI(manifest.launch_path, null, url);
>   document.getElementById("content").setAttribute("src", url.spec);

This is ok, but the early exit from the function on a non-fatal conditional is prone to bugs like bug 746645.  So it would be better to reverse the logic:

  if ("arguments" in window) {
    // Load the webapp's launch path.
    ...
  }

Nit: change "launch path" to "launch URL" in the process to make the comment more accurate, as we load the full URL, not just the path!


Note to future selves: we could take this a step further and move construction of the launch URL to CommandLineHandler, passing it to the initial main window via an argument, which the main window then loads.  That would pave the way for us to explicitly open windows to non-launch URLs, should we identify a use case for doing so.


It's fascinating how this Just Works‌™.

Comment 24

[Ed Lee :Mardak]

Attachment: for checkin

Comment 25

[Ed Lee :Mardak]

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

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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