1806735 - Wait until fetch resolves in before_request_sent_tentative.py

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, task, P1)

Description

[Julian Descottes [:jdescottes]]

The test before_request_sent_tentative.py will create and destroy network observers as subscribe and unsubscribe to network events. We can make the test safer by awaiting on fetch.

On the implementation side, we can also avoid potential issues by keeping the network observer instance and only destroying it when we destroy the listener.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1806735 - [remote] Add browser mochitest for the shared NetworkListener

Repeatedly creating and destroying the NetworkObserver can lead to detect late requests and create incomplete NetworkEvents for them.
We should still filter out such events, depending on how we want to handle such cases in BiDi specifically, but for the sake of the tests this should reduce the risk
of failures.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1806735 - [wdspec] before_request_sent_tentative.py should await for fetch requests

Depends on D165227

We currently do not handle the Response objects returned by the fetch promise, hence the empty then() used here.
This avoids moving on to the next test until the started requests have completed.

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1806735 - [remote] Ignore network events for already in-progress requests

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1806735 - [remote] Fix argument name in signature for NetworkListener:#onNetworkEvent

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1806735 - [remote] Ignore network events for already in-progress requests

Depends on D165856

Comment 6

[Julian Descottes [:jdescottes]]

Hi James, to fix the intermittent related to this bug, one option was to ignore requests for which we missed the beginning. For instance, we start listening to network events as a response is coming back and we start getting events for this request/response, even if we "missed" the real beforeRequestSent.

The topic of ongoing requests was discussed in a working group meeting, logged at https://github.com/w3c/webdriver-bidi/pull/204#issuecomment-1309085820 , and people preferred to only emit events that were captured live if I understand correctly? Would that mean the beforeRequestSentMap (and the logic around it) is no longer needed?

(for the intermittent itself, in the end I will just improve the fetch helper to wait completely until the response has completed, but would still be interested to know if there are changes planned around beforeRequestSentMap)

Comment 7

[James Graham [:jgraham]]

I think that people wanted to emit all events that happen when we're subscribed, irrespective of whether we saw only part of a request.

From a spec point of view that means that the only required bookkeeping is to ensure that we always emit an error event when the request has an error (but the spec is unlike the implementation here because in the spec we only decide right at the end whether to emit an event or not, whereas the implementation will avoid configuraing event listeners unless we're actually subscribed).

Does that answer the question?

Comment 8

[Julian Descottes [:jdescottes]]

(In reply to James Graham [:jgraham] from comment #7)

I think that people wanted to emit all events that happen when we're subscribed, irrespective of whether we saw only part of a request.

From a spec point of view that means that the only required bookkeeping is to ensure that we always emit an error event when the request has an error (but the spec is unlike the implementation here because in the spec we only decide right at the end whether to emit an event or not, whereas the implementation will avoid configuraing event listeners unless we're actually subscribed).

Does that answer the question?

Thanks for the answer, but it's still not 100% clear for me.

For responseStarted, we first do an assert on the before request sent map before we emit the event (https://pr-preview.s3.amazonaws.com/w3c/webdriver-bidi/pull/204.html#event-network-responseStarted). The way I understand it, if we did not emit the corresponding beforeRequestSent event, we will not emit responseStarted because of this assert? So if we subscribe to network events after the beforeRequestSent trigger but before the responseStarted trigger, I'm not sure what is the expected behavior?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ecb851096f63
[remote] Add browser mochitest for the shared NetworkListener r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/31c4d20fa417
[wdspec] before_request_sent_tentative.py should await for fetch requests r=webdriver-reviewers,whimboo

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/37783 for changes under testing/web-platform/tests

Comment 11

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/ecb851096f63
https://hg.mozilla.org/mozilla-central/rev/31c4d20fa417

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot