1747041 - Frequent error when running puppeteer tests: can't access property "sendAsyncMessage", this.mm is null

Status: RESOLVED FIXED

(Remote Protocol :: CDP, defect, P3)

Description

[Julian Descottes [:jdescottes]]

When running puppeteer tests logs often show the following error.

 0:39.57 pid:22497 1640083231954	RemoteAgent	ERROR	unable to stop listener: TypeError: can't access property "sendAsyncMessage", this.mm is null(chrome://remote/content/cdp/sessions/TabSession.jsm:53:5) JS Stack trace: [e-mail]:53:5
 0:39.57 pid:22497 [e-mail]:193:15
 0:39.57 pid:22497 [e-mail]:63:18
 0:39.57 pid:22497 [e-mail]:50:20
 0:39.57 pid:22497 [e-mail]:51:12
 0:39.57 pid:22497 [e-mail]:91:12
 0:39.57 pid:22497 [e-mail]:102:12
 0:39.57 pid:22497 [e-mail]:137:34
 0:39.58 pid:22497 JavaScript error: chrome://remote/content/cdp/domains/parent/Emulation.jsm, line 154: TypeError: can't access property "customUserAgent", browsingContext is null
 0:39.58 pid:22497 JavaScript error: chrome://remote/content/cdp/sessions/TabSession.jsm, line 78: TypeError: can't access property "sendAsyncMessage", this.mm is null
 0:39.83 pid:22497 

This doesn't make the tests fail and seems to happen when closing the browser at the end of a test file.

Can be reproduced locally. To avoid running the whole suite, modify a test to use it.only eg

diff --git a/remote/test/puppeteer/test/click.spec.ts b/remote/test/puppeteer/test/click.spec.ts
--- a/remote/test/puppeteer/test/click.spec.ts
+++ b/remote/test/puppeteer/test/click.spec.ts
@@ -21,17 +21,17 @@ import {
   setupTestBrowserHooks,
   itFailsFirefox,
 } from './mocha-utils'; // eslint-disable-line import/extensions
 import utils from './utils.js';
 
 describe('Page.click', function () {
   setupTestBrowserHooks();
   setupTestPageAndContextHooks();
-  it('should click the button', async () => {
+  it.only('should click the button', async () => {
     const { page, server } = getTestState();
 
     await page.goto(server.PREFIX + '/input/button.html');
     await page.click('button');
     expect(await page.evaluate(() => globalThis.result)).toBe('Clicked');
   });
   it('should click svg', async () => {
     const { page } = getTestState();

and run ./mach puppeteer-test --subset

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

What we are trying to do here is to send remote:destroy via the message manager which doesn't exist anymore because it has been destroyed. I assume we should probably check for an existent message manager before, or maybe even put the whole block into a try/catch?

Comment 2

[Julian Descottes [:jdescottes]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #1)

What we are trying to do here is to send remote:destroy via the message manager which doesn't exist anymore because it has been destroyed. I assume we should probably check for an existent message manager before, or maybe even put the whole block into a try/catch?

We could also listen for "message-manager-close" but that would make things a lot more complex for very little added value. I think checking for this.mm here is a good idea.

Comment 3

[Julian Descottes [:jdescottes]]

For info, in a typical puppeteer test, the TargetList will have several TabTargets and a MainProcessTarget. The TabTargets will be closed when the corresponding tab closes, but the main process target is only closed when we stop CDP. The main process target which holds the connection and will trigger the destruction of the TabSession. But the TabTarget of the TabSession was already destroyed, and the actual tab was already closed.

Maybe the TabSession should be destroyed early when its TabTarget is closed, but for now, let's just make sure we don't pollute the logs.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1747041 - [cdp] Check if message manager exists in TabSession destructor

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Julian Descottes [:jdescottes] from comment #3)

Maybe the TabSession should be destroyed early when its TabTarget is closed, but for now, let's just make sure we don't pollute the logs.

Great investigation. If we don't do that it will be indeed the underlying problem. Can you please file a new bug? I wonder how much more complicated it will be to get this fixed instead of this workaround.

Comment 6

[Julian Descottes [:jdescottes]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #5)

(In reply to Julian Descottes [:jdescottes] from comment #3)

Maybe the TabSession should be destroyed early when its TabTarget is closed, but for now, let's just make sure we don't pollute the logs.

Great investigation. If we don't do that it will be indeed the underlying problem. Can you please file a new bug? I wonder how much more complicated it will be to get this fixed instead of this workaround.

I'm not sure about the potential impacts of such a change, so I'd definitely keep this for a follow up :)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/974fddf0c2f6
[cdp] Check if message manager exists in TabSession destructor r=webdriver-reviewers,whimboo

Comment 8

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/974fddf0c2f6