fix: issue 1909: call chromiumLaunch with parameter userDataDir instead of --user-data-dir and set of --profile-directory #1920

pearnaly · 2020-06-04T14:31:37Z

fix: issue 1909: call chromiumLaunch with parameter userDataDir instead of --user-data-dir and set of --profile-directory
See discussion #1909
Thank you @Rob--W

coveralls · 2020-06-04T14:39:14Z

Coverage remained the same at 100.0% when pulling e344e6c on pearnaly:pearnaly_master into e3f4d7f on mozilla:master.

Rob--W

See comment below, and please undo the unrelated formatting changes from the patch.

Rob--W · 2020-06-04T15:07:16Z

src/extension-runners/chromium.js

-      chromeFlags.push(`--user-data-dir=${this.params.chromiumProfile}`);
+      const pathParts = path.parse(this.params.chromiumProfile);
+      const profileDir = pathParts.base;
+      chromeFlags.push(`--profile-directory=${profileDir}`);


This does not implement what I described at #1909 (comment)

According to your comment at #1909 (comment), the wrong --user-data-dir is being used. To fix that, the directory should be passed via userDataDir instead of chromeFlags.

The --profile-directory flag is not necessary. Anyone who cares can use the method that I described in #1909 (comment)

Yes exactly! I changed it this way to have it fully compatible with --firefox-profile, so that the user provides the full path to the profile, and not the folder containing the profiles. This way the user also doesn't have to deal with the undocumented --profile-directory flag. The documentation of --chromium-profile ("Path to a custom Chromium profile") remains correct this way.

If the default --user-data-dir is correct, it is also possible to provide only the name of the profile, like for Firefox.

Do you have concerns?

To simplify @Rob--W , --chromium-profile = --user-data-dir + --profile-directory

For the most common scenarios (using a temporary directory, or even a copy of a single-user profile), just passing --user-data-dir is sufficient. I don't want to complicate web-ext with --profile-directory, because this is a special case AND already possible without special support from web-ext (i.e. the --arg flag).

Okay for me @Rob--W, but it will mean:

change the documentation of --chromium-profile from "Path to a custom Chromium profile." to "Path to the directory containing the profiles" (or equivalent). Can you organize this change?

loose the compatibility with --firefox-profile

I don't agree with it, but if you insist I do the change since I would like to have it finally somehow working.

There are two bugs here:

According to your comment at Unable to use custom chromium profile. --chromium-profile flag is not getting respected. #1909 (comment) , the --chromium-profile flag fails to match the specified directory on Windows (even though it works fine on Linux). This is a bug that we should certainly fix.

Now in this PR and thread, you are advocating for support of a specific "User Account" within a given user data directory, via the --profile-directory flag. This is more work than needed to fix the previous bug, and I didn't feel the need to delay the fix of the previous bug for this new feature.

I'm not adamant against supporting User Accounts, but note that it will take some effort. I'll post a separate comment with lots of more details, I hope that it helps.

Rob--W · 2020-06-06T21:10:15Z

Since you appear to be very interested in supporting Chrome's "Profiles" other than Default, here are a few notes:

