855147 - Defect: Settings do not persist when closing Firefox from desktop application bar

Status: VERIFIED FIXED

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

Description

[Kamil Jozwiak [:kjozwiak]]

Settings will not persist in Metro Firefox when you follow these steps:

1. Open Metro Firefox
2. Change some settings
3. Go to Desktop
4. Expand the application bar on the left and locate Metro Firefox
5. Right click on Metro Firefox and click the close menu
6. Re-open Metro Firefox

Actual Results:

- No settings are persisted.

Expected Results:

- Settings should be persisted.

Comment 1

[Kamil Jozwiak [:kjozwiak]]

For consideration in it5

Comment 2

[Brian R. Bondy [:bbondy]]

When closing Firefox in this way we basically don't get a clean shutdown. Maybe the fix here is to save after dismissing the settings pane instead of on app shutdown.

Comment 3

[Brian R. Bondy [:bbondy]]

Unassigned myself for now, but if no one else wants it I'll take it.

Comment 4

[Marco Mucci [:MarcoM]]

Being placed into Story Backlog based on Asa's review.

Comment 5

[Marco Mucci [:MarcoM]]

Point Estimation =2.

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: patch v1

This patch listens saves preferences when the metro process is suspended.

This patch modifies metro's browser.js to listen for process suspension, but really I'm thinking that the preferences service should do this at [1].  I'm not sure how that will affect other platforms, so maybe let's land this patch and file a follow-up to modify the preferences service?

[1] https://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/Preferences.cpp?rev=36a1a6b81f90#405

Comment 7

[Brian R. Bondy [:bbondy]]

Comment on attachment 745323 [details] [diff] [review]
patch v1

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

We'll be saving it sometimes when we don't need to but I don't think that matters too much since it's on suspend. Thanks!

> I'm not sure how that will affect other platforms, so maybe let's land this patch and file a follow-up to modify the preferences service?

Improper shutdowns like this from the application bar is unique to windows so we don't have to worry about other platforms w/ -metrodesktop.
suspend_process_notification will only be fired from windows so this patch has pretty much no effect on those other platforms, which is fine.

::: browser/metro/base/content/browser.js
@@ +610,5 @@
>     */
>    _handleCertException: function _handleCertException(aMessage) {
>      let json = aMessage.json;
>      if (json.action == "leave") {
> +      // Get the start page from the *default* prefbranch, not the user's

nit: I think I prefer pref branch

Comment 8

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Brian R. Bondy [:bbondy] from comment #7)
> Comment on attachment 745323 [details] [diff] [review]
> patch v1
> 
> Review of attachment 745323 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We'll be saving it sometimes when we don't need to but I don't think that
> matters too much since it's on suspend. Thanks!
> 
> > I'm not sure how that will affect other platforms, so maybe let's land this patch and file a follow-up to modify the preferences service?
> 
> Improper shutdowns like this from the application bar is unique to windows
> so we don't have to worry about other platforms w/ -metrodesktop.
> suspend_process_notification will only be fired from windows so this patch
> has pretty much no effect on those other platforms, which is fine.

Ah, then maybe I can go ahead and change the prefs service instead of modifying browser.js?  The only reason I was thinking that this change should go in the prefs service instead of browser.js is that there are other shared services that handle the message directly [1][2][3] instead of us doing the processing in metro-specific code.

> ::: browser/metro/base/content/browser.js
> @@ +610,5 @@
> >     */
> >    _handleCertException: function _handleCertException(aMessage) {
> >      let json = aMessage.json;
> >      if (json.action == "leave") {
> > +      // Get the start page from the *default* prefbranch, not the user's
> 
> nit: I think I prefer pref branch

ha, I'm not even sure how that change got in there.  I guess a cat stepped on my keyboard while I had that file open in a text editor :)


[1] https://mxr.mozilla.org/mozilla-central/source/xpcom/threads/TimerThread.cpp?rev=fb7aae8421bc#417
[2] https://mxr.mozilla.org/mozilla-central/source/netwerk/cache/nsCacheService.cpp?rev=e73333270ce5#408
[3] https://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp?rev=c8b3e4b6a8b5#2400

Comment 9

[Brian R. Bondy [:bbondy]]

Ya I added those calls, I think that's a good suggestion but it may take a while to get a review if you do it that way. I'm ok with landing it this way to save time, but I'll leave it up to you.

Comment 10

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2

This patch save preferences when the process is being suspended.

Benjamin: You're listed as a Preferences peer. Dan Witte is listed as the module owner, but his bugzilla name indicates that he doesn't read bugmail.  Please feel free to forward this review to whomever makes sense/has time.

Comment 11

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Marco Mucci [:MarcoM]]

For testing and verification.

Comment 14

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (Windows NT 6.2; rv:24.0) Gecko/20130526 Firefox/24.0

Could still reproduce with my older profile (used since before the fix), but it works on a clean one. So I'll set this to verified.

- settings (Startup, Character Encoding, Password and DNT) are remembered when exiting Metro Firefox normally through drag and drop.
- settings (Startup, Character Encoding, Password and DNT) are remembered when exiting Metro Firefox through the application bar