Re-enable launching Firefox on Windows CI test

[jackfranklin]

@mjzffr in #5637 we had to disable this test on the Windows CI run because it would timeout.

      it('should be able to launch Firefox', async() => {
        const {puppeteer} = getTestState();
        const browser = await puppeteer.launch({product: 'firefox'});
        const userAgent = await browser.userAgent();
        await browser.close();
        expect(userAgent).toContain('Firefox');
      });

I'm not quite sure why but it'd be great to have a look to see if we can enable this test on Windows again. Would you mind having a look when you get a chance? Thank you :)

[mathiasbynens]

cc @whimboo

[hlawuleka]

Either this or Could not find browser revision latest. Run "npm install" or "yarn install" to download a browser binary.

Yet there's a line in the docs that suggests that all you need to do is set product: 'firefox'.

[mjzffr]

I'll def take a look when I have the chance.

[whimboo]

So didn't it find a suitable download of Firefox Nightly? Or which line of the test actually times out?

[mjzffr]

From a user perspective, Browser.close is broken for Firefox on Windows.

I believe BrowserRunner needs a special case for Firefox on Windows to record the actual browser process to kill, which is a child of the initial process we run.

For future reference, here are debug logs.

2020-08-06T19:50:52.299Z puppeteer:launcher Calling C:\Users\mf\dev\puppeteer\.local-firefox\win64-81.0a1\firefox\firefox.exe --no-remote --foreground --wait-for-browser --headless about:blank --remote-debugging-port=0 --profile C:\Users\mf\AppData\Local\Temp\puppeteer_dev_firefox_profile-Ij7Ruz
*** You are running in headless mode.
DevTools listening on ws://localhost:49962/devtools/browser/f6f6da26-da96-4145-98af-21aab53ef000
2020-08-06T19:51:03.660Z puppeteer:protocol:SEND {"method":"Target.setDiscoverTargets","params":{"discover":true},"id":1}
1596743463673   RemoteAgent     TRACE   (connection {dafcaf62-af68-434b-bab1-fcc99fd65dc0})-> {"method":"Target.setDiscoverTargets","params":{"discover":true},"id":1}
1596743463674   RemoteAgent     TRACE   <-(connection {dafcaf62-af68-434b-bab1-fcc99fd65dc0}) {"method":"Target.targetCreated","params":{"targetInfo":{"browserContextId":null,"targetId":"a901a635-8ed0-42d4-a46c-fbdc891ca6ab","type":"page","url":"about:blank"}}}
1596743463674   RemoteAgent     TRACE   <-(connection {dafcaf62-af68-434b-bab1-fcc99fd65dc0}) {"method":"Target.targetCreated","params":{"targetInfo":{"targetId":"f6f6da26-da96-4145-98af-21aab53ef000","type":"browser"}}}
2020-08-06T19:51:03.662Z puppeteer:protocol:RECV ΓùÇ {"method":"Target.targetCreated","params":{"targetInfo":{"browserContextId":null,"targetId":"a901a635-8ed0-42d4-a46c-fbdc891ca6ab","type":"page","url":"about:blank"}}}
2020-08-06T19:51:03.662Z puppeteer:protocol:RECV ΓùÇ {"method":"Target.targetCreated","params":{"targetInfo":{"targetId":"f6f6da26-da96-4145-98af-21aab53ef000","type":"browser"}}}
1596743463674   RemoteAgent     TRACE   <-(connection {dafcaf62-af68-434b-bab1-fcc99fd65dc0}) {"id":1}
2020-08-06T19:51:03.662Z puppeteer:protocol:RECV ΓùÇ {"id":1}
2020-08-06T19:51:03.662Z puppeteer:protocol:SEND {"method":"Browser.getVersion","id":2}
1596743463675   RemoteAgent     TRACE   (connection {dafcaf62-af68-434b-bab1-fcc99fd65dc0})-> {"method":"Browser.getVersion","id":2}
1596743463676   RemoteAgent     TRACE   <-(connection {dafcaf62-af68-434b-bab1-fcc99fd65dc0}) {"id":2,"result":{"protocolVersion":"1.3","product":"Headless Firefox","revision":"1","userAgent":"Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:81.0) Gecko/20100101 Firefox/81.0","jsVersion":"1.8.5"}}
2020-08-06T19:51:03.663Z puppeteer:protocol:RECV ΓùÇ {"id":2,"result":{"protocolVersion":"1.3","product":"Headless Firefox","revision":"1","userAgent":"Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:81.0) Gecko/20100101 Firefox/81.0","jsVersion":"1.8.5"}}
[Error: EBUSY: resource busy or locked, unlink 'C:\Users\mf\AppData\Local\Temp\puppeteer_dev_firefox_profile-Ij7Ruz\cert9.db'] {
  errno: -4082,
  code: 'EBUSY',
  syscall: 'unlink',
  path: 'C:\\Users\\mf\\AppData\\Local\\Temp\\puppeteer_dev_firefox_profile-Ij7Ruz\\cert9.db'
}

