-
Notifications
You must be signed in to change notification settings - Fork 9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(webdriver): configure Firefox to start with the default user context only #12309
Conversation
if (options.disableExtraUserContexts) { | ||
const containersPath = path.join(options.path, 'containers.json'); | ||
|
||
const singleContainer = JSON.stringify({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@whimboo we want to disable extra user contexts for Firefox in Puppeteer (for profiles created by Puppeteer). This works but I wonder if this solution is something we can rely on?
P.S. @Lightning00Blade did you notice any speed up in launching Firefox?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running page.spec.ts
with hook, shows diff in the margin of error (under 1%)
after(() => {
console.log('End', performance.now());
});
But I think there is slight overhead for writing the file if I isolate a test is around 100ms, I think I can optimize it by paralleling the writing of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With parallelization it seems the overhead of writing a file to be around 30-50ms
and that varies from run to run a lot have seen the old one be slower and the new one be faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(whimboo is off this week)
There are two internal containers that should be preserved:
https://searchfox.org/mozilla-central/rev/6121b33709dd80979a6806ff59096a561e348ae8/toolkit/components/contextualidentity/ContextualIdentityService.sys.mjs#85-105
userContextIdInternal.thumbnail and userContextIdInternal.webextStorageLocal
I am not sure what are the side effects of removing the second one, but I think it's better to keep it.
Note that the current version was recently bumped to 5 (https://bugzilla.mozilla.org/show_bug.cgi?id=1814969), but it's fine to keep 4 for now, and anyway the contextualidentityservice has migration logic based on the version.
Overall I would say this is fine as a temporary solution but we should try to introduce a preference in Firefox in order to skip the default user profiles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lightning00Blade let's probably rewrite the tests to expect any number of non-default contexts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sound good, I think I would extract some of the changes from this PR as well, I think we can make FF launch just a little bit faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait for feedback from Henrik
By default Firefox start with multiple UserContexts (containers). We need to disable this by only providing single Container to the
containers.json
file and pointing to it fromlastUserContextId
.