1367813 - Remove readUserPref API?

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement, P3)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

See bug 789945 comment 94 - some add-ons use readUsePref API, this may cause a main thread synchronized pref file save.

Comment 1

[Benjamin Smedberg]

Attachment: Bug 1367813 - 1) Add telemetry for prefs.js not existing or being corrupted, and the presence of a user.js file. 2) Rename and change the nsIPrefService.readUserPrefs API. The new API reads user prefs from a file but doesn't remember that location or save

Review commit: https://reviewboard.mozilla.org/r/150952/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150952/

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8879611 [details]
Bug 1367813 - 1) Add telemetry for prefs.js not existing or being corrupted, and the presence of a user.js file. 2) Rename and change the nsIPrefService.readUserPrefs API. The new API reads user prefs from a file but doesn't remember that location or save

https://reviewboard.mozilla.org/r/150952/#review155742

::: modules/libpref/Preferences.cpp:97
(Diff revision 1)
>  static NS_DEFINE_CID(kZipReaderCID, NS_ZIPREADER_CID);
>  
>  void
>  Preferences::DirtyCallback()
>  {
> +  if (!XRE_IsParentProcess()) {

We could just not hook up the callback function in Preferences::Init by not calling PREF_SetDirtyCallback() when we're not in the parent process?

::: modules/libpref/nsIPrefService.idl:122
(Diff revision 1)
> +   * @param aFile The file to be read.
> +   *
> +   * @throws Error File failed to read or contained invalid data.
> +   * @note This method is intended for internal unit testing only!
> +   */
> +  void readUserPrefsFromFile(in nsIFile aFile);

Minor naming thing; this now feels more like "importPrefsFromFile" rather than "read..." as we maintain no connection to the file afterwards.  No big deal either way.

::: modules/libpref/test/unit/test_dirtyPrefs.js
(Diff revision 1)
>    let userBranch = ps.getBranch("");
>  
>    let prefFile = do_get_profile();
>    prefFile.append("prefs.js");
>  
> -  // dirty flag only applies to the default pref file save, not all of them,

Isn't this test now going to modify the default preference file?  Is that OK for the other tests?

::: modules/libpref/test/unit/test_libPrefs.js:322
(Diff revision 1)
>  
>    // load a preexisting pref file
>    let prefFile = do_get_file("data/testPref.js");
> -  ps.readUserPrefs(prefFile);
> +  ps.readUserPrefsFromFile(prefFile);
> +
> +  do_print("before throw tests");

Assuming this and the other do_print are not needed in the version we'd land?

Comment 3

[Benjamin Smedberg]

> We could just not hook up the callback function in Preferences::Init by not
> calling PREF_SetDirtyCallback() when we're not in the parent process?

We could, but I'd like to try adding an assert in the future and so I'd prefer to keep this explicit.

> > -  // dirty flag only applies to the default pref file save, not all of them,
> 
> Isn't this test now going to modify the default preference file?  Is that OK
> for the other tests?

It is, and I checked that should be ok for the test. It's an xpcshell test so the profile is a throwaway directory.

> Assuming this and the other do_print are not needed in the version we'd land?

Ah yeah, leftover debugging code.

Comment 4

[Benjamin Smedberg]

Comment on attachment 8879611 [details]
Bug 1367813 - 1) Add telemetry for prefs.js not existing or being corrupted, and the presence of a user.js file. 2) Rename and change the nsIPrefService.readUserPrefs API. The new API reads user prefs from a file but doesn't remember that location or save

rweiss for data review. This is type 1 data.

Comment 5

[Benjamin Smedberg]

Comment on attachment 8879611 [details]
Bug 1367813 - 1) Add telemetry for prefs.js not existing or being corrupted, and the presence of a user.js file. 2) Rename and change the nsIPrefService.readUserPrefs API. The new API reads user prefs from a file but doesn't remember that location or save

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150952/diff/1-2/

Comment 6

[[e-mail]]

Comment on attachment 8879611 [details]
Bug 1367813 - 1) Add telemetry for prefs.js not existing or being corrupted, and the presence of a user.js file. 2) Rename and change the nsIPrefService.readUserPrefs API. The new API reads user prefs from a file but doesn't remember that location or save

https://reviewboard.mozilla.org/r/150952/#review156770

In accordance with the Data Collection categories on the wiki (https://wiki.mozilla.org/Firefox/Data_Collection), I assert that this is Type 1 data, and thus eligible for default on collection in release.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f0ee0f75a30
1) Add telemetry for prefs.js not existing or being corrupted, and the presence of a user.js file. 2) Rename and change the nsIPrefService.readUserPrefs API. The new API reads user prefs from a file but doesn't remember that location or save changed preferences to that location. r=milan data-r=rweiss

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4f0ee0f75a30

Comment 9

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/mozilla-central/rev/6fed2516a664ecb9c2793b9beb0f081bc5833764 because something in that push made "dom/filesystem/tests/test_worker_basic.html | Something when wrong" permaorange on Windows opt, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=109432905&repo=mozilla-inbound

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66aba634f34d
1) Add telemetry for prefs.js not existing or being corrupted, and the presence of a user.js file. 2) Rename and change the nsIPrefService.readUserPrefs API. The new API reads user prefs from a file but doesn't remember that location or save changed preferences to that location. r=milan data-r=rweiss

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/66aba634f34d