Investigate harness crashes for "Puppeteer.launch" unit test's (launcher.spec.js)
Categories
(Remote Protocol :: Agent, task, P3)
Tracking
(Not tracked)
People
(Reporter: whimboo, Unassigned)
References
Details
All the unit tests for the launcher under Puppeteer.launch are currently disabled due to possible hangs of maybe Firefox, and a resulting crash of the test harness. Given that launching the browser is a crucial part of Puppeteer support, we should get this investigated and fixed.
|
Reporter | |
Comment 1•4 years ago
|
||
Actually I can see that when running the tests locally an instance of Firefox is NOT closed and as such remains open and most likely blocking the websocket port. As such all successive tests are going to fail.
|
|
Reporter | |
Comment 2•4 years ago
|
||
Given that those tests open and close Firefox kinda quickly I wonder if it's overlapping some startup and shutdown code. Lots of failures can be seen in the log for domains not being destructed, including the Network domain for Puppeteer.launch userDataDir option:
1579766759922 RemoteAgent ERROR unable to stop listener: [Exception... "ServiceManager::GetService returned failure code:" nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)" location: "JS frame :: chrome://remote/content/domains/parent/network/ChannelEventSink.jsm :: ChannelEventSinkFactory.getService :: line 104" data: no] Stack trace: ChannelEventSinkFactory.getService()@ChannelEventSink.jsm:104
dispose()@NetworkObserver.jsm:86
disable()@Network.jsm:82
If any destructor in the clear() method of the DomainCache fails, it will bail out immediately, instead of trying to destruct the other domains, and the rest of the teardown code. Moving the try/catch from RemoteAgent to that code would maybe make more sense:
And for ChannelEventSinkFactory.getService() we should return null if the service is not available.
I actually have the launcher unit tests fixed as part of my WIP on Bug 1596886. The tests were launching Firefox in a way that didn't make sense.
I think what you're referring to in Comment 2 is due to the browser being killed after a harness timeout. The fix you propose isn't a bad idea, but it's not a blocker for anything.
|
|
Reporter | |
Comment 4•4 years ago
|
||
That's great to hear! So we can move this out of beta.
|
|
Reporter | |
Comment 5•4 years ago
|
||
Maja, this is all good now given that your patch landed?
There are two failures that are unresolved: tests that work fine on Travis but hang (and the harness crashes) in TaskCluster
|
|
Reporter | |
Comment 7•4 years ago
|
||
Which exact tests, which still need to be fixed, are remaining here?
|
|
Reporter | |
Comment 8•4 years ago
|
||
Please also see https://github.com/puppeteer/puppeteer/issues/5466 for one reason why we have those crashes when running the Puppeteer unit tests. Maybe there is some other or similar behavior here for the launcher tests.
|
|
Reporter | |
Comment 9•4 years ago
|
||
Given that Puppeteer 3 came with a lot of harness changes, and Maja currently works on a downstream sync via bug 1632710, we should wait until that patch landed before doing any further investigation here.
|
|
Reporter | |
Comment 10•4 years ago
|
||
Maja, given that you were able to remove the skip from all the tests can I assume that we no longer have the crashes in case of missing Promise resolutions? That would be great!
Yep
Description
•