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

As seen at least for bug 1347456 from Treeherder but also noticed locally a couple of times, we have hangs in the driver when a remoteness change occurred and the communiction with the framescript happened via the dispatcher while the command hasn't queued in pendingCommands.

This is clearly a serious issue and can happen at various times when a command triggers a page load and Marionette doesn't wait for it to be finished. Please note that right now we only wait for that in get(), goBack(), and goForward(). But nowhere else.

I will try to setup a testcase for that which actually might not be that hard.

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

Trying to reproduce with current code is hard, but if we exclude a page load for file:// urls in navigate.js, the following snippet will promptly cause a hang all the times:

    def test(self):
        self.marionette.navigate("file:///")
        self.marionette.get_url()

The call to get_url() never returns, and Marionette runs into the socket timeout.

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

Btw. there seem to be events we might be able to use to get informed about a remoteness change. Those are BeforeTabRemotenessChange and TabRemotenessChange.

Maybe we should listen to the events and re-send the message to the listener once the remoteness change has been completed.

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

Something which also helps here is to ensure that every possible navigation request is clearly routed through the loadListener. But this is something we cannot always expect, eg. if the user presses enter on a submit button. In such a case a new page is most likely to get loaded, and the following command will fail to execute due to a possible remoteness change.

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

The underlying issue here is that at any point, although more likely
during navigation, we cannot trust the content frame script to send
back a reply to the IPC message because the content frame might have
suffered a remoteness change and the frame script will have been reloaded.

When the frame script gets reloaded it loses all context.  After it
is has finished re-registering we are forced to re-send the IPC
message so that it can finish whatever work it was doing before,
and then send back an ack to the original IPC request, resolving
the promise in testing/marionette/proxy.js.

The window tracking refactoring is not going to solve this fundamental
limitation of the IPC system.  Instead I think we need to look at
other alternatives, such as perhaps building in a message queue
system in testing/marionette/proxy.js that always re-sends IPC
messages when we notice a remoteness change occurring.  This means
we would need a reliable way of detecting remoteness changes as
well as reworking Marionette:waitForPageLoaded because it is not
exactly re-sending the same Marionette:get IPC message.

Because the window tracking patches will contain some of the
tools we need to build such a system, I will leave the dependency
in place.  I should end by saying that I’m not 100% sure this is
the right approach, but it seems plausible that we can get rid of
the this.curBrowser.pendingCommands.push(() => { … }) paradigm
by enqueing _all_ IPC messages.

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

Please note that we can NOT send the original IPC message multiple times. At least not for those types of commands which actually trigger an activity by the browser. Sending the same message again would trigger that action again, and should clearly cause unexpected behavior.

This is indeed tricky, and I'm also not sure without thinking too much about all that, what's best to do.

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

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

> Please note that we can NOT send the original IPC message multiple
> times. At least not for those types of commands which actually
> trigger an activity by the browser. Sending the same message
> again would trigger that action again, and should clearly cause
> unexpected behavior.

Indeed, that’s not exactly what I meant.  It is true that the two
messages are distinct.  In other words, certain commands have a
two-step round trip between chrome and the frame script before they
succeed.