[mathiasbynens]

The hardcoded date has come and gone, and so this is causing CI failures again: https://travis-ci.com/github/puppeteer/puppeteer/jobs/382167500

@mjzffr Any progress on this?

In the meantime, I'll restore CI by extending the deadline by a month.

[mjzffr]

I'm assigned to other work atm but this is on our list of priorities.

[whimboo]

@mathiasbynens any chance that you could trigger such a unit test job with a debug version of Firefox and fetch all the stdout logging? It would help a lot in figuring out what's going on here. I don't see why it can take more than 30s to launch Firefox. I assume that maybe the devtools websocket port is in use, and as such no connection can be established?

[mathiasbynens]

@whimboo I'd happily review a PR that changes the Travis config in the way you describe if it helps y’all investigate 👍

[whimboo]

Sorry, I was wrong. I missed the part that this is related to killing the browser process. So in this case any additional logging will not help. Can we please adjust the summary so that it clarifies it? Thanks.

[saschanaz]

Friendly ping that it's near October and the CI will fail again 👀

[whimboo]

For reference Maja filed the following bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1656962

Lets see what's actually necessary.

[whimboo]

@jackfranklin and @jschfflr is there a reason why Puppeteer doesn't always try to gracefully shutdown the browser first?

puppeteer/src/node/BrowserRunner.ts

Lines 131 to 139 in b046fd7

	if (this._tempDirectory) {
	this.kill();
	} else if (this.connection) {
	// Attempt to close the browser gracefully
	this.connection.send('Browser.close').catch((error) => {
	debugError(error);
	this.kill();
	});
	}

Why is that made dependent on the temporary folder? Killing could always happen when the browser doesn't shutdown itself within a given time period. When I remove this check the tests are passing all well.

[whimboo]

Actually this code was added by @mjzffr when getting the Launcher updated to handle Firefox: c5a72e9

I don't say that there is no bug in correctly handling the call to kill(), but it would help a lot in making sure we always try to gracefully shutdown the browser first. Eg this blocks me from creating Profiler data because it only gets written out when a graceful shutdown is triggered.

[whimboo]

Also in BrowserRunner.kill() the temporary profile directly should be removed AFTER the process has been killed. Especially on Windows files will remain because they are still in use.

[jschfflr]

The original idea was to kill the browser when we don't have a user supplied data dir but do a graceful shutdown if we do in order to not corrupt files (see #693) - not sure where the decision to just kill came from and would assume that it happened for performance reasons. @mathiasbynens Do you think it would be ok to always go via CDPs Browser.close first and just have the kill code path if the the shutdown takes longer than expected?

[whimboo]

Maybe it might be good to have some time measurements for both paths. I don't know how long Chrome might need for graceful shutdowns in comparison to just killing the process.

Also as mentioned to @jschfflr on Elements I'm aware of some longer shutdown scenarios with Firefox, but especially for our case I would accept those to give users a chance to test with Firefox at all on Windows. There are lots of open issues all around this broken shutdown behavior.

[jschfflr]

After thinking a while about it, I think it's best to keep the current behaviour for Chrome and just change it for Firefox.
Would you mind creating a patch for that?

[whimboo]

@jschfflr I surely could do. But there is one open question. The BrowserRunner class doesn't know about the product itself. It only has the executable path, process args, and the profile dir. As such I cannot simply add an if condition for Firefox specifically there.

So one solution could be that the Launcher class passes this.product into BrowserRunner, which could then be used for a conditional opt-in for a clean shutdown. Would that be fine, or do you have another suggestion?

[whimboo]

I just pushed this proposal to #6895. So far it looks good.

[jschfflr]

Your patch looks good! I just added a small nit there about the types.

[whimboo]

Tests on Windows run again in Puppeteer CI since my PR on #6895 got merged.