1595795 - Synchronous domain methods return before their code is being executed

Status: RESOLVED INCOMPLETE

(Remote Protocol :: Agent, defect, P3)

Description

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

I noticed that problem when implementing Target.activateTarget() (bug 1592643) and now with Network.setUserAgentOverride() (bug 1595697). Both methods are basically defined as synchronous in the parent domains. Also they do not trigger any kind of event a client would have to wait for.

What basically happens is the following. When I put even a dump() statement into such a method, the client has already moved on before that line gets printed to the log. Here an example:

Part of remote/domains/parent/Network.jsm:

  setUserAgentOverride(options = {}) {
    const { userAgent } = options;
    dump(`Setting user agent to ${userAgent}`);
    Services.prefs.setStringPref("general.useragent.override", userAgent);
  }

Client code like in browser chrome tests:

add_task(async function setUserAgent({ Network }) {
  const userAgent = "foo bar";
  Network.setUserAgentOverride({ userAgent });
  is(
    Services.prefs.getStringPref("general.useragent.override"),
    userAgent,
    "Preference for user agent has been set"
  );
});

This test always fails and when checking the logs the dump output comes after the assertion check.

It means that somewhere in the Remote Agent code we put an async wrapper or Promise around those methods, and return immediately.

This is a somewhat critical issue given that it causes flakiness due to race conditions. As such I think it should block the alpha release.

Comment 1

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Can you reproduce this outside of tests? I wonder whether it has something to do with chrome-remote-interface.js used in the bc tests.

Comment 2

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

That's actually a very good point! I haven't thought about that, and indeed this could be the underlying reason. I will have a look right now.

Comment 3

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

Here an excerpt of the code and log when running the sync setUserAgentOverride method:

    info("\n*** about to call setUserAgentOverride\n");
    Emulation.setUserAgentOverride({ userAgent });
    info("\n*** about to check user agent preference from test\n");
    is(Services.prefs.getStringPref("general.useragent.override"), userAgent);

*** about to call setUserAgentOverride

 0:09.68 INFO
*** about to check user agent preference from test

 0:09.68 INFO
*** finally

 0:09.69 GECKO(28344) 1573765948652	RemoteAgent	TRACE	(connection {2c66da63-6772-844a-adc3-c8e736eea475})-> {"id":1,"method":"Emulation.setUserAgentOverride","params":{"userAgent":"foo bar"}}
 0:09.69 GECKO(28344) ** Set User agent pref to foo bar
 0:09.69 GECKO(28344) ** User agent pref has been set
 0:09.69 GECKO(28344) 1573765948654	RemoteAgent	TRACE	<-(connection {2c66da63-6772-844a-adc3-c8e736eea475}) {"id":1}
 0:09.70 GECKO(28344) ** Set User agent pref to
 0:09.70 GECKO(28344) ** User agent pref has been cleared

This clearly indicates that it is the fault of the chrome-remote-interface client. Calling one of its methods they all are asynchronous and require an explicit await. Otherwise the test continues and a next command / check will fail.

If we are ok with that, I'm happy to get this bug closed. But it won't allow us to check the synch/async nature of our implementation.

Comment 4

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #0)

Client code like in browser chrome tests:

add_task(async function setUserAgent({ Network }) {
  const userAgent = "foo bar";
  Network.setUserAgentOverride({ userAgent });
  is(
    Services.prefs.getStringPref("general.useragent.override"),
    userAgent,
    "Preference for user agent has been set"
  );
});

In this example, aren’t you missing async in front of Network.setUserAgentOverride?

This is calling an API in chrome-remote-interface.js that is
asynchronous because it does a network request, which means the
server is doing right thing and delaying the response, but that the
client is continuing on to send another request before the last
response has arrived back.

Comment 5

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

That's what I was referring to. As it looks like the client always handles methods asynchronously. As such we would always have to use await.

This opens the question, do we have to test if a synchronous method works as expected? Would that have to be done via an xpcshell test?

If it's not worth the trouble we could close this bug.

Comment 6

[Andreas Tolfsen ❲:ato❳]

It would be lovely if eslint was able to detect that functions
annotated async at some point gets await’ed or has .then()
called on them.

Comment 7

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

So lets keep that in mind when reviewing browser chrome tests for remote agent in the future. I'm closing this bug for now.