335864 - Hookup Session Restore to installs view and blocklist restart app capability

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Robert Strong (they/them - no direct email)]

Once Session Restore lands we need to hookup the installs view and blocklist restart app buttons to Session Restore.

Comment 1

[Steffen Wilberg]

Looks like the Add-ons manager needs to set browser.sessionstore.resume_session_once to true before closing Firefox:
http://lxr.mozilla.org/seamonkey/source/browser/components/sessionstore/src/nsSessionStore.js#58

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/mozapps/extensions/content/extensions.js&rev=1.91&mark=1320-1327#1320
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/mozapps/extensions/content/list.js&rev=1.2.2.4&mark=202-208#202

Comment 2

[Nickolay_Ponomarev]

Shouldn't session store default to preserving sessions when restarting using nsIAppStartup? IMO it would be better than having to fix all of nsIAppStartup consumers (that includes some extensions, like RestartFirefox).

Do any nsIAppStartup consumers want to bypass automatic session restoring? Is it a good idea to make all consumers, that want to take advantage of session store, set a SS-specific pref?

Comment 3

[Simon Bünzli]

I hope the idea wouldn't be to let all consumers set a specific preference but to offer a simple API (nsISessionStore.restart) which would take care of the implementation details.

It might be worth though to think about nsAppStartup sending a notification "restart-application" so that SessionStore and other extensions can react appropriately. Would you mind to file a bug for that?

Comment 4

[Nickolay_Ponomarev]

> I hope the idea wouldn't be to let all consumers set a specific preference but
> to offer a simple API (nsISessionStore.restart) which would take care of the
> implementation details.
> 
This makes those components dependent on session store, when all they *really* want to do is restart the app.

> It might be worth though to think about nsAppStartup sending a notification
> "restart-application" so that SessionStore and other extensions can react
> appropriately. Would you mind to file a bug for that?
> 
I hope it can happen in this bug. nsAppStartup already sends out quit-application with no additional information. Perhaps it can also inform observers whether it's a normal quit or a quit-and-restart situation.

Comment 5

[Robert Strong (they/them - no direct email)]

With other work I have to finish up I won't be taking on anything outside of what this bug is specifically for (e.g. using the method provided by session restore).

Comment 6

[Simon Bünzli]

This shouldn't really be too complicated to fix. Taking.

Comment 7

[Simon Bünzli]

Attachment: add exit reason to quit-application

The good news is that the fix is indeed quite simple (mostly just adding a shutdown reason to the quit-application notification). The bad news is that this isn't enough. SessionStore still needs the quit-application-requested notification to know when the last chance for updating its internal state is and the quit-application-granted notification to know from what point on to ignore closing windows. So you'll at least have to call canQuitApplication before restarting.

In the end, extensions will still have to make sure to be good citizens, but there's no new API to know (note that all extension authors whose extensions I've found to do this wrong have already been notified - because they break my Session Manager/Crash Recovery extensions which rely on these notifications as well).

The alternative to this fix would be to call canQuitApplication's code from nsAppStartup::Quit when it's first called. Who's to decide on this?

Comment 8

[Simon Bünzli]

An alternative to the above would be to introduce goRestartApplication in globalOverlay.js (which does the same as goQuitApplication with the subtle difference of adding the eRestart flag). Though this bug probably isn't the place for that decision...

Comment 9

[Robert Strong (they/them - no direct email)]

Comment on attachment 221627 [details] [diff] [review]
add exit reason to quit-application

bsmedberg for nsAppStartup review.

Comment 10

[Mike Connor [:mconnor]]

So, this is a better approach, since nsISessionRestore is in browser, not toolkit, so that's a bad dependency.  The patch looks ok to me, but I think we can wait on adding the right method to nsISessionStore instead of setting the pref directly.

Comment 11

[Benjamin Smedberg]

Comment on attachment 221627 [details] [diff] [review]
add exit reason to quit-application

So in general if code wants to restart the app it should be using nsIAppStartup.quit(eRestart)?

Comment 12

[Simon Bünzli]