--user-data-dir is the primary mechanism to create separate data directories for running parallel instances of the Chrome browser.
--profile-directory cannot live on its own without --user-data-dir (when unspecified, some default user data directory is chosen).
When --profile-directory is used, the state of the user data directory changes (the default profile then changes to the selected profile directory). Consequently, using a profile within the default user data directory may cause unexpected behavior. For example, when an external application opens a link, a new tab will be opened in the (last opened) testing profile rather than the actual chosen (default) profile from the user.
web-ext run currently supports the selection of Firefox profiles by name rather than path. This is not documented nor supported in web-ext run with Chrome. To have the same feature for Chrome (using the system's default user data dir), we would have to detect the location of the default user data dir, and pass --profile-directory if needed. There is ambiguity: does --chromium-profile "Profile 1" mean "Profile 1" of the default user data dir, or "Profile 1" in the current directory? Chrome will create the user data dir/profile if it does not exist. This ambiguity is bad and is destructive.
The official chrome-launcher package does not support --profile-directory ( How to automate non-default profile dynamically? GoogleChrome/chrome-launcher#172 / Directory "\Default" is added to userDataDir GoogleChrome/chrome-launcher#190). Once they figure out a standard way to select a specific directory (e.g. even without --user-data-dir support), then it would be easier to support the same here in web-ext.
Changing the meaning of --chromium-profile is a backwards-incompatible change for existing users of web-ext. With the proposed change, Chrome is going to create extra directories / files in the parent directory of the --chromium-profile path, which would be very unexpected and therefore undesirable.
If you are interested in introducing support for combing --user-data-dir and --profile-directory, make sure that there is no ambiguity whatsoever before using the parent directory. You need to make sure that the parent directory is really the user data dir, AND that the given directory is NOT a user data dir (AND maybe even that the given directory is a profile directory). If unclear, treat the given path as a user-data-dir.

_{(edited to use numbers instead of bullet points)}

pearnaly · 2020-06-09T17:39:34Z

Hi @Rob--W, okay I see now the conflict between the two chrome mechanisms for profiles. both directories levels can be considered as "profiles", which is confusing. According to the chromium docu, the naming is "profile"/"account", but according to the command line params names and to the chrome://version/ page it is "data dir"/"profile".
Yes you can provide only --profile-directory (on my Windows at least). in this case Chrome takes the default user-data-dir (like for Firefox).
Yes it is true, using --profile-directory changes the default profil in user-data-dir. But is is the same behavious if you provide it using this fix or passing the parameters as you proposed. At least, if the users uses profiles in his everyday user-data-dir to debug, it is his problem (actually wanted) that these data are modified (what I were looking for). Users who don't want it will provide a path different from the defaut one or won't use it to use the temp profile.
It is fortunately not necessary to detect the position of the default used-data-dir. We just have to not provide it and chrome will use the default. But you are right the documentation has to be anyway extended, either to explain that --chromium-profile has the meaning of user-data-dir and profile-directory is not supported by the lib and can be passe manually OR that --chromium-profile has the meaning of the profile path according to chrome://version/ and that (new!) only the profile directory can be provided, as for Firefox.
the chrome launcher doesn't support this flag, and it is probably okay since we can pass it ourselves. But if they start to support it yes we will have to make sure they don't try to set some colliding default.
It is indeed the critical point... I was pushing to this change since I considered as a bug that the docu named chromium-profile what would be the profiles container according to the chrome://version/ naming schema. Is actually also the way @deepTier5 understood it in his comment. I think most of the user will instinctively want to provide the full path. Since the chromium support is quite new and the profile flag probably few used (?) and has this windows bug anyway, it could be our last chance to change (fix) its meaning, but I understand your concern and leave it to you.
No I think double support in the same param is too complicated and even unpossible to tell apart. It will be either/or. Linux users currently using it would have to add /Default. But it is more clear on the long term.

What should I do?

Rob--W · 2020-06-09T18:04:52Z

2. Yes you can provide only `--profile-directory` (on my Windows at least). in this case Chrome takes the default `user-data-dir` (like for Firefox).

3. Yes it is true, using `--profile-directory` changes the default profil in `user-data-dir`. But is is the same behavious if you provide it using this fix or passing the parameters [as you proposed](https://github.com/mozilla/web-ext/issues/1909#issuecomment-636385559). At least, if the users uses profiles in his everyday `user-data-dir` to debug, it is his problem (actually wanted) that these data are modified (what I were looking for). Users who don't want it will provide a path different from the defaut one or won't use it to use the temp profile.

4. It is fortunately not necessary to detect the position of the default used-data-dir. We just have to not provide it and chrome will use the default.

web-ext intentionally tries to avoid damaging the user profile unless explicitly requested.
When a Firefox profile name is given instead of a path, web-ext run --keep-profile-changes -p [name here] will look up the location and copy it:

web-ext/src/firefox/index.js

Lines 404 to 447 in f2043c7

    
           /* 
        
            * Copies an existing Firefox profile and creates a new temporary profile. 
        
            * The new profile will be configured with some preferences required to 
        
            * activate extension development. 
        
            * 
        
            * It resolves with the new profile object. 
        
            * 
        
            * The temporary profile will be deleted when the system process exits. 
        
            * 
        
            * The existing profile can be specified as a directory path or a name of 
        
            * one that exists in the current user's Firefox directory. 
        
            */ 
        
           export async function copyProfile( 
        
             profileDirectory: string, 
        
             { 
        
               app, 
        
               configureThisProfile = configureProfile, 
        
               copyFromUserProfile = defaultUserProfileCopier, 
        
               customPrefs = {}, 
        
             }: CopyProfileOptions = {}, 
        
           ): Promise<FirefoxProfile> { 
        
             const copy = promisify(FirefoxProfile.copy); 
        
             const copyByName = promisify(copyFromUserProfile); 
        
             try { 
        
               const dirExists = await isDirectory(profileDirectory); 
        
               let profile; 
        
               if (dirExists) { 
        
                 log.debug(`Copying profile directory from "${profileDirectory}"`); 
        
                 profile = await copy({profileDirectory}); 
        
               } else { 
        
                 log.debug(`Assuming ${profileDirectory} is a named profile`); 
        
                 profile = await copyByName({name: profileDirectory}); 
        
               } 
        
               return configureThisProfile(profile, {app, customPrefs}); 
        
             } catch (error) { 
        
               throw new WebExtError( 
        
                 `Could not copy Firefox profile from ${profileDirectory}: ${error}`); 
        
             } 
        
           }

In order to support this for Chrome, there should also be something similar.

5. the chrome launcher doesn't support this flag, and it is probably okay since we can pass it ourselves. But if they start to support it yes we will have to make sure they don't try to set some colliding default.

I mentioned this not only because of potential compat concerns, but also to point out that the odds of having proper support for this feature increases if the official upstream project properly supports the feature.

6. It is indeed the critical point... I was pushing to this change since I considered as a bug that the docu named `chromium-profile` what would be the profiles container according to the `chrome://version/` naming schema. Is actually also the way @deepTier5 understood it in [his comment](https://github.com/mozilla/web-ext/issues/1909#issuecomment-636323174). I think most of the user will instinctively want to provide the full path. Since the chromium support is quite new and the profile flag probably few used (?) and has this windows bug anyway, it could be our last chance to change (fix) its meaning, but I understand your concern and leave it to you.

7. No I think double support in the same param is too complicated and even unpossible to tell apart. It will be either/or. Linux users currently using it would have to add `/Default`. But it is more clear on the long term.

What should I do?

The default behavior should be to treat the flag as --user-data-dir. I see your points on the user taking the directory location from chrome://version (in the specific scenario of re-using an existing profile). That specific case could be supported by detecting the directories (there are some files/directories that are specific to user-data-dir / profile-dir, it is not as impossible as it seems).

I'll have to discuss this with other project members, but I currently believe that the path forwards is 7 and/or awaiting official upstream support (in chrome-launcher).

Rob--W · 2020-06-11T16:23:07Z

I've discussed this with @rpl and we have agreed that the following approach will provide the requested functionality with reasonable compatibility:

Only one parameter, --chromium-profile.
If the directory exists, check if it is certainly a profile directory and NOT a user-data-dir. If this cannot be decided, keep the current behavior (treat as user-data-dir) (end of steps).
If --keep-profile-changes is used, check if the parent directory is a user-data-dir. If it is, then use the given profile. If it is not, then we should not use the parent directory as the user-data-dir (whether to exit with an error or treating it as --user-data-dir is up to you).
Otherwise (by default, if --keep-profile-changes is not set), create a new directory (to use as user-data-dir) and put the non-Default profile directory in it (and pass it to --profile-directory).

Note that this approach favors the use of --user-data-dir when in doubt. This is reasonable because users can always use --arg=" --profile-directory=Profile 1" if wanted.

To detect whether a directory looks like a user-data-dir: Check if the "Local State" file and "Default" directory exist.
To detect whether a directory looks like a profile-directory: Check if the given directory exists and the "Secure Preferences" file exist.
I looked in a new profile and in a 9-year-old Chromium profile, and confirmed that checking the existence of those files doesn't cause false positives (at first --user-data-dir contained the actual profile. Based on timestamps it seems that somewhere in 2011 the profile moved to the Default subdirectory).

pearnaly · 2020-06-13T04:59:40Z

It's fine for me and I started.
I use sofar fs-extra for the files operations
I will have a hard time with the tests, I will try with mock-fs...

…ad of --user-data-dir

…recognize if it is meant as --user-data-dir or --user-data-dir/--profile-directory

Rob--W

One big function with multiple nested if-else blocks is not very readable. To increase the readability and maintainability, I suggest to move the new logic to a separate method. That avoids the need for bloating the setupInstance function, and makes it easier to specifically create unit tests without too much boilerplate.

Specifically, untangle the current logic and split it in three parts:

Find out user-data-dir and profile-directory
Copy directory locations if needed.
Pass userDataDir / chromeFlags if needed (this is a relatively small part that does belong to setupInstance).

This makes the code easier to follow, and also easier to test.

On the tests: There are three tests, for the following case:

path is a profile-directory, with keepProfileChanges
path is a profile-directory, but parent is not a user-data-dir, with keepProfileChanges - exit out of caution.
path is a profile-directory, parent is not a user-data-dir, without keepProfileChanges.

This only covers some common/desired situations, but is not complete, as there are several conditions that are not covered. Since we are working with user data, I want to make sure that all relevant states are fully covered by unit tests (we don't want to accidentally damage real user profiles). I expect that tests will be smaller and easier to write after splitting the logic in three parts as I suggested.

.eslintrc

Rob--W · 2020-06-15T19:49:00Z

src/extension-runners/chromium.js

  default as ChromeLauncher,
+  launch as defaultChromiumLaunch,
+  LaunchedChrome, Launcher,


This will need to change due to the chromium-launcher update from #1931.

I reverted this import

Rob--W · 2020-06-15T19:54:01Z

src/extension-runners/chromium.js

+      const localStateCandidate = path.join(dirCandidate, 'Local State');
+      const defaultCandidate = path.join(dirCandidate, 'Default');
+      // Local State and Default are typical for the user-data-dir
+      return fs.pathExistsSync(localStateCandidate)


Since we're trying hard to minimize the risk of misidentification, let's verify that the given paths are really directories / files. We have util.isDirectory and util.isFile for that: https://github.com/mozilla/web-ext/blob/588550008c1d0381736f435956095255200707d5/src/util/is-directory.js

For files we also have file.fileExists - https://github.com/mozilla/web-ext/blob/588550008c1d0381736f435956095255200707d5/src/util/file-exists.js

I switched to isDirectory. The use of fs-extra is now reduced, and we could reevaluate if it is still justified.

src/extension-runners/chromium.js

Rob--W · 2020-06-15T20:11:01Z

src/extension-runners/chromium.js

+        && !isUserDataDir(chromiumProfile);
+
+      if (isProfileDirAndNotUserData) {
+        const pathParts = path.parse(chromiumProfile);


Let's use the following:

const { dir: parentDir, base: profileName } = path.parse(chromiumProfile);

The parentDir variable makes it very obvious that we're working with parent directories, which removes the need for the // now check if the parent dir is a user-data-dir comment below.

Rob--W · 2020-06-15T20:19:33Z

src/extension-runners/chromium.js

+          // the user provided an existing profile directory but doesn't want
+          // the changes to be kept. we copy this directory to a temporary
+          // user data dir.
+          userDataDir = Launcher.prototype.makeTmpDir();


Don't use undocumented internal implementation details from an external library.

The web-ext project itself already has an utility to create temporary directories here: https://github.com/mozilla/web-ext/blob/9315b781ec73eead946c07551a24a413ddb1338a/src/util/temp-dir.js

An example of usage is here: https://github.com/mozilla/web-ext/tree/9315b781ec73eead946c07551a24a413ddb1338a/src/extension-runners

I switched to TempDir, but with TempDir you cannot just get a temp path without creating the directory itself. I have to delete it and rewrite it after, which is quite ugly. My method would like to just get a path, and create the actual dir separately...

src/extension-runners/chromium.js

Rob--W · 2020-06-15T20:28:05Z

src/extension-runners/chromium.js

+        }
+        chromeFlags.push(`--profile-directory='${profileDirName}'`);
+      } else {
+        userDataDir = chromiumProfile;


Prepend comment to explain what chromiumProfile means at this point, so that the reader doesn't have to scroll all the way up.

And this doesn't properly handle the --keep-profile-changes case - see my top-level review comment for suggestions on improving this.

I made an early return statement for it, so it should be clear now

tests/unit/test-extension-runners/test.chromium.js

Rob--W · 2020-06-15T20:53:33Z

package.json

@@ -65,6 +65,7 @@
    "es6-error": "4.1.1",
    "event-to-promise": "0.8.0",
    "firefox-profile": "1.3.1",
+    "fs-extra": "9.0.1",


I'm not keen on adding new dependencies because it generally results in more maintenance load (e.g. having to publish new versions when a security issue is found in the dependency). But since we're already indirectly depending on this module, I'm fine with it.

In a follow-up issue/PR we can probably replace mz with this, since mz's functionality overlaps with fs-extra.

…recognize if it is meant as --user-data-dir or --user-data-dir/--profile-directory

pearnaly · 2020-06-17T20:13:19Z

I think my 3 tests test all the pathes. Do you see another test case?

pearnaly · 2020-06-17T20:40:10Z

I have an issue with the error test. npm run test is passing on my computer but failing on the CI pipeline. I will have a look at it again tomorrow

Rob--W

I think my 3 tests test all the pathes. Do you see another test case?

Yes, lots of cases are missing. The purpose of splitting getProfilePaths (renamed from getProfileParts - see below) is to make it easier to unit test every possible case. There are bugs in the implementation, and before fixing the bugs that I pointed out, I suggest that you first write some unit tests with expected behavior, and then run the tests with your current (buggy) implementation. Those tests are then expected to fail. After fixing the bugs, the tests are expected to pass.

Think of all possible ways that getProfilePaths can fail, and for each scenario that you can come up with, create a temporary directory with that filesystem layout. Think of missing files/directories, files being directories, directories being files, a file/directory being a symlink to a file/directory, and generally anything that can cause confusion on whether the given path refers to a user-data-dir or a profile-directory.

With thorough unit tests for getProfilePaths, the number of tests with an instance of ChromiumExtensionRunner is reasonably less. The interesting cases are combinations of: userDataDir exists/does not exist, profileDir exists/does not, with and without --keep-profile-changes.

(the next part is for completeness, but please focus on the previous parts before concerning yourself with the next part.)
Finally, after having those unit tests, I also recommend to actually run web-ext run with special characters in the profile directory name, and see if Chrome starts as expected. Think of double quotes and single quotes (apostrophes) (at the start, in the middle and at the end of the directory name). These characters have special meanings in the shell, so make sure to properly escape them, or use webExt as a library and pass the variables directly as the chromiumProfile parameter. If you find that there is a bug (e.g. the path name is incorrectly interpreted), then fix it and add a unit test.

I have an issue with the error test. npm run test is passing on my computer but failing on the CI pipeline.

The tests themselves pass on CI, but the code coverage check fails because there is a regression in code coverage. When I open the coverage file, I can clearly see an uncovered condition, which should really have been covered by a unit test: https://coveralls.io/builds/31520533/source?filename=src%2Fextension-runners%2Fchromium.js#L129

Sometimes the coveralls report is incorrect, but in this case it did correctly point to missing coverage (and incidentally, an actual bug that would have been revealed if there was test coverage).

Rob--W · 2020-06-25T17:12:26Z

src/extension-runners/chromium.js

+    const localStateCandidate = path.join(dirCandidate, 'Local State');
+    const defaultCandidate = path.join(dirCandidate, 'Default');
+    // Local State and Default are typical for the user-data-dir
+    return await isDirectory(localStateCandidate)


"Local State" is a file, not a directory.

And replace the "Candidate" suffix with "Path", everywhere.

Rob--W · 2020-06-25T17:12:49Z

src/extension-runners/chromium.js

+    const securePreferencesCandidate = path.join(
+      dirCandidate,
+      'Secure Preferences');
+    //Secure Preferences is typical for a profile dir


"profile dir" -> "profile dir inside a user data dir."

Rob--W · 2020-06-25T17:15:14Z

src/extension-runners/chromium.js

+  }
+
+  static async getProfileParts(params: any): Promise<{
+    userDataDir: (string | false),


Make it a ?string (= string or null / undefined) instead of a union type composed of string + false.

Rob--W · 2020-06-26T13:41:11Z

src/extension-runners/chromium.js

+    return await isDirectory(securePreferencesCandidate);
+  }
+
+  static async getProfileParts(params: any): Promise<{


Rename to getProfilePaths(chromiumProfile: ?string)

params: any is too broad, see below.

Rob--W · 2020-06-26T13:42:30Z

src/extension-runners/chromium.js

+      // the changes to be kept. we copy this directory to a temporary
+      // user data dir.
+      const tmpDir = new TempDir();
+      await tmpDir.create();


It's not the responsibility of this function to create a temporary non-existing directory. getProfilePaths should only parse the path and return its best guess. Callers can then use this to copy the directories if needed.

Rob--W · 2020-06-26T14:05:35Z

src/extension-runners/chromium.js

@@ -127,8 +193,21 @@ export class ChromiumExtensionRunner {
      chromeFlags.push(...this.params.args);
    }

-    if (this.params.chromiumProfile) {
-      chromeFlags.push(`--user-data-dir=${this.params.chromiumProfile}`);
+    const profileParts =


Instead of profileParts, expand the variables as follows (note with let instead of const because we may update the value - see further).

let { userDataDir, profileDirName } = ...

and then check keepProfileChanges and copy / change the path if needed:

If keepProfileChanges is falsey:

Create a new temp dir.

If profileDirName is not set, copy userDataDir's content to that temp dir.

If profileDirName is set, copy profileDirName (as a directory, not just its content) to that temp dir.

Update userDataDir to that temp dir location.

If keepProfileChanges is truthy:

If profileDirName is set, but userDataDir is not a user data dir: Report an error. (basically move part of the logic that you've currently put in getProfileParts here)

Rob--W · 2020-06-26T14:06:19Z

src/extension-runners/chromium.js

+      await ChromiumExtensionRunner.getProfileParts(this.params);
+
+    if (profileParts.userDataDir
+      && profileParts.profileDirName


This shows a bug: If the user only specified --user-data-dir, then the absence of --keep-profile-changes is not processed.

Rob--W · 2020-06-26T15:21:44Z

src/extension-runners/chromium.js

+      && !this.params.keepProfileChanges
+    ) {
+      // copy profile dir to this temp user data dir.
+      fs.copySync(this.params.chromiumProfile, path.join(


Let's use await fs.copy, as there is no real need for this to be synchronous.

The use of fs-extra is now reduced, and we could reevaluate if it is still justified.

There are multiple (dev) dependencies with overlapping functionality. For example, mz and fs-extra both offer promisified fs methods, and fs-extra and copy-dir both offer ways to copy. I think that fs-extra is the best replacement in the long term.

…userDataDir if chromiumProfile is not set

…ically if needed

Rob--W

The new tests and changes looks good. I only have a few minor comments below.

Rob--W · 2020-07-09T14:00:30Z

src/extension-runners/chromium.js

+        );
+      } else {
+        await fs.copy(path.join(
+          userDataDir), path.join(


These path.join calls aren't needed, just pass them as-is to fs.copy.

Rob--W · 2020-07-09T14:08:09Z

tests/unit/test-extension-runners/test.chromium.js

+      await fs.outputFile(path.join(tmpPath, 'Local State'), '');
+
+      assert.isTrue(await fileExists(path.join(tmpPath, 'Local State')));
+      assert.isTrue(await isDirectory(path.join(tmpPath, 'Default')));


These two assertions are redundant. It is part of the test setup and we assume that the test setup was successful.

Rob--W · 2020-07-13T10:29:01Z

tests/unit/test-extension-runners/test.chromium.js

+    ' to chrome', async () => withTempDir(
+    async (tmpDir) => {
+      const tmpPath = tmpDir.path();
+      await fs.mkdirsSync(path.join(tmpPath, 'userDataDir/Default'));


Rob--W · 2020-07-13T10:35:02Z

tests/unit/test-extension-runners/test.chromium.js

+     })
+  );
+
+  it('does copy the profile and pass user-data-dir and profile-directory' +


You also have to verify that the relevant directories/files have been copied as expected.

src/extension-runners/chromium.js

Rob--W · 2020-07-13T16:46:42Z

src/extension-runners/chromium.js

+          'user-data-dir directory');
+      }
+    } else if (userDataDir) {
+      // the user provided an existing profile directory but doesn't want


Let's unconditionally create a temporary directory, so that we have full control over whether the temp file is deleted (see also my other comment below about #1957).

Currently, due to the way that chrome-launcher is implemented, passing userDataDir: null (instead of userDataDir: undefined) causes the temporary profile to be left behind: https://github.com/GoogleChrome/chrome-launcher/blob/08406b2804c6efba7b602fb276397df00cd0041f/src/chrome-launcher.ts#L373

This comment hasn't been addressed yet.

To address it, you need to remove if (userDataDir) { here (to turn this into } else {) and it to the } else { below.

} else if (userDataDir) { // <-- Here await fs.copy(userDataDir, tmpDirPath);

Rob--W · 2020-07-13T16:54:03Z

tests/unit/test-extension-runners/test.chromium.js

+
+      const tmpPath = tmpDir.path();
+      await fs.mkdirs(path.join(tmpPath, 'Default'));
+      await fs.mkdirs(path.join(tmpPath, 'Local State'));


Add comment that this should be a file (for those unfamiliar with how the profile is supposed to look like, which is almost everyone, I guess).

Also for other tests below.

…_master

…ad of --user-data-dir

…recognize if it is meant as --user-data-dir or --user-data-dir/--profile-directory

…userDataDir if chromiumProfile is not set

…ically if needed

…_master

# Conflicts: # package.json

Rob--W

Thanks for all of your changes. Could you make one more change (see below, because that causes chrome-launcher to leave temp files behind)? After that it should be good to land.

The tests time out for some reason, but I guess that it's unrelated to your patch.

Rob--W · 2020-07-20T08:35:16Z

src/extension-runners/chromium.js

+          'user-data-dir directory');
+      }
+    } else if (userDataDir) {
+      // the user provided an existing profile directory but doesn't want


This comment hasn't been addressed yet.

To address it, you need to remove if (userDataDir) { here (to turn this into } else {) and it to the } else { below.

} else if (userDataDir) { // <-- Here await fs.copy(userDataDir, tmpDirPath);

Rob--W · 2020-07-20T08:36:24Z

src/extension-runners/chromium.js

+      // the changes to be kept. we copy this directory to a temporary
+      // user data dir.
+      const tmpDir = new TempDir();
+      // TODO: Remove tmpDir - https://github.com/mozilla/web-ext/issues/1957


Sorry for the confusion, as Luca mentioned at https://github.com/mozilla/web-ext/pull/1920/files#r454511824 , the tmp library already removes the file at exit. So this comment can be removed.

Rob--W · 2020-07-20T13:11:06Z

I locally checked out the PR to prepare for merging, but noticed that the test failure is actually caused by your changes.

Minimal reproduction:

$ ./node_modules/.bin/mocha tests/unit/test-extension-runners/test.chromium.js -f 'throws an error on profile in invalid user-data-dir'


  util/extension-runners/chromium
    ✓ throws an error on profile in invalid user-data-dir


  1 passing (40ms)

^C
$ # ^ only exited because I explicitly triggered an exit.

In that test, you're not calling runnerInstance.exit();. As a result, the internal server that it had started doesn't exit and keeps the process alive.

Rob--W

It's green \o/

pearnaly · 2020-07-20T14:06:50Z

We got it! Thank you @Rob--W for accompanying me in my fist open source PR.

Rob--W · 2020-07-20T14:30:48Z

You're welcome, and thanks again for raising attention on issue #1909 and submitting a patch!

caitmuenster · 2020-07-20T19:59:55Z

Thanks for the patch, @pearnaly! 🎉 Your contribution has been added to our recognition wiki.

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!

pearnaly · 2020-07-22T12:33:52Z

Thank you Caitlin! i created an account: pearnaly Have a nice day Viele Grüße Paul-Émile Arnaly Am Mo., 20. Juli 2020 um 22:00 Uhr schrieb Caitlin Neiman < notifications@github.com>:

…

Thanks for the patch, @pearnaly <https://github.com/pearnaly>! 🎉 Your contribution has been added to our recognition wiki <https://wiki.mozilla.org/Add-ons/Contribute/Recognition>. Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1920 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEDBRHOJAIF4ADC52ITGCHTR4SO4ZANCNFSM4NSV3LLQ> .

caitmuenster · 2020-07-22T15:32:59Z

Excellent! Your profile has been vouched. 🎉

Welcome onboard! We look forward to seeing you around.

Rob--W requested changes Jun 4, 2020

View reviewed changes

rpl added the state: in progress label Jun 11, 2020

pearnaly force-pushed the pearnaly_master branch from d53b0db to 9a5e329 Compare June 14, 2020 10:57

pearnaly requested a review from Rob--W June 14, 2020 11:01

pearnaly added 2 commits June 15, 2020 20:52

fix: issue 1909: call chromiumLaunch with parameter userDataDir inste…

9db48df

…ad of --user-data-dir

fix: issue 1909: call chromiumLaunch with parameter userDataDir will …

9315b78

…recognize if it is meant as --user-data-dir or --user-data-dir/--profile-directory

pearnaly force-pushed the pearnaly_master branch from 9a5e329 to 9315b78 Compare June 15, 2020 18:52

Rob--W requested changes Jun 15, 2020

View reviewed changes

fix: issue 1909: call chromiumLaunch with parameter userDataDir will …

1fe2f63

…recognize if it is meant as --user-data-dir or --user-data-dir/--profile-directory

Rob--W requested changes Jun 26, 2020

View reviewed changes

pearnaly added 2 commits June 30, 2020 15:43

fix: issue 1909: some more tests, correction of false->null for flag …

a5ce3b8

…userDataDir if chromiumProfile is not set

fix: issue 1909: Correction: doubles quotes are already added automat…

3d24f8d

…ically if needed

Rob--W added this to Backlog in 5.0.0 Jul 9, 2020

Rob--W reviewed Jul 13, 2020

View reviewed changes

pearnaly added 8 commits July 20, 2020 08:49

fix: issue 1909: review fixes

da15851

fix: issue 1909: review fixes

e94c8c6

Merge remote-tracking branch 'pearnaly/pearnaly_master' into pearnaly…

8af8489

…_master

fix: issue 1909: call chromiumLaunch with parameter userDataDir inste…

8bf91a2

…ad of --user-data-dir

fix: issue 1909: call chromiumLaunch with parameter userDataDir will …

bbca399

…recognize if it is meant as --user-data-dir or --user-data-dir/--profile-directory

fix: issue 1909: call chromiumLaunch with parameter userDataDir will …

32c75a6

…recognize if it is meant as --user-data-dir or --user-data-dir/--profile-directory

fix: issue 1909: some more tests, correction of false->null for flag …

0c3bfc1

…userDataDir if chromiumProfile is not set

fix: issue 1909: Correction: doubles quotes are already added automat…

2df08e7

…ically if needed

pearnaly added 4 commits July 20, 2020 08:51

fix: issue 1909: review fixes

d5ce29b

Merge remote-tracking branch 'pearnaly/pearnaly_master' into pearnaly…

4566013

…_master

Merge remote-tracking branch 'origin/master' into pearnaly_master

9707b63

# Conflicts: # package.json

fix: issue 1909: merged (package-lock generated)

bf997c3

Rob--W reviewed Jul 20, 2020

View reviewed changes

fix: issue 1909: review

55d6541

fix: issue 1909: exit missing

e344e6c

Rob--W approved these changes Jul 20, 2020

View reviewed changes

Rob--W added state: pull request ready and removed state: in progress labels Jul 20, 2020

Rob--W merged commit 29798cb into mozilla:master Jul 20, 2020

rpl moved this from Backlog to Done in 5.0.0 Jul 22, 2020

rpl mentioned this pull request Mar 17, 2021

Browser profile directories (Chrome and Firefox) are not deleted after web-ext disconnects #2205

Open

kevin-zx mentioned this pull request Apr 2, 2024

I can't override wxt default --user-data-dir by chromiumArgs wxt-dev/wxt#586

Closed

fix: issue 1909: call chromiumLaunch with parameter userDataDir instead of --user-data-dir and set of --profile-directory #1920

fix: issue 1909: call chromiumLaunch with parameter userDataDir instead of --user-data-dir and set of --profile-directory #1920

Conversation

pearnaly commented Jun 4, 2020

coveralls commented Jun 4, 2020 • edited

Rob--W left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pearnaly Jun 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rob--W commented Jun 6, 2020 • edited

pearnaly commented Jun 9, 2020

Rob--W commented Jun 9, 2020

Rob--W commented Jun 11, 2020

pearnaly commented Jun 13, 2020

Rob--W left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pearnaly commented Jun 17, 2020

pearnaly commented Jun 17, 2020

Rob--W left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rob--W left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rob--W left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rob--W commented Jul 20, 2020

Rob--W left a comment

Choose a reason for hiding this comment

pearnaly commented Jul 20, 2020

Rob--W commented Jul 20, 2020

caitmuenster commented Jul 20, 2020

pearnaly commented Jul 22, 2020 via email

caitmuenster commented Jul 22, 2020

coveralls commented Jun 4, 2020 •

edited

pearnaly Jun 6, 2020 •

edited

Rob--W commented Jun 6, 2020 •

edited