Closed Bug 579604 Opened 11 years ago Closed 9 years ago

force a prefs file save after setup completes

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: jrmuizel, Assigned: sankha)

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Component: Firefox UI → Sync
OS: Mac OS X → All
QA Contact: firefox → sync
Hardware: x86 → All
Summary: Firefox sync reasks for setup information if browser crashes after setting up → force a prefs file save after setup completes
Target Milestone: --- → Future
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIPrefService#savePrefFile%28%29 is relevant to fixing this.
Whiteboard: [good first bug][mentor=gps][lang=js]
Can I work on this bug?
(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?
(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.
(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?
If nobody objects, you could add a new notification and use an observer in the test.
Assignee: nobody → sankha93
Attachment #643961 - Flags: review?(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 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.
Attachment #643961 - Flags: review?(gps)
(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.
You can get the nsIDirectoryServiceProvider service and call GetFile("PrefF") to get a file object representing prefs.js.
Sorry, you'll need something like GetFile("PrefF", {}). See MXR for examples.
@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.
Attachment #647242 - Flags: feedback?(gps)
Attachment #643961 - Attachment is obsolete: true
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
It appears there isn't a prefs.js file in the xpcshell profile directory. The plot thickens...
Attachment #647242 - Flags: feedback?(gps) → feedback+
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.
Whiteboard: [good first bug][mentor=gps][lang=js] → [fixed in services]
Target Milestone: Future → mozilla17
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. :)
Verified with s-c build from 20120802  - prefs.js file is updated on Sync account creation
Whiteboard: [fixed in services] → [verified in services]
https://hg.mozilla.org/mozilla-central/rev/cb8e301cc81d

This will make it in to Firefox 17.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [verified in services]
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.