579604 - force a prefs file save after setup completes

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

I installed Firefox Sync today later in the evening my browser crashed. When it restarted I was asked to go through the setup process again.

Comment 1

[Mike Connor [:mconnor]]

Well... Firefox doesn't save prefs if you crash, and that's where we store data about setup, like the server URL.  This might be a dupe, but maybe we should just explicitly flush to disk at the end of setup.

Comment 2

[Gregory Szorc [:gps]]

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIPrefService#savePrefFile%28%29 is relevant to fixing this.

Comment 3

[Sankha Narayan Guria [:sankha]]

Can I work on this bug?

Comment 4

[Gregory Szorc [:gps]]

(In reply to Sankha Narayan Guria from comment #3)
> Can I work on this bug?

Yes. As we talked on IRC, I think you have a good idea on what needs done.

I'd like a Prefs peer to sign off on the eventual patch. CC bsmedberg

Also, I have no clue how to actually test this. Does the Prefs API have an observer that fires after prefs flush to disk that a test can listen for? If not, how can a test verify this reliably?

Comment 5

[Josh Matthews [:jdm]]

(In reply to Gregory Szorc [:gps] from comment #4)
> Also, I have no clue how to actually test this. Does the Prefs API have an
> observer that fires after prefs flush to disk that a test can listen for? If
> not, how can a test verify this reliably?

It does not. It just writes to a temporary file, removes the old file and renames the new one.

Comment 6

[Sankha Narayan Guria [:sankha]]

(In reply to Josh Matthews [:jdm] from comment #5)
> It does not. It just writes to a temporary file, removes the old file and
> renames the new one.

Then is there any other way we can test this?

Comment 7

[Josh Matthews [:jdm]]

If nobody objects, you could add a new notification and use an observer in the test.

Comment 8

[Sankha Narayan Guria [:sankha]]

Attachment: Pref save forced after sync setup

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 643961 [details] [diff] [review]
Pref save forced after sync setup

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

I still need to verify all the potential call sites for weave:service:setup-complete. So, leaving review open.

FWIW, savePrefFile() will raise an exception if it fails. However, if prefs save fails, things are probably really messed up. Nowhere else in the source tree seems to handle this gracefully. So, I'm OK leaving out a try..catch.

As for testing, perhaps we could verify that the mtime of prefs.js changes when expected? I'm not sure if it has all the features yet, but OS.File (https://developer.mozilla.org/en/JavaScript_OS.File) might be a good tool for this. Otherwise, we'll need to use some XPCOM interfaces, like nsILocalFile. I can help you with this, if needed.

::: services/sync/modules/policies.js
@@ +194,5 @@
>          break;
>        case "weave:service:setup-complete":
> +         let syncPrefs = Cc["@mozilla.org/preferences-service;1"]
> +                           .getService(Ci.nsIPrefService);
> +         syncPrefs.savePrefFile(null);

I just realized that you should be able to use Services.prefs to get at the global nsIPrefService instance.

"Services" is a magic global variable inside Firefox code, so it should just work.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 643961 [details] [diff] [review]
Pref save forced after sync setup

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

OK. I just verified that setup-complete only gets fired during initial client configuration. I was worried that it fired every time Sync was initialized, which would result in a lot of overhead. So, the only change that needs to be made is to reference Services.prefs instead of doing the XPCOM foo.

Comment 11

[Sankha Narayan Guria [:sankha]]

(In reply to Gregory Szorc [:gps] from comment #10)
> Comment on attachment 643961 [details] [diff] [review]
> Pref save forced after sync setup
> 
> Review of attachment 643961 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK. I just verified that setup-complete only gets fired during initial
> client configuration. I was worried that it fired every time Sync was
> initialized, which would result in a lot of overhead. So, the only change
> that needs to be made is to reference Services.prefs instead of doing the
> XPCOM foo.

Ok. I will submit a new patch with Services.prefs change. As for the tests, I went through the documentation, it seems that we will have to use the nsIFile interface to get the last modified time. But how do I get the path to prefs.js?

Sorry, I am not getting much time to talk to you on IRC due to the timezone difference.

Comment 12

[Josh Matthews [:jdm]]

You can get the nsIDirectoryServiceProvider service and call GetFile("PrefF") to get a file object representing prefs.js.

Comment 13

[Josh Matthews [:jdm]]

Sorry, you'll need something like GetFile("PrefF", {}). See MXR for examples.

Comment 14

[Sankha Narayan Guria [:sankha]]

Attachment: Previous patch updated with test and Services.prefs

@gps: Please give feedback on this updated patch. I am having problems running the xpcshell-test on my system, so could not run the tests.

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 647242 [details] [diff] [review]
Previous patch updated with test and Services.prefs

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

::: services/sync/tests/unit/test_service_login.js
@@ +66,5 @@
>  }
>  
>  add_test(function test_login_logout() {
> +  let prefFile = FileUtils.getFile("PrefF", {});
> +  let oldmTime = prefFile.lastModifiedTime;

Running this locally, FileUtils.jsm doesn't like PrefF because it has no path components. That sounds like a bug in FileUtils. Boo.

I'll modify the patch on my machine to do it the old-fashioned way.

FWIW, no code in mozilla-central is using the "PrefF" identifier for the directory service! I guess Sync will be the first :D

Comment 16

[Gregory Szorc [:gps]]

It appears there isn't a prefs.js file in the xpcshell profile directory. The plot thickens...

Comment 17

[Gregory Szorc [:gps]]

After wrangling with this for a little while, I gave up trying to get the test to work. Pity.

I removed the test code from the patch and committed:

https://hg.mozilla.org/services/services-central/rev/cb8e301cc81d

I verified locally that signing up for Sync causes the prefs.js file in the profile to update. QA will also need to verify this, of course.

I feel bad for committing without a test. But, sometimes that's what you need to do. I further feel bad for making Sankha work on a test only to ultimately not include it. This happens sometimes, unfortunately.

Anyway, Sankha, thank you for the patch and your work! This should be part of Firefox 17.

Comment 18

[Sankha Narayan Guria [:sankha]]

Hmmm. Its sad that the test could not be added. But my work did not go waste Gregory. I went through FileUtils, nsILocalFile, etc. docs before I wrote that test. It was good to familiarize myself with those as well. :)

Comment 19

[Tracy Walker [:tracy]]

Verified with s-c build from 20120802  - prefs.js file is updated on Sync account creation

Comment 20

[Gregory Szorc [:gps]]

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

This will make it in to Firefox 17.