Correctly prepare Firefox profiles with required preferences for all scenarious by whimboo · Pull Request #7684 · puppeteer/puppeteer

whimboo · 2021-10-13T09:40:53Z

PR to fix issue #7615 by making sure that also temporary folders for the Firefox profile are properly prepared with required preferences.

whimboo · 2021-10-13T12:25:05Z

@jschfflr let me know if this is something that would work for you. As already asked on the issue itself, I wonder if it would make sense to pass the userDataDir via the launch options. Or if this could be a follow-up given that it would also affect Chrome.

whimboo · 2021-10-13T12:30:08Z

Here are the first results for this PR:
https://github.com/puppeteer/puppeteer/actions/runs/1337331528/attempts/1

I will trigger some more to make sure it also succeeds all the time in CI.

OrKoN

LGTM thanks (let's let Jan to take a look as well)

whimboo

During an internal discussion two important points have been raised and need to be clarified first. Please see my inline comments.

whimboo · 2021-10-14T18:28:08Z

+        firefoxArguments.push(temporaryUserDataDir);
+      } else {
+        const prefs = this.defaultPreferences(extraPrefsFirefox);
+        await this.writePreferences(prefs, profilePath);


@jschfflr what's the actual expected behavior for user profiles that exists but should be used once for Puppeteer. Right now we would overwrite all the preferences which is not ideal. Instead we could only write the user.js file, which values will automatically be copied into prefs.js, and maybe fail if that file already exists?

Otherwise creating a backup of the prefs.js would be an idea, but there might be cases when the clean-up doesn't really work and we might miss to restore it.

That's an interesting case. Would firefox still work normally with the same user profile after the changes to users.js?

Any preferences in user.js will be copied by Firefox to prefs.js and just removing user.js will not be enough to get it reverted. That means that all the changes will remain post Puppeteer.

Given that we tweak the profile for automation the user might indeed see differences in Firefox' behavior afterward. I see two options:

Create a backup of prefs.js and restore it during teardown, which will not always work like in cases the user hits Ctrl+C. Also changes to prefs.js will trigger different behaviors for various components in Firefox, and can cause other files in the profile to get modified too.

Create a copy of the user's profile in the temp folder and let Puppeteer remove it as usual.

Only with option 2 the users profile will be kept identical and no traces from a usage with Puppeteer will remain in the original profile. But note that this might also have side-effects if multiple tests require the same profile and in worst cases steps as done by a previous test.

With geckodriver we actually make use of option 2 and never modify the original profile.

Actually creating a temporary copy of the specified user profile causes some of the user-data-dir tests to fail because the expected path isn't the same anymore.

@jschfflr as it looks like this might be a larger refactoring because the newly created temp directory will not be accessible by the consumer, and the Browser class doesn't have a property.

What do you suggest? Shall we go with a backup of pref.js for now and delete both .js files after Puppeteer finished? It would at least copy the behavior as what is used for Chrome by using the real user data dir.

Ah damn, that's too bad. Let's go with your proposed solution then!

I can do. What would be a good place to actually reset the prefs.js and delete user.js? Is https://github.com/puppeteer/puppeteer/blob/main/src/node/BrowserRunner.ts#L100 the right place so that it's always executed?

Yes, that's probably the best place for it!

jschfflr · 2021-10-29T12:30:45Z

The user profiles seem to be a tricky thing. I'm wondering what kind of usecases we want to support and how they can be implemented. From the top of my head, I guess the following situations should be considered:

A Puppeteer user wants to store state - normal firefox will not be running with this user profile
A Puppeteer user wants to pick up state that has been created through a previous session with normal firefox
A Puppeteer user wants a normal firefox instance to pick up the state from a previous puppeteer run
There is a back and forth between normal FF and Puppeteer using the same user profile
What happens if normal FF and Puppeteer try to use the same user profile at the same time?

Let me know if there are other situations to take into account!

whimboo · 2021-11-02T12:32:25Z

2. A Puppeteer user wants to pick up state that has been created through a previous session with normal firefox

This might be the most important case IMHO given that the profile can be pre-configured with imported certificates, accepted cookies and others.

3. A Puppeteer user wants a normal firefox instance to pick up the state from a previous puppeteer run

I'm not sure about the benefits here and what they actually could make use of. I would consider this a rare case.

5. What happens if normal FF and Puppeteer try to use the same user profile at the same time?

It's not possible. The profile is locked once a Firefox instance is using it.

Let me know if there are other situations to take into account!

You got a pretty good listing. So I cannot think of something else right now.

whimboo · 2021-11-03T07:48:04Z

Today (I was out the last two weeks) I noticed that this broken behavior is also causing a nearly permanent failure (#7615) in our own CI. As such I would like to get it fixed sooner than later. @jschfflr if we can find a proper solution in the next days I would appreciate. Thanks.

jschfflr · 2021-11-03T08:22:27Z

Happy to get this through :)
What do you think about going with option 2

Create a copy of the user's profile in the temp folder and let Puppeteer remove it as usual.

to get us unblocked and then iterating on that to find a better solution going forward?

whimboo · 2021-11-03T11:35:26Z

Thank you for the quick reply. Yes, that sounds fine. I'll go ahead with option 2 and we can observe incoming issues in case that this new behavior causes trouble to users.

whimboo · 2021-11-08T11:33:41Z

@jschfflr I'm struggling a bit with the prefix of the commits. Is chore the right one to use? If not what shall I use for the commits? As I saw recently the changes for Firefox that I did not that long ago were not automatically mentioned in the changelog. Is there a manual process to request additions?

OrKoN · 2021-11-08T13:26:33Z

Here is some guidance on the commits https://github.com/puppeteer/puppeteer/blob/main/CONTRIBUTING.md#commit-messages I think all commits in a PR get squashed though and only the final commit message matters. I am not sure if there is a manual process for generating the changelog now. cc @mathiasbynens

whimboo · 2021-11-08T17:18:47Z

Here is some guidance on the commits https://github.com/puppeteer/puppeteer/blob/main/CONTRIBUTING.md#commit-messages I think all commits in a PR get squashed though and only the final commit message matters. I am not sure if there is a manual process for generating the changelog now. cc @mathiasbynens

Thanks for pointing that out! I rebased my commits and updated the commit message prefixes appropriately. I would still like to see the commits pushed individually if possible.

The last three CI checks were all successful, so if that's the case again the PR would be ready for another review @jschfflr. Thanks in advance!

OrKoN · 2021-11-10T07:58:36Z

I am not sure Jan would be able to review this any time soon. So let me take another look.

I am not sure it's possible to maintain individual commits as the squash merge is enforced for this repo.

OrKoN

Still LGTM

whimboo · 2021-11-10T09:29:45Z

Thanks for the review @OrKoN!

I didn't know that squash is enforced. So I will squash them all together and give it a proper commit message. I assume after this update you can land it, or do we have to wait for someone else for doing that?

OrKoN · 2021-11-10T09:46:53Z

@whimboo yes, I can land it then

When using a custom Firefox profile for Puppeteer the modified preferences as present in prefs.js need to be reset once the profile is no longer needed by Puppeteer. If not done this could cause side-effects when the profile is used next time outside of Puppeteer. As ride-along fix the "--foreground" argument for Firefox will only be used on MacOS because that's the only supported platform.

whimboo · 2021-11-10T12:30:10Z

@OrKoN the commits have all been squashed and I force-pushed them. Also all the tests are still passing. So it looks like it's ready for landing.

OrKoN · 2021-11-10T12:31:27Z

Merged!

whimboo · 2021-11-10T12:32:19Z

Thanks a lot!

google-cla Bot added the cla: yes label Oct 13, 2021

whimboo force-pushed the firefox_preferences branch 2 times, most recently from 16ba01e to 04bec14 Compare October 13, 2021 12:00

whimboo requested a review from jschfflr October 13, 2021 12:24

whimboo requested a review from OrKoN October 13, 2021 12:25

OrKoN approved these changes Oct 13, 2021

View reviewed changes

whimboo commented Oct 14, 2021

View reviewed changes

whimboo force-pushed the firefox_preferences branch from 04bec14 to 10c2415 Compare November 8, 2021 11:27

whimboo force-pushed the firefox_preferences branch 4 times, most recently from 8b22617 to 80eb9ec Compare November 8, 2021 13:17

whimboo force-pushed the firefox_preferences branch 2 times, most recently from 497a873 to a83bf6d Compare November 8, 2021 17:16

OrKoN approved these changes Nov 10, 2021

View reviewed changes

whimboo force-pushed the firefox_preferences branch from 87c2f03 to 510f897 Compare November 10, 2021 12:03

whimboo added the firefox-client label Nov 10, 2021

OrKoN merged commit 790c7a0 into puppeteer:main Nov 10, 2021

whimboo deleted the firefox_preferences branch November 10, 2021 12:32

whimboo mentioned this pull request Nov 10, 2021

'userDataDir option should restore state' test is flakily timing out for Firefox #7615

Closed

whimboo added firefox and removed firefox-client labels Nov 10, 2021

whimboo mentioned this pull request Nov 18, 2021

Using userDataDir overwrites extraPrefsFirefox #6119

Closed

YusukeIwaki mentioned this pull request Dec 10, 2021

Correctly prepare Firefox profiles with required preferences YusukeIwaki/puppeteer-ruby#180

Merged

OrKoN mentioned this pull request Feb 23, 2022

[Bug]: Undocumented breaking behavior for userDataDir between puppeteer v11.x and >= 12.x #8059

Closed

This was referenced May 30, 2022

chore(main): release 1.0.0 #8407

Closed

chore(main): release 1.0.0 #8411

Closed

This was referenced May 30, 2022

chore(main): release 1.0.0 #8414

Closed

chore(main): release 1.0.0 #8418

Closed

chore(main): release puppeteer 14.2.0 #8420

Closed

This was referenced May 30, 2022

chore(main): release 1.0.0 #8425

Closed

chore(main): release 1.0.0 #8427

Closed

This was referenced Jun 13, 2022

chore(main): release 15.0.0 #8513

Closed

chore(main): release 15.0.0 #8514

Closed

chore(main): release 15.0.0 #8516

Closed

Uh oh!

Conversation

whimboo commented Oct 13, 2021

Uh oh!

whimboo commented Oct 13, 2021

Uh oh!

whimboo commented Oct 13, 2021

Uh oh!

OrKoN left a comment

Choose a reason for hiding this comment

Uh oh!

whimboo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

whimboo Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

jschfflr Oct 29, 2021

Choose a reason for hiding this comment

Uh oh!

whimboo Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

whimboo Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

jschfflr Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

whimboo Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

jschfflr Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

jschfflr commented Oct 29, 2021

Uh oh!

whimboo commented Nov 2, 2021

Uh oh!

whimboo commented Nov 3, 2021

Uh oh!

jschfflr commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

whimboo commented Nov 3, 2021

Uh oh!

whimboo commented Nov 8, 2021

Uh oh!

OrKoN commented Nov 8, 2021

Uh oh!

whimboo commented Nov 8, 2021

Uh oh!

OrKoN commented Nov 10, 2021

Uh oh!

OrKoN left a comment

Choose a reason for hiding this comment

Uh oh!

whimboo commented Nov 10, 2021

Uh oh!

OrKoN commented Nov 10, 2021

Uh oh!

whimboo commented Nov 10, 2021

Uh oh!

OrKoN commented Nov 10, 2021

Uh oh!

whimboo commented Nov 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jschfflr commented Nov 3, 2021 •

edited

Loading