1412236 - Prevent additional registrations of framescripts for multiple client connections

Status: RESOLVED WONTFIX

(Remote Protocol :: Marionette, enhancement, P3)

Description

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

When I start multiple client connections the following can be seen:

1509093102043	Marionette	DEBUG	Accepted connection 5 from 127.0.0.1:64938
1509093102047	Marionette	TRACE	5 -> [0,1,"newSession",{}]
1509093102055	Marionette	DEBUG	Register listener.js for window 2147483649
1509093102112	Marionette	TRACE	5 <- [1,1,null,{"sessionId":"57d03275-57bd-8843-9396-447bbe8d5125","capabilities":{}}]
1509093102117	Marionette	DEBUG	Accepted connection 6 from 127.0.0.1:64939
1509093102122	Marionette	DEBUG	Closed connection 6
1509093102123	Marionette	DEBUG	Accepted connection 7 from 127.0.0.1:64940
1509093102131	Marionette	TRACE	7 -> [0,1,"newSession",{}]
1509093102138	Marionette	DEBUG	Register listener.js for window 2147483649
1509093102155	Marionette	TRACE	7 <- [1,1,null,{"sessionId":"a057649a-b2ff-c646-a080-2ee4bf16cc87","capabilities":{}}]
1509093102165	Marionette	DEBUG	Accepted connection 8 from 127.0.0.1:64941
1509093102169	Marionette	DEBUG	Closed connection 8
1509093102172	Marionette	DEBUG	Accepted connection 9 from 127.0.0.1:64942
1509093102177	Marionette	TRACE	9 -> [0,1,"newSession",{}]
1509093102186	Marionette	DEBUG	Register listener.js for window 2147483649
1509093102202	Marionette	TRACE	9 <- [1,1,null,{"sessionId":"82857084-234e-8c4b-8d99-2dd6aa3f8034","capabilities":{}}]

As you can see we register new framescripts for each open window/tab, whenever a new client gets connected. Right now I don't know about the side-effects of that, but given my feeling it could interfere with currently executed commands in the framescript, eg. any kind of navigation.

Andreas, I think we should really turn off the feature to allow multiple client connections. I don't see any benefit at all. Anything you have in mind?

Comment 1

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #0)

> As you can see we register new framescripts for each open
> window/tab, whenever a new client gets connected. Right now I
> don't know about the side-effects of that, but given my feeling
> it could interfere with currently executed commands in the
> framescript, eg. any kind of navigation.

I think it is a valid concern that not every part of Marionette is
written so that multiple connections are safe.  But is it a real
problem that consumers are making more than one connection today?

> I think we should really turn off the feature to allow multiple
> client connections. I don't see any benefit at all. Anything you
> have in mind?

In addition to the emulate-user-interactions part of Marionette
that can cause problems, disallowing multiple connections for the
remainder of browser instrumentation features doesn’t seem ideal.

We have to bear in mind that the Marionette protocol will likely be
replaced by CDP at some point.  CDP allows multiple connections,
so we need to decide whether to disallow multiple connections from
initialising the WebDriver service or if we want to allow it at the
user’s own risk.

I would argue that the right thing to do here is to allow multiple
connections, but to prevent multiple connections from initialising
the WebDriver service.  We need to address https://bugzil.la/1330309
before that can become a reality.

Comment 2

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

(In reply to Andreas Tolfsen ‹:ato› from comment #1)
> > As you can see we register new framescripts for each open
> > window/tab, whenever a new client gets connected. Right now I
> > don't know about the side-effects of that, but given my feeling
> > it could interfere with currently executed commands in the
> > framescript, eg. any kind of navigation.
> 
> I think it is a valid concern that not every part of Marionette is
> written so that multiple connections are safe.  But is it a real
> problem that consumers are making more than one connection today?

I cannot say for sure, because I haven't tested it. But just the multiple creation of the framescripts made me curious. It may be a red herring through.

> We have to bear in mind that the Marionette protocol will likely be
> replaced by CDP at some point.  CDP allows multiple connections,
> so we need to decide whether to disallow multiple connections from
> initialising the WebDriver service or if we want to allow it at the
> user’s own risk.

That would make sense, eg. for retrieving data in parallel while another commands are running.

> I would argue that the right thing to do here is to allow multiple
> connections, but to prevent multiple connections from initialising
> the WebDriver service.  We need to address https://bugzil.la/1330309
> before that can become a reality.

Sure, I just marked as dependency and updated the bug's summary.

Comment 3

[David Burns :automatedtester]

[Mass Change 2018-01-15] Moving bugs to backlog

Comment 4

[Andreas Tolfsen ❲:ato❳]

More information: https://developers.google.com/web/updates/2017/10/devtools-release-notes#multi-client

Comment 5

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

The answer to this problem can actually be found at:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Multiprocess_Firefox/Message_Manager/Frame_script_loading_and_lifetime

If you use allowDelayedLoad, you can cancel it by using removeDelayedFrameScript.
[..]
This means we will stop loading the script into new tabs. Note that this function will not remove any scripts which have been loaded earlier.

Another problem is:

There is currently no way to unload them when loaded, other than closing the tab they were loaded into

Given these restrictions we would have to replace the initial tab to finally get rid of the already loaded framescripts. But that sounds like a no-go.

So best is really to switch over to the JSWindowActor implementation as soon as possible. Until that happened we have to make sure that every registered listener including observers and event listeners are correctly removed.

Given that I'm going to close this bug as WONTFIX.