353592 - session restore (incl undo-close-tab) doesn't work after restart-for-update is aborted [Mac]

Status: NEW

(Firefox :: Session Restore, defect, P3)

Description

[Mike Shaver (:shaver emeritus)]

If restart-for-update doesn't actually quit[*], the session restore infrastructure seems to fall down.  Undo-close-tab produces these error console entries:

Error: this._windows[aWindow.__SSi] has no properties
Source File: file:///Applications/BonEcho.app/Contents/MacOS/components/nsSessionStore.js
Line: 648

Error: uncaught exception: [Exception... "'[JavaScript Error: "this._windows[aWindow.__SSi] has no properties" {file: "file:///Applications/BonEcho.app/Contents/MacOS/components/nsSessionStore.js" line: 648}]' when calling method: [nsISessionStore::getClosedTabCount]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://browser/content/browser.js :: undoCloseTab :: line 6658"  data: yes]

If you "apply downloaded update", I think you end up with a bogus session store data set on restart, but I'll report back on that in a minute when I do just that!

[*] I think mconnor was filing the bug on the fact that restart-for-update doesn't actually quit the browser if you have a page open which prompts onbeforeunload, like a webmail UI.  It closes the window if you let it, but that doesn't quit us on the Mac.  I suspect this also happens if you let onbeforeunload cancel the close by returning false, but I haven't tried that.

Comment 1

[Mike Shaver (:shaver emeritus)]

Yeah, when I applied downloaded update, I didn't end up with the session that was active when I picked that menu item.  I think I got some of what I had when I _originally_ tried to restart for the update, but maybe only one of the windows?  Sorry, I didn't take very good notes here, but it should be easy enough for someone who actually knows SS to reproduce this on a Mac of their choosing.

Comment 2

[Dietrich Ayala (:dietrich)]

To clarify the STR:

1. open a page that handles onbeforeunload: http://dietrich.ganx4.com/mozilla/353592.html
2. select "check for updates" via the help menu
3. click "download and install now"
4. when update is downloaded, click "restart"
5. click the default on all prompts (which closes all windows, and OKs the onbeforeunload prompt)

Expected result: app should close and restart.

Actual result: app does not close. Continuing the session from that point causes session-restore features such as undo-close-tab to not work, and the original pre-update-applied session to be restored at next startup.

The problem with session restoration is caused because the quit-application-granted notification is received, and SS stops collecting session information at that point.

Note that this scenario only occurs when there's an onbeforeunload handler. Aborting the update and later selecting "apply downloaded update" works fine in the absence of an onbeforeunload handler. Also note that a manual closing+restarting of the app works fine if there's an onbeforeunload handler. Only a restart post-update seems to fail to actually close the app.

The proper solution is to close the application, as is expected by the user *and* any code doing cleanup on quit-application-granted.

Potential workarounds might be to either not send quit-application-granted if we're not actually shutting down the app, or to have SS continue to collect data until quit-application is received. However, both of those will likely cause new and different problems.

Both options do suffer from the same major problem: the user has already voluntarily closed windows and tabs *without* closing the application. This effectively clears the session, therefore those windows will not be restored upon restart.

Another problem is that you can't quit the app at that point without it immediately restarting and applying the update. This restart is unexpected to the user (who just manually quit the app), therefore another reason to fix the root cause of the problem.

Note that the errors from undo-close-tab while in the limbo state should be fixed. We should cleanly handle all situations where either session-restore is disabled, or particular windows and tabs aren't being tracked.

Comment 3

[Simon Bünzli]

(In reply to comment #2)
> Potential workarounds might be to either not send quit-application-granted if
> we're not actually shutting down the app, or to have SS continue to collect
> data until quit-application is received. However, both of those will likely
> cause new and different problems.

I don't see how not sending quit-application-granted could cause issues when the quit-application-request effectively hasn't been granted (in a very quirky way though). Fixing bug 338040 might make things somewhat easier to resolve.

> Note that the errors from undo-close-tab while in the limbo state should be
> fixed.

For reference: that's bug 345898 and bug 352524.

Comment 4

[Dietrich Ayala (:dietrich)]

(In reply to comment #3)
> 
> I don't see how not sending quit-application-granted could cause issues when
> the quit-application-request effectively hasn't been granted (in a very quirky
> way though).

Yes, you are right, that's the proper behavior.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Drivers sayeth: this is an edge case on Mac only, and the session restore code is otherwise pretty solid, so we don't really want to mess around with this at this time. Also, it's not clear if the best way to handle this would be by modifying session restore or the way we deal with various APIs (as per comment 3).

Comment 6

[Mike Shaver (:shaver emeritus)]

relnote at least?  I don't think this is that uncommon a case, but don't really want to get in a fight about it.  (I'd nom for 1.8.1.1-blocking if I could!)

Comment 7

[Dietrich Ayala (:dietrich)]

(In reply to comment #0)
> [*] I think mconnor was filing the bug on the fact that restart-for-update
> doesn't actually quit the browser if you have a page open which prompts
> onbeforeunload, like a webmail UI.  It closes the window if you let it, but
> that doesn't quit us on the Mac.  I suspect this also happens if you let
> onbeforeunload cancel the close by returning false, but I haven't tried that.
> 

mconnor, did you file a bug about this? The fact that an update is not actually applied after the user hits "restart" has the potential for problems, especially if the update is a critical security fix.

Comment 8

[Adam Guthrie]

*** Bug 354200 has been marked as a duplicate of this bug. ***

Comment 9

[Wayne Mery (:wsmwk)]

> mconnor, did you file a bug about this? The fact that an update is not actually
> applied after the user hits "restart" has the potential for problems,

I don't find such a bug was filed.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Is this something that we'd be able to fix on the branches? It should definitely be fixed for Firefox 3.

Comment 11

[Mike Connor [:mconnor]]

I don't remember the details enough to file a bug at this point, if someone knows what I meant a year ago, please file the right bug!

Comment 12

[Mike Connor [:mconnor]]

moving out bugs that don't need to block b1

Comment 14

[Mike Connor [:mconnor]]

this is gonna sting a little, but not going to block on this, not a regression from Fx2.  would _love_ a fix though.

Comment 15

[Mike Shaver (:shaver emeritus)]

relnote?

Comment 16

[Negritas Sergiu, Desktop QA]

Hello! I have tried to reproduce this issue with the latest firefox versions on all os's. I will change the severity of this issue to minor, if you think this issue is still valid please feel free to change the severity.

Have a nice day!