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
Check Firefox compatibility for Windows #7668
Conversation
Forcing Fission disabled doesn't actually fix the test. So some other change that I might have done recently in Firefox might have caused it. I'll check with custom Firefox builds where it might have been introduced. |
127a948
to
8588ede
Compare
The problem that we currently have with Windows is that Firefox uses a launcher process that initially gets started, and then itself launches the real Firefox process. As such Puppeteer only knows about the launcher process, and that is the process that gets killed by a couple of tests. By doing that the real Firefox process remains running and is blocking the temporary profile from being removed. And these are the failures that happen most right now. As such I would suggest to find a solution for that problem first, which means that we would need to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1656962 on our side. Requiring client to find the real process I don't see as a possible solution. CC @OrKoN. |
@whimboo thanks for the info, can we disable that test for FireFox on Windows for now? |
It might not be only a single test. Basically each test that causes But hey, I just saw some code which looks kinda weird! Why are we trying to remove the temporary user data directory BEFORE we kill the browser process? Is that somehow on purpose? From our point of view this will clearly cause issues when the process still has hold on some of the files of the profile. Can we flip that around? I think that this might already help in some cases. |
I created #7761 to check that. |
@OrKoN we also kill the browser here: puppeteer/src/node/Launcher.ts Lines 374 to 396 in 790c7a0
So what I see is that when |
8588ede
to
1ce1de6
Compare
We landed a fix in Firefox which connects the launcher process as used by default on Windows with all the other Firefox processes within the same job. At the same time we also set Nevertheless there are remaining issues, and maybe I can continue next week. |
Actually the test Launcher.spec should filter out ignored default arguments causes troubles. In case of Firefox it removes the
While this might work for Chrome it's not appropriate for Firefox. Also depending on the platform and some other options different default arguments are getting set for Firefox, so the above problem will not only happen on Windows. @OrKoN any suggestions? IMHO we could update the test so that it only removes the very last argument, which seems to always be the |
@whimboo sorry, I am not really familiar with this part. It looks like |
Ok, so IMHO there should be a distinction between default arguments and mandatory arguments. As such it might be that some of the currently used default arguments for Firefox should actually be mandatory: puppeteer/src/node/Launcher.ts Line 447 in 4c3caaa
First the Beside that the test also removes the 3rd default argument which in case of Windows and a headless CI job (which is done by default) will be As such I would propose the following:
|
@whimboo is there a case when a user might want to remove |
FYI a patch for that actually landed in #8233 and #8250. Now there are two distinct tests for both Chrome and Firefox. I'll rebase and trigger new checks in a moment. |
1ce1de6
to
94c5406
Compare
Note that right now there seems to be only a single remaining failure on Windows which would prevent us from re-enabling results in CI. The failing test is:
|
94c5406
to
0241c7b
Compare
@OrKoN the error trace as visible in the CI logs is far from being helpful. Do you see what could be wrong here? I'm not able to see what is actually failing here, and I cannot reproduce it locally on my local Windows machine. |
@whimboo I am not sure what exactly fails but if I read the code right: a CDPSession receives a message that has an ID (thus, a callback is expected as the message is a response to a message from Puppeteer); yet the callback is missing so it looks like an unsolicited response from the browser. I suggest adding |
Ah sorry, you should also probably include a |
As it looks like the job on Windows doesn't finish at all. It's running 5:30h now and I think that I should cancel. Not sure how to continue investigation given that I cannot reproduce locally. https://github.com/puppeteer/puppeteer/runs/6171558887?check_suite_focus=true |
I think it actually might be the problem with GitHub actions. Let's try to restart it and see if it times out again. I can also try on my Windows locally. |
@OrKoN it doesn't look like that this has made any difference. As such if you could have a quick look on your local machine I would appreciate! Thanks. |
Unfortunately, I am unable to repro on my Win too :( |
Let me run the full test suite: perhaps there is some state leaking from previous tests. |
Hm, this is sad to hear that you cannot reproduce the failure locally and that the CI checks do not have the failures that you are seeing locally. :( As such I think that due to time constraints I'll have to post-pone any work on this PR for some time. |
I believe we can close this one. |
@OrKoN, are tests on Windows run again for Firefox? I cannot remember if / that we enabled those. |
@whimboo no, we have not enabled them |
As it looks like there might be some incompatibilities with our CDP implementation when Fission is enabled on Windows. This was mentioned in #7659.
I'm going to force disable Fission for now. Maybe this helps to make the failure go away.