prefs option is incorrectly treated as a flat Record<string,JSONLike>, nested objects are not merged but overwritten

[Rob--W]

The chrome-launcher implementation assumes that prefs are a flat dictionary-like object.

• Type definition:
  https://github.com/GoogleChrome/chrome-launcher/blob/v1.1.1/src/chrome-launcher.ts#L34
• Unit tests:
  https://github.com/GoogleChrome/chrome-launcher/blob/v1.1.1/test/chrome-launcher-test.ts#L73-L96
• Implementation merges prefs with existing Preferences with a top-level merge:
  https://github.com/GoogleChrome/chrome-launcher/blame/v1.1.1/src/chrome-launcher.ts#L247-L250

This assumption is incorrect. Preferences are stored/read as nested properties: https://source.chromium.org/chromium/chromium/src/+/main:components/prefs/json_pref_store.cc;l=215-267;drc=34ad7f3844f882baf3d31a6bc6e300acaa0e3fc8

Here is an example:

{"download": { "open_pdf_in_system_reader": false } }
+
{"download": { "default_directory": "/tmp/" } }

Given the above example, I would expect the following:

{
  "download": {
    "open_pdf_in_system_reader": false,
    "default_directory": "/tmp/"
  }
}

The actual result, however, is as follows (because the top-level "download" key is overwritten):

{"download": { "default_directory": "/tmp/" } }

To fix this, one could replace the top-level merge with a deepmerge, at
https://github.com/GoogleChrome/chrome-launcher/blame/v1.1.1/src/chrome-launcher.ts#L247-L250 . One potential downside to this is that it becomes impossible to clear an existing value, but I don't think that this is a big deal: Because the set of meaningful pref is fixed (or at least known), anyone who cares about clearing could explicitly list all relevant prefs with the default value instead of specifying something like {"download":{}}.

[connorjclark]

Thanks for reporting. I've confirmed the issue.

Seems the original code contribution was mistaken, thinking that the disk format for this is a flat object. But I think the dotted path format is only used as a key inside Chromium when interfacing with the preference value store. This test suggests that to me. I don't see anything that would normalize from a flat structure to the expected nested structure when reading the preferences file, so I don't think this ever worked. I confirmed by setting my preferences file to {"download.default_directory":"/some/path"} and found that the download directory was not altered.

I don't think the use case is well-defined enough for me to answer this, but I question why we want to merge anything here at all. If one has an existing user data directory, they could grab Default/Preferences, modify, and pass as prefs to chrome launcher (or just save to disk). By merging ourselves (well, trying to...) we are excluding the possibility of removing existing unwanted preferences, as you mentioned. It seems the most basic building block here would be for chrome launcher to override the preferences totally - and defer anything more complex to the user.

I also don't like how this option handles an existing preferences file by overwriting / destroying it - doing any destructive action to an existing chrome profile seems undesirable - or at least unexpected. Ideally we would set the existing preferences aside and restore when done, but that has other issues (do users expect preferences changed during use to persist? what happens if chrome launcher is force-killed and can't restore it?). So I guess we should just document this destructive behavior.

tldr, i think we need to:

• update the README.md comment for prefs to not use the incorrect example
• document that prefs with an existing chrome profile is destructive
• drop the broken merging functionality (or fix it)

ref #213 #247
cc @paulirish
cc @christian-bromann @Niek (I hope you can share some insights, as users/contributors of this feature)