1933331 - Update initiatorType and destination to null/"" for top level document loads

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, defect, P2)

Description

[Julian Descottes [:jdescottes]]

See more details in https://bugzilla.mozilla.org/show_bug.cgi?id=1933250

While discussing the underlying issue with initiatorType/destination for top level document loads, it probably makes sense to detect top level loads on the BiDi side and return the expected values for them.

The HTML spec says that if the navigable container is null the request should be emitted without any destination/initiatorType, so let's set default values in this case.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1933331 - [bidi] Update initiatorType and destination for top level document loads

The platform API currently returns values for initiatorType and destination
for top level document loads, but according to the HTML spec they should be null / empty.

The implementation in WebDriver BiDi NetworkRequest wrapper is updated to handle this
edge case.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1933331 - [wdspec] Update initiatorType and destination tests for top level loads

Depends on D230174

Update current expectations for top level document loads in our wdspec tests

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1933331 - [wdspec] Add tests for initiatorType and destination for iframe loads

Depends on D230175

Add new test request in initiatorType / destination tests to check iframe document loads.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2a0a1e8d0eee
[bidi] Update initiatorType and destination for top level document loads r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/4c947cf102e0
[wdspec] Update initiatorType and destination tests for top level loads r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/04f9bd3ee9b6
[wdspec] Add tests for initiatorType and destination for iframe loads r=webdriver-reviewers,Sasha

Comment 5

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

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

Comment 6

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/2a0a1e8d0eee
https://hg.mozilla.org/mozilla-central/rev/4c947cf102e0
https://hg.mozilla.org/mozilla-central/rev/04f9bd3ee9b6

Comment 7

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

Upstream PR merged by moz-wptsync-bot

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 9439824 [details]
Bug 1933331 - [bidi] Update initiatorType and destination for top level document loads

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: The information provided by two fields of the BiDi network events is incorrect for top level document loads.
  This is one of the blockers for Cypress to migrate from CDP to WebDriver BiDi, so the earliest we can provide them with a working feature, the better.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple change, WebDriver BiDi only, unit tested.
• String changes made/needed:
• Is Android affected?: No

Comment 9

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/1f92204eeb15

Comment 10

[Cosmin Sabou [:CosminS]]

Backed out from beta for causing WD test_destination_initiator related failures.

Push with failures

Failure log

Backout link

Comment 11

[Julian Descottes [:jdescottes]]

Ah sorry, I thought the test patches would be also considered for uplift here, at least D230175 is necessary here.

Comment 12

[Pascal Chevrel:pascalc]

You need to request uplift on all the patches you want uplifted.

Comment 13

[Julian Descottes [:jdescottes]]

(In reply to Pascal Chevrel:pascalc from comment #12)

You need to request uplift on all the patches you want uplifted.

Thanks for clarifying!

I used to do that systematically, but I think at some point I was told that it was not necessary to do it?

Checking other recent uplifts I requested:

• https://bugzilla.mozilla.org/show_bug.cgi?id=1922390
• https://bugzilla.mozilla.org/show_bug.cgi?id=1880477

All patches were uplifted even if the request was only on the first one.
I'll make sure to flag all patches from now on, but might be worth discussing with other release owners to make sure the policy/process is the same from one release to another?

Comment 14

[Julian Descottes [:jdescottes]]

It also seems I can't request uplift for the other patches (maybe because 1 was already accepted?).
I can only select + in the approval-mozilla-beta dropdown, and it doesn't seem to do anything.

Comment 15

[Julian Descottes [:jdescottes]]

Attachment: Bug 1933331 - [bidi] Update initiatorType and destination for top level document loads

The platform API currently returns values for initiatorType and destination
for top level document loads, but according to the HTML spec they should be null / empty.

The implementation in WebDriver BiDi NetworkRequest wrapper is updated to handle this
edge case.

Original Revision: https://phabricator.services.mozilla.com/D230174

Comment 16

[Julian Descottes [:jdescottes]]

Attachment: Bug 1933331 - [wdspec] Update initiatorType and destination tests for top level loads

Update current expectations for top level document loads in our wdspec tests

Original Revision: https://phabricator.services.mozilla.com/D230175

Comment 17

[Julian Descottes [:jdescottes]]

Attachment: Bug 1933331 - [wdspec] Add tests for initiatorType and destination for iframe loads

Add new test request in initiatorType / destination tests to check iframe document loads.

Original Revision: https://phabricator.services.mozilla.com/D230178

Comment 18

[Julian Descottes [:jdescottes]]

(In reply to Julian Descottes [:jdescottes] from comment #14)

It also seems I can't request uplift for the other patches (maybe because 1 was already accepted?).
I can only select + in the approval-mozilla-beta dropdown, and it doesn't seem to do anything.

Ok so looks like this is because I'm still using bugzilla for uplift requests while I should not. I was mostly away when this was introduced last year, so I completely missed this change and kept using bugzilla. Might be good to have a warning on bugzilla UI ?

Comment 19

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: The information provided by two fields of the BiDi network events is incorrect for top level document loads. This is one of the blockers for Cypress to migrate from CDP to WebDriver BiDi, so the earliest we can provide them with a working feature, the better.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: --
• Risk associated with taking this patch: Low
• Explanation of risk level: Simple change, WebDriver BiDi only, unit tested.
• String changes made/needed: None
• Is Android affected?: no

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/dfe01caabb2d
https://hg.mozilla.org/releases/mozilla-beta/rev/36a7f720de4f
https://hg.mozilla.org/releases/mozilla-beta/rev/410896982084