Andreas Tolfsen ❲:ato❳ Reporter
	Description • 8 years ago

The Get command is one of the few remaining not using the new dispatching technique:

    https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver.js#1192

Specifically we want to avoid having to expose curId on the chrome-to-listener proxy since it is meant to be stateless.

Immediately it would look like Marionette:pollForReadyState needs to be integrated with the proxy so that it can handle remoteness changes implicitly, so that the Get command’s handling of this is no longer a special case.

Marionette:pollForReadyState is what returns the Marionette:ok (soon to be Marionette:asyncReply) message that ends up resolving the this.listener.get(…) promise in AsyncContentSender#send.

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

I just noticed that get() also fails for our slow loading example page as served via wptserve. Debugging showed me that we correctly see the page loads events, and also that we send an ok(), but we still hang in driver.js:get() until the global timeout.

Could this change help here? If yes, I think we should do it sooner than later, given that we want to kill all the hangs.

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

We might not need the get function in listener anymore if bug 1333458 gets finally implemented as we currently think of.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 3 • 7 years ago

I agree.  Resolving this as WONTFIX.

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

It's not that clear if my idea of bug 1333458 will work. So lets keep this open given that it would still need a fix if the page load algorithm has to stay in the content process.

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

So this is not only about the `get` command but also for any other which causes a navigation like `back`, `forward`, `refresh`, and `click`. The problem here is that the new technique doesn't seem to handle queued commands which are necessary because navigation can cause a reload of the listener at every time. 

Andreas, I still haven't digged into the new method that deep yet to understand everything. Maybe you can make a call if we really still need this bug?

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

(In reply to Henrik Skupin (:whimboo) [away 07/21 - 07/31] from
comment #5)

> So this is not only about the `get` command but also for any other
> which causes a navigation like `back`, `forward`, `refresh`, and
> `click`. The problem here is that the new technique doesn't seem
> to handle queued commands which are necessary because navigation
> can cause a reload of the listener at every time.

If we could handle all loading from chrome space, dealing directly
with the web progress listener on <xul:browser>, or if listener.js
would send us updates on the relevant events in the web progress
listener, one would presumably not need a queue of IPC messages at
all.

> Andreas, I still haven't digged into the new method that deep yet
> to understand everything. Maybe you can make a call if we really
> still need this bug?

I still think we need to get solve the problem that IPC messages
sent to content space can randomly disappear.  Re-issuing messages
when they aren’t processed, which can also be dangerous as there
is no immediate ack, is not a particularly clean solution.

But it’s maybe worth filing a more specific bug about that than to
say it’s about making these commands “use the new dispatching
technique”.