Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:71.0) Gecko/20100101 Firefox/71.0 ID:20190926094200

When I'm trying to run the Puppeteer tests with Firefox something fails during the setup so that not a single test gets run. What I'm trying to execute is:

mach test remote/test/puppeteer/test/page.spec.js

The failure as thrown after a 10s timeout failure is:

## TERMINATED ##
Message:
  puppeteer.spec.js:107:7 - FAILED while running "beforeEach" in suite "Chromium Browser Page"
Stack:
  Error: Protocol error (Network.enable): Component returned failure code: 0xc1f30100 (NS_ERROR_FACTORY_EXISTS) [nsIComponentRegistrar.registerFactory] NetworkObserver@chrome://remote/content/domains/parent/network/NetworkObserver.jsm:74:15
  enable@chrome://remote/content/domains/parent/Network.jsm:61:29
  execute@chrome://remote/content/domains/Domains.jsm:99:25
  execute@chrome://remote/content/sessions/Session.jsm:54:25
  execute@chrome://remote/content/sessions/TabSession.jsm:67:20
  onPacket@chrome://remote/content/Connection.jsm:210:36
  onMessage@chrome://remote/content/server/WebSocketTransport.jsm:85:18
  handleEvent@chrome://remote/content/server/WebSocketTransport.jsm:67:14
  EventListener.handleEvent*ready@chrome://remote/content/server/WebSocketTransport.jsm:29:17
  Connection@chrome://remote/content/Connection.jsm:33:20
  handle@chrome://remote/content/targets/Target.jsm:56:18
  async*createHandlerFunc/<@chrome://remote/content/server/HTTPD.jsm:2109:13
  handleResponse@chrome://remote/content/server/HTTPD.jsm:2434:36
  process@chrome://remote/content/server/HTTPD.jsm:1329:26
  _handleResponse@chrome://remote/content/server/HTTPD.jsm:1776:22
  _processBody@chrome://remote/content/server/HTTPD.jsm:1621:14
  onInputStreamReady@chrome://remote/content/server/HTTPD.jsm:1500:14

This actually happens because during shutdown we are trying to register the same class again, which indeed will fail. I can get around this failure by checking if the class is already registered via registrar.isCIDRegistered(SINK_CLASS_ID), but that puts me into an endless loop, and the command never returns. But maybe it just tries to run the tests for page?

As the file history shows this code was originally landed as part of the Juggler import. Given by Andreas the Puppeteer tests don't actually work at the moment due to some setup/teardown code, which doesn't play well with Firefox yet.

The NetworkObserver class is actually used by the Network domain. It registers itself as a ChannelEventSink to track redirects. It gets created when Network.enable() is called, and disposed when Network.disable() is called. As such the code can run multiple times depending on how often the network is enabled and disabled.

While constantly Network.enabled is called, I never see a call to Network.disable() or its destructor. As such with creating the second instance of the Network domain, the failures are about to get started.

I'm not sure where exactly those instances are created, and I don't have the time to dig into the framework of the Puppeteer tests right now. As such we should ignore it for now, also because those tests aren't a high priority right now.

We had a discussion in the team and there are actual several problems here:

1. We should not register and unregister the event sink class each time the NetworkObserver class gets created and disposed. Instead that should be done on a global level and only once. This would not only prevent the XPCOM error as covered by the bug, but also makes it easier to handle in terms of multiple instances of the NetworkObserver class which is basically how it is used in devtools because it allows multiple client connections. As such we decided to simply fork the file channel-event-sink.js.

2. As it looks like Network.disable() is never called, and as such none of the registered observer notifications from the NetworkObserve class will ever be removed, which actually results in a leak of memory (bug 1587814). Even worth I can see that Network.enable() is called a lot of times when running at least the Puppeteer unit tests. That means leaked objects accumulate over time. To prevent that we should dispose/destruct all the enabled domains when the client disconnects. For the time being it might be fine to just do that by hard-coding the obvious and possible leaky domains. A better implementation should track enabled domains, but which might take more time to get it working.

Andreas, did I miss something here, or is all fine in how we want to have it for now?

If we want to support multiple client connections we need to refcount
the usage of the NetworkObserver. If you imagine a scenario where
there are multiple simultaenous connections to the remote agent
that both enable the Network domain, the NetworkObserver should
only be shut down (“disposed of”) after the last connection terminates
since we can only have exactly one instance of the NetworkObserver
at any one time in Gecko.

If we disregard the need for multiple simulatenous client connections
you may get away with disposing of the NetworkObserver when the
Network domain is disabled. We should in any case, regardless of
the above statements, ensure that all domains’ destructors are
invoked when client connections are closed.

cc’ing everyone who was involved in this discussion today.

To allow for multiple client connections the ChannelEventSinkFactory
class only has to be registered once. By reference counting instances
of the NetworkObserver, it can be made sure to unregister the class
once no consumers of the class exist anymore.

With this refactoring we basically allow multiple client connections
to our CDP implementation.

Depends on D49203

https://hg.mozilla.org/mozilla-central/rev/56f934c9a812