(In reply to comment #11)
> So in general if code wants to restart the app it should be using
> nsIAppStartup.quit(eRestart)?

Something along these lines would be the obvious choice, wouldn't it? Although to get a somewhat nicer (XPCOM hiding) interface and prevent code from forgetting to call canQuitApplication, I'd add goRestartApplication as suggested in bug 338039.

(In reply to comment #10)
> I think we can wait on adding the right method to nsISessionStore
> instead of setting the pref directly.

If we go this way, we won't need any specific method to nsISessionStore at all. Setting the pref would then indeed be all that is required.

Comment 13

[Robert Strong (they/them - no direct email)]

Simon, you want a checkin?

Comment 14

[Simon Bünzli]

Sure. When doing so, please also remove the now obsolete comment line right below restartApp in extensions.js that I overlooked (though I hope to get rid of that method altogether as soon as bug 338039 is fixed). Thanks.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Attachment: as checked in on the trunk

mozilla/toolkit/components/startup/src/nsAppStartup.cpp 	1.13
mozilla/browser/components/sessionstore/src/nsSessionStore.js 	1.12
mozilla/toolkit/mozapps/extensions/content/extensions.js 	1.92
mozilla/toolkit/mozapps/extensions/content/list.js 	1.7
mozilla/toolkit/mozapps/extensions/content/list.xul 	1.4

Comment 16

[(not reading, please use [e-mail] instead)]

robert asked me to back-port the second patch to the 1.8 branch for FF 2.0

Comment 17

[Dietrich Ayala (:dietrich)]

Please don't put this on branch yet. I think it broke session restoration, and am looking further into it.

Comment 18

[(not reading, please use [e-mail] instead)]

ok, I will wait for the "all clear" signal before I attempt to port it to the branch.

Comment 19

[Dietrich Ayala (:dietrich)]

Attachment: Fixes regression in previous patch

The patch as checked-in prevents any session data from being saved: Windows are closed after quit-application-granted, and before quit-application. The patch caused data to be saved after quit-application was received, at which time windows have already closed and sessionstore has already removed their state information.

This fix forces data to be saved at quit-application-granted, and no longer saves after receiving quit-application.

Comment 20

[Robert Strong (they/them - no direct email)]

Comment on attachment 224123 [details] [diff] [review]
Fixes regression in previous patch

>Index: browser/components/sessionstore/src/nsSessionStore.js
...
>@@ -256,20 +256,18 @@ SessionStoreService.prototype = {
...
>+    // if not resuming, discard all session related data 
>+    if (!this._doResumeSession()) {
>       this._clearDisk();
>     }
I suspect the first thing mconnor is going to say is remove the brackets. ;)

Comment 21

[Simon Bünzli]

(In reply to comment #19)
> Created an attachment (id=224123) [edit]

> This fix forces data to be saved at quit-application-granted, and no longer
> saves after receiving quit-application.

So far, quit-application is pretty much guaranteed to happen, quit-application-granted is not. Rather change onClose to not delete the window data when _loadState == STATE_QUITTING.

Comment 22

[Simon Bünzli]

Attachment: additional trunk patch

Comment 23

[Dietrich Ayala (:dietrich)]

Comment on attachment 224123 [details] [diff] [review]
Fixes regression in previous patch

zeniko's patch fixes the problem better, obsoletes this

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Landed "additional trunk patch" on the trunk.
mozilla/browser/components/sessionstore/src/nsSessionStore.js 	1.13

Should I land both attachments on this bug on the branch?

Comment 25

[Steffen Wilberg]

Setting target milestone to b1 since this is needed for bug 335864 as I understand it, which is targetted at b1.

Comment 26

[Simon Bünzli]

(In reply to comment #24)
> Should I land both attachments on this bug on the branch?

Any more objections, Dietrich? AFAICS, both patches should be landable "as is" on the branch.

Comment 27

[Dietrich Ayala (:dietrich)]

(In reply to comment #26)
> (In reply to comment #24)
> > Should I land both attachments on this bug on the branch?
> 
> Any more objections, Dietrich? AFAICS, both patches should be landable "as is"
> on the branch.
> 

Looks fine to me :)

Comment 28

[:Gavin Sharp [email: [e-mail]]]

Attachment: branch patch

Comment 29

[:Gavin Sharp [email: [e-mail]]]

Attachment 224882 [details] [diff] and attachment 224126 [details] [diff] [review] checked in on the branch. Simon/Dietrich, could you guys verify that everything is as it should be?

Comment 30

[Dietrich Ayala (:dietrich)]

(In reply to comment #29)
> Attachment 224882 [details] [diff] [edit] and attachment 224126 [details] [diff] [review] [edit] checked in on the branch.
> Simon/Dietrich, could you guys verify that everything is as it should be?
> 

Clobbered branch, rebuilt, tested. Seems fine, thanks Gavin.