Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Description • 7 years ago

Given that our Marionette server code is using a lot of Promise related code, we should consider that each condition like for event handlers could cause an infinite hang if the event is never called. As result we hang forever and finally the client bails out with an IOError and force kills Firefox. 

We might get around this situation by defining a timer inside those critical promises. Which means by default we assume the expected event will happen and resolves the promise. But if that never happens the timeout timer could actually reject the promise.

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 1 • 7 years ago

When I had a look over this code I wonder how we could pass in a sane timeout value for the promises which are handled by the proxy. I don't think that we can set it via a parameter for specific commands, but might have to define the maximum threshold. In this case it has to be larger than the page load timeout and smaller than the socket timeout:

300s < timeout < 360s

I would suggest to use 330s which is perfectly in the middle and balances out times for slow running builds.

Andreas, what do you think? Maybe I miss a part which would allow us to specify specific timeout values?

	Andreas Tolfsen ❲:ato❳
	Comment 2 • 7 years ago

Timed promises should have a default timeout duration, but makes sense to allow this to be overridden through an argument.  The timeout when navigating should indeed be greater than the page load timeout, and conversely the timeout when executing a script should be greater than the set script timeout.  This indicates that giving the timed promises configurable timeout values is necessary.

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 3 • 7 years ago

Another situation which I noticed via bug 1353599 is that we hang for a remoteness change, and when the listener cannot send back its response to the driver. So a timeout and a check for remoteness changes might be good.

	Andreas Tolfsen ❲:ato❳
	Comment 4 • 7 years ago

If, somehow, all promises got registered centrally in chrome space, we could cancel/reject them whenever a listener runs into problems, e.g. related to its inability to send a message to a handler that no longer exists.

We had a central register of promises, we could also have a timeout sweeper that periodically cancelled promises that had been inactive for a while, but I’d wait with implementing such a solution until it becomes a significant problem.  There is always a danger of cancelling something that is meant to run for infinity.  For example, scripts with script timeout null are meant to continue to run forever.

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 5 • 7 years ago

Andreas, I assume we can close this bug given your patch for `TimedPromise` in `wait.js`?

	Andreas Tolfsen ❲:ato❳
	Comment 6 • 7 years ago

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

> Andreas, I assume we can close this bug given your patch for
> `TimedPromise` in `wait.js`?

I _think_ that should fulfill the requirements, but I should add
it’s not the sturdiest implementation in the world.

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 7 • 7 years ago

I don't mind the quality. :) Given that such a promise class exists now is fine with me. Enhancements if necessary can still be made later.

So marking as fixed by bug 1381876.