Yoav Weiss (@Shopify) (Gerrit)

unread,

Jun 18, 2024, 5:07:32 PMJun 18

to Arthur Sonzogni, Vladimir Levin, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni, Reilly Grant and Vladimir Levin

Yoav Weiss (@Shopify) added 1 comment

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Yoav Weiss (@Shopify) . resolved

Vlad and Reilly, this should be identical to the changes you reviewed in https://chromium-review.googlesource.com/c/chromium/src/+/5581251.

Arthur - can you take a look at the tests? I still need add the process isolation tests..

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni
Reilly Grant
Vladimir Levin

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reilly Grant (Gerrit)

unread,

Jun 18, 2024, 6:46:13 PMJun 18

to Yoav Weiss (@Shopify), Reilly Grant, Arthur Sonzogni, Vladimir Levin, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni, Vladimir Levin and Yoav Weiss (@Shopify)

Reilly Grant voted and added 1 comment

Votes added by Reilly Grant

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Reilly Grant . resolved

//services/network LGTM

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni

Vladimir Levin
Yoav Weiss (@Shopify)

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Arthur Sonzogni (Gerrit)

unread,

Jun 19, 2024, 6:54:19 AM (14 days ago) Jun 19

to Yoav Weiss (@Shopify), Reilly Grant, Vladimir Levin, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Vladimir Levin and Yoav Weiss (@Shopify)

Arthur Sonzogni added 8 comments

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Arthur Sonzogni . resolved

THanks!

File third_party/blink/web_tests/TestExpectations

Line 6560, Patchset 2 (Latest):

crbug.com/344963946 virtual/coop-noopener-allow-popups/external/wpt/html/cross-origin-opener-policy/coop-noopener-allow-popups-restrict-properties.tentative.https.html [ Failure Pass ]

Arthur Sonzogni . unresolved

Does it fail, or does it pass?

Line 6561, Patchset 2 (Latest):

crbug.com/344963946 virtual/coop-noopener-allow-popups/external/wpt/html/cross-origin-opener-policy/coop-noopener-allow-popups.https.html [ Failure Pass ]

Arthur Sonzogni . unresolved

This is meant for flaky tests or things the gardeners quickly disabled.

It would be better to have some test expectations files instead.
See:
https://chromium.googlesource.com/chromium/src/+/refs/heads/main/docs/testing/web_test_expectations.md#local-manual-rebaselining

You can use `--reset-results` to create those files.

File third_party/blink/web_tests/VirtualTestSuites

Line 1882, Patchset 2 (Latest): "bases": ["external/wpt/html/cross-origin-opener-policy/coop-noopener-allow-popups.https.html",

Arthur Sonzogni . unresolved

Should you group everything related to this feature under a common directory?

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/coop-noopener-allow-popups-restrict-properties.tentative.https.html

Line 7, Patchset 2 (Latest):<script src=/resources/testharness.js></script>

Arthur Sonzogni . unresolved

optional nit: add " around every attributes value.

Line 21, Patchset 2 (Latest): "restrict-properties-allow-popups",

Arthur Sonzogni . unresolved

This doesn't exist.

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/coop-noopener-allow-popups.https.html

Line 1, Patchset 2 (Latest):<!doctype html>

Arthur Sonzogni . unresolved

This is testing non specified behavior, so the filename should contain "tentative" or be put an a directory about experimental tests.

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/resources/noopener-helper.js

Line 10, Patchset 2 (Latest):const test_noopener_opening_popup = (opener_coop, openee_coop, opener_expectation) => {

Arthur Sonzogni . unresolved

nit: wrap to fit 80 columns.

Open in Gerrit

Related details

Attention is currently required from:

Vladimir Levin
Yoav Weiss (@Shopify)

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yoav Weiss (@Shopify) (Gerrit)

unread,

Jun 19, 2024, 9:31:07 AM (14 days ago) Jun 19

to Chromium LUCI CQ, Reilly Grant, Arthur Sonzogni, Vladimir Levin, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni, Reilly Grant and Vladimir Levin

Yoav Weiss (@Shopify) added 7 comments

File third_party/blink/web_tests/TestExpectations

Line 6560, Patchset 2:

crbug.com/344963946 virtual/coop-noopener-allow-popups/external/wpt/html/cross-origin-opener-policy/coop-noopener-allow-popups-restrict-properties.tentative.https.html [ Failure Pass ]

Arthur Sonzogni . resolved

Does it fail, or does it pass?

Yoav Weiss (@Shopify)

Removed!

Line 6561, Patchset 2:

crbug.com/344963946 virtual/coop-noopener-allow-popups/external/wpt/html/cross-origin-opener-policy/coop-noopener-allow-popups.https.html [ Failure Pass ]

Arthur Sonzogni . resolved

This is meant for flaky tests or things the gardeners quickly disabled.
It would be better to have some test expectations files instead.
See:
https://chromium.googlesource.com/chromium/src/+/refs/heads/main/docs/testing/web_test_expectations.md#local-manual-rebaselining
You can use `--reset-results` to create those files.

Yoav Weiss (@Shopify)

Created expectations!

File third_party/blink/web_tests/VirtualTestSuites

Line 1882, Patchset 2: "bases": ["external/wpt/html/cross-origin-opener-policy/coop-noopener-allow-popups.https.html",

Arthur Sonzogni . resolved

Should you group everything related to this feature under a common directory?

Yoav Weiss (@Shopify)

Done

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/coop-noopener-allow-popups-restrict-properties.tentative.https.html

Line 7, Patchset 2:<script src=/resources/testharness.js></script>

Arthur Sonzogni . resolved

optional nit: add " around every attributes value.

Yoav Weiss (@Shopify)

Done

Line 21, Patchset 2: "restrict-properties-allow-popups",

Arthur Sonzogni . resolved

This doesn't exist.

Yoav Weiss (@Shopify)

oops!

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/coop-noopener-allow-popups.https.html

Line 1, Patchset 2:<!doctype html>

Arthur Sonzogni . resolved

This is testing non specified behavior, so the filename should contain "tentative" or be put an a directory about experimental tests.

Yoav Weiss (@Shopify)

Done

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/resources/noopener-helper.js

Line 10, Patchset 2:const test_noopener_opening_popup = (opener_coop, openee_coop, opener_expectation) => {

Arthur Sonzogni . resolved

nit: wrap to fit 80 columns.

Yoav Weiss (@Shopify)

Done

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni
Reilly Grant
Vladimir Levin

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Vladimir Levin (Gerrit)

unread,

Jun 19, 2024, 9:29:00 PM (13 days ago) Jun 19

to Yoav Weiss (@Shopify), Chromium LUCI CQ, Reilly Grant, Arthur Sonzogni, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni, Reilly Grant and Yoav Weiss (@Shopify)

Vladimir Levin voted and added 1 comment

Votes added by Vladimir Levin

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Vladimir Levin . resolved

VTS lgtm

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni
Reilly Grant
Yoav Weiss (@Shopify)

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Vladimir Levin (Gerrit)

unread,

Jun 20, 2024, 11:14:23 AM (13 days ago) Jun 20

to Yoav Weiss (@Shopify), Chromium LUCI CQ, Reilly Grant, Arthur Sonzogni, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni and Yoav Weiss (@Shopify)

Vladimir Levin voted and added 1 comment

Votes added by Vladimir Levin

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Vladimir Levin . resolved

VTS slgtm

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni
Yoav Weiss (@Shopify)

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Arthur Sonzogni (Gerrit)

unread,

Jun 21, 2024, 7:21:39 AM (12 days ago) Jun 21

to Yoav Weiss (@Shopify), Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Yoav Weiss (@Shopify)

Arthur Sonzogni voted and added 13 comments

Votes added by Arthur Sonzogni

Code-Review

+1

13 comments

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Arthur Sonzogni . resolved

Thanks!
LGTM % 12 suggestions below:

File content/browser/isolated_origin_browsertest.cc

Line 6753, Patchset 4 (Latest):

// Verify that the `noopener-allow-popups COOP header value triggers
// isolation, and that this behaves sanely with window.open().

Arthur Sonzogni . unresolved

nit: fit 80 columns:
```
// Verify that the `noopener-allow-popups COOP header value triggers isolation,
// and that this behaves sanely with window.open().
```

Line 6777, Patchset 4 (Latest):

  // swap a BrowsingInstance because of noopener-allow-popups.  Verify that the
  // popup ends up in a different SiteInstance from the opener (which requires a
  // dedicated process).

Arthur Sonzogni . unresolved

The implication: "different SiteInstance" => "different process" is not true in general. Could you rework the comment to avoid making this claim?

Line 6788, Patchset 4 (Latest): EXPECT_EQ(popup_rfh->GetSiteInstance(), coop_instance);

Arthur Sonzogni . unresolved

Can we check if the SiteInstance::GetProcess() are different?

File content/browser/security/coop/cross_origin_opener_policy_browsertest.cc

Line 8723, Patchset 4 (Latest): NavigationVirtualBrowsingContextGroupNoopener) {

Arthur Sonzogni . unresolved

Could you duplicate this test, but use COOP-RO instead of COOP?

Line 8739, Patchset 4 (Latest):

          false,
          false,

Arthur Sonzogni . unresolved

Note: I am expecting those values to be updated once you land the implementation. You might want to add a TODO making it clear this is expected to be updated.

File services/network/public/cpp/cross_origin_opener_policy_parser_unittest.cc

Line 23, Patchset 4 (Latest): features::kCoopNoopenerAllowPopups},

Arthur Sonzogni . unresolved

add a trailing "," to get a better formatting.
```
      {
          features::kCrossOriginOpenerPolicy,
          features::kCoopRestrictProperties,
          features::kCoopNoopenerAllowPopups,
      },
```

Line 209, Patchset 4 (Latest): {"noopener-allow-popups", kCoepNone, kNoHeader, kCoepNone, kNoEndpoint,

Arthur Sonzogni . unresolved

Optional: You might want to add a TODO about fixing landing the implementation to fix this.

File services/network/public/cpp/features.cc

Line 98, Patchset 4 (Latest):// severe its opener relationship with the document that opened it.

Arthur Sonzogni . unresolved

Could you add a link to the proposal?

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/coop-noopener-allow-popups-restrict-properties.tentative.https.html

Line 7, Patchset 2:<script src=/resources/testharness.js></script>

Arthur Sonzogni . unresolved

optional nit: add " around every attributes value.

Yoav Weiss (@Shopify)

Done

Arthur Sonzogni

(re-open, because it is not done)

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/resources/noopener-helper.js

Line 10, Patchset 4 (Latest):const test_noopener_opening_popup =

Arthur Sonzogni . unresolved

nit: Add a description of the test:
```
// Open a same-origin popup with `opener_coop` header, then open a popup with
// `openee_coop` header. Check whether the opener can script the openee.
```

Line 103, Patchset 4 (Latest):const test_noopener_navigating_away = (popup_coop) => {

Arthur Sonzogni . unresolved

nit: Add a description.
```
// Open a same-origin popup with `popup_coop` header, then navigate away toward
// one with `noopener-allow-popups`. Check the opener can't script the openee.
```

Line 1, Patchset 4 (Latest):

const executor_path = '/common/dispatcher/executor.html?pipe=';
const coop_header = policy => {
  return `|header(Cross-Origin-Opener-Policy,${policy})`;
};

function getExecutorPath(uuid, origin, coop_header) {
  return origin.origin + executor_path + coop_header + `&uuid=${uuid}`;


}

const test_noopener_opening_popup =
    (opener_coop, openee_coop, opener_expectation) => {

      promise_test(
          async t => {
            // Set up dispatcher communications.
            const popup_token = token();
            const reply_token = token();
            const popup_reply_token = token();
            const popup_openee_token = token();

            const popup_url = getExecutorPath(
                popup_token, SAME_ORIGIN, coop_header(opener_coop));

            // We open a popup and then ping it, it will respond after loading.
            const popup = window.open(popup_url);
            t.add_cleanup(() => send(popup_token, 'window.close()'));
            send(popup_token, `send('${reply_token}', 'Popup loaded');`);
            assert_equals(await receive(reply_token), 'Popup loaded');

            if (opener_coop == 'noopener-allow-popups') {
              // Assert that we can't script the popup.
              assert_equals(popup.window, null, 'Opener popup.window is null');
              assert_true(popup.closed, 'Opener popup.closed');
            }

            // Ensure that the popup has no access to its opener.
            send(popup_token, `
      let openerDOMAccessAllowed = false;
      try {
        openerDOMAccessAllowed = !!self.opener.document.URL;
      } catch(ex) {
      }
      const payload = {
        opener: !!self.opener,
        openerDOMAccess: openerDOMAccessAllowed
      };
      send('${reply_token}', JSON.stringify(payload));
    `);
            let payload = JSON.parse(await receive(reply_token));
            if (opener_coop == 'noopener-allow-popups') {
              assert_false(payload.opener, 'popup opener');
              assert_false(payload.openerDOMAccess, 'popup DOM access');
            }

            // Open another popup from inside the popup, and wait for it to
            // load.
            const popup_openee_url = getExecutorPath(
                popup_openee_token, SAME_ORIGIN, coop_header(openee_coop));
            send(popup_token, `
      window.openee = open("${popup_openee_url}");
      await receive('${popup_reply_token}');
      const payload = {
        openee: !!window.openee,
        closed: window.openee.closed
      };
      send('${reply_token}', JSON.stringify(payload));
    `);
            t.add_cleanup(() => send(popup_token, 'window.openee.close()'));
            // Notify the popup that its openee was loaded.
            send(popup_openee_token, `send('${popup_reply_token}',
         'popup openee opened');`);

            // Assert that the popup has access to its openee.
            payload = JSON.parse(await receive(reply_token));
            assert_true(payload.openee, 'popup openee');

            // Assert that the openee has access to its popup opener.
            send(popup_openee_token, `
      let openerDOMAccessAllowed = false;
      try {
        openerDOMAccessAllowed = !!self.opener.document.URL;
      } catch(ex) {
      }
      const payload = {
        opener: !!self.opener,
        openerDOMAccess: openerDOMAccessAllowed
      };
      send('${reply_token}', JSON.stringify(payload));
    `);
            payload = JSON.parse(await receive(reply_token));
            if (opener_expectation) {
              assert_true(payload.opener, 'Opener is not null');
              assert_true(payload.openerDOMAccess, 'No DOM access');
            } else {
              assert_false(payload.opener, 'Opener is null');
              assert_false(payload.openerDOMAccess, 'No DOM access');
            }
          },
          'noopener-allow-popups ensures that the opener cannot script the openee,' +
              ' but further popups with no COOP can access their opener: ' +
              opener_coop + '/' + openee_coop);
    };

const test_noopener_navigating_away = (popup_coop) => {
  promise_test(
      async t => {
        // Set up dispatcher communications.
        const popup_token = token();
        const reply_token = token();
        const popup_reply_token = token();
        const popup_second_token = token();

        const popup_url =
            getExecutorPath(popup_token, SAME_ORIGIN, coop_header(popup_coop));

        // We open a popup and then ping it, it will respond after loading.
        const popup = window.open(popup_url);
        send(popup_token, `send('${reply_token}', 'Popup loaded');`);
        assert_equals(await receive(reply_token), 'Popup loaded');
        t.add_cleanup(() => send(popup_token, 'window.close()'));

        // Assert that we can script the popup.
        assert_not_equals(popup.window, null);
        assert_false(popup.closed, 'popup closed');

        // Ensure that the popup has no access to its opener.
        send(popup_token, `
      let openerDOMAccessAllowed = false;
      try {
        openerDOMAccessAllowed = !!self.opener.document.URL;
      } catch(ex) {
      }
      const payload = {
        opener: !!self.opener,
        openerDOMAccess: openerDOMAccessAllowed
      };
      send('${reply_token}', JSON.stringify(payload));
    `);
        let payload = JSON.parse(await receive(reply_token));
        assert_true(payload.opener, 'popup opener');
        assert_true(payload.openerDOMAccess, 'popup DOM access');

        // Navigate the popup
        const popup_second_url = getExecutorPath(
            popup_second_token, SAME_ORIGIN,
            coop_header('noopener-allow-popups'));

        send(popup_token, `
      window.location = '${popup_second_url}';
    `);

        // Notify the popup that its openee was loaded.
        send(
            popup_second_token,
            `send('${reply_token}', 'popup navigated away');`);
        assert_equals(await receive(reply_token), 'popup navigated away');

        assert_equals(popup.window, null);
        assert_true(popup.closed, 'popup.closed');
      },
      'noopener-allow-popups ensures that the opener cannot script the openee,' +
          ' even after a navigation: ' + popup_coop);
};

Arthur Sonzogni . unresolved

The indentation is broken. Could you fix it?
```suggestion
const executor_path = '/common/dispatcher/executor.html?pipe=';
const coop_header = policy => {
  return `|header(Cross-Origin-Opener-Policy,${policy})`;
};

function getExecutorPath(uuid, origin, coop_header) {
  return origin.origin + executor_path + coop_header + `&uuid=${uuid}`;
}

// Open a same-origin popup with `opener_coop` header, then open a popup with
// `openee_coop` header. Check whether the opener can script the openee.
const test_noopener_opening_popup = (
  opener_coop,
  openee_coop,
  opener_expectation
) => {
  promise_test(async t => {
    // Set up dispatcher communications.
    const popup_token = token();
    const reply_token = token();
    const popup_reply_token = token();
    const popup_openee_token = token();

    const popup_url = getExecutorPath(
      popup_token, SAME_ORIGIN, coop_header(opener_coop));

    // We open a popup and then ping it, it will respond after loading.
    const popup = window.open(popup_url);
    t.add_cleanup(() => send(popup_token, 'window.close()'));
    send(popup_token, `send('${reply_token}', 'Popup loaded');`);
    assert_equals(await receive(reply_token), 'Popup loaded');

    if (opener_coop == 'noopener-allow-popups') {
      // Assert that we can't script the popup.
      assert_equals(popup.window, null, 'Opener popup.window is null');
      assert_true(popup.closed, 'Opener popup.closed');
    }

    // Ensure that the popup has no access to its opener.
    send(popup_token, `
      let openerDOMAccessAllowed = false;
      try {
        openerDOMAccessAllowed = !!self.opener.document.URL;
      } catch(ex) {}
      const payload = {
        opener: !!self.opener,
        openerDOMAccess: openerDOMAccessAllowed
      };
      send('${reply_token}', JSON.stringify(payload));
    `);
    let payload = JSON.parse(await receive(reply_token));
    if (opener_coop == 'noopener-allow-popups') {
      assert_false(payload.opener, 'popup opener');
      assert_false(payload.openerDOMAccess, 'popup DOM access');
    }

    // Open another popup from inside the popup, and wait for it to load.
    const popup_openee_url = getExecutorPath(popup_openee_token, SAME_ORIGIN,
      coop_header(openee_coop));
    send(popup_token, `
      window.openee = open("${popup_openee_url}");
      await receive('${popup_reply_token}');
      const payload = {
        openee: !!window.openee,
        closed: window.openee.closed
      };
      send('${reply_token}', JSON.stringify(payload));
    `);
    t.add_cleanup(() => send(popup_token, 'window.openee.close()'));
    // Notify the popup that its openee was loaded.
    send(popup_openee_token, `
      send('${popup_reply_token}', 'popup openee opened');
    `);

    // Assert that the popup has access to its openee.
    payload = JSON.parse(await receive(reply_token));
    assert_true(payload.openee, 'popup openee');

    // Assert that the openee has access to its popup opener.
    send(popup_openee_token, `
      let openerDOMAccessAllowed = false;
      try {
        openerDOMAccessAllowed = !!self.opener.document.URL;
      } catch(ex) {}
      const payload = {
        opener: !!self.opener,
        openerDOMAccess: openerDOMAccessAllowed
      };
      send('${reply_token}', JSON.stringify(payload));
    `);
    payload = JSON.parse(await receive(reply_token));
    if (opener_expectation) {
      assert_true(payload.opener, 'Opener is not null');
      assert_true(payload.openerDOMAccess, 'No DOM access');
    } else {
      assert_false(payload.opener, 'Opener is null');
      assert_false(payload.openerDOMAccess, 'No DOM access');
    }
  },
  'noopener-allow-popups ensures that the opener cannot script the openee,' +
  ' but further popups with no COOP can access their opener: ' +
  opener_coop + '/' + openee_coop);
};

// Open a same-origin popup with `popup_coop` header, then navigate away toward
// one with `noopener-allow-popups`. Check the opener can't script the openee.
const test_noopener_navigating_away = (popup_coop) => {
  promise_test(async t => {
    // Set up dispatcher communications.
    const popup_token = token();
    const reply_token = token();
    const popup_reply_token = token();
    const popup_second_token = token();

    const popup_url =
      getExecutorPath(popup_token, SAME_ORIGIN, coop_header(popup_coop));

    // We open a popup and then ping it, it will respond after loading.
    const popup = window.open(popup_url);
    send(popup_token, `send('${reply_token}', 'Popup loaded');`);
    assert_equals(await receive(reply_token), 'Popup loaded');
    t.add_cleanup(() => send(popup_token, 'window.close()'));

    // Assert that we can script the popup.
    assert_not_equals(popup.window, null);
    assert_false(popup.closed, 'popup closed');

    // Ensure that the popup has no access to its opener.
    send(popup_token, `
      let openerDOMAccessAllowed = false;
      try {
        openerDOMAccessAllowed = !!self.opener.document.URL;
      } catch(ex) {}
      const payload = {
        opener: !!self.opener,
        openerDOMAccess: openerDOMAccessAllowed
      };
      send('${reply_token}', JSON.stringify(payload));
    `);
    let payload = JSON.parse(await receive(reply_token));
    assert_true(payload.opener, 'popup opener');
    assert_true(payload.openerDOMAccess, 'popup DOM access');

    // Navigate the popup.
    const popup_second_url = getExecutorPath(
      popup_second_token, SAME_ORIGIN,
      coop_header('noopener-allow-popups'));

    send(popup_token, `
      window.location = '${popup_second_url}';
    `);

    // Notify the popup that its openee was loaded.
    send(popup_second_token, `
      send('${reply_token}', 'popup navigated away');
    `);
    assert_equals(await receive(reply_token), 'popup navigated away');

    assert_equals(popup.window, null);
    assert_true(popup.closed, 'popup.closed');
  },
  'noopener-allow-popups ensures that the opener cannot script the openee,' +
  ' even after a navigation: ' + popup_coop);
};
```

Open in Gerrit

Related details

Attention is currently required from:

Yoav Weiss (@Shopify)

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yoav Weiss (@Shopify) (Gerrit)

unread,

Jul 1, 2024, 10:21:52 AM (2 days ago) Jul 1

to Arthur Sonzogni, Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Yoav Weiss (@Shopify) added 5 comments

File content/browser/isolated_origin_browsertest.cc

Line 6753, Patchset 4:// Verify that the `noopener-allow-popups COOP header value triggers


// isolation, and that this behaves sanely with window.open().

Arthur Sonzogni . resolved

nit: fit 80 columns:
```
// Verify that the `noopener-allow-popups COOP header value triggers isolation,
// and that this behaves sanely with window.open().
```

Yoav Weiss (@Shopify)

Done

Line 6777, Patchset 4: // swap a BrowsingInstance because of noopener-allow-popups. Verify that the


  // popup ends up in a different SiteInstance from the opener (which requires a
  // dedicated process).

Arthur Sonzogni . resolved

The implication: "different SiteInstance" => "different process" is not true in general. Could you rework the comment to avoid making this claim?

Yoav Weiss (@Shopify)

Removed the parenthesis. (but a similar comment is already in line 6715)

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/resources/noopener-helper.js

Line 10, Patchset 4:const test_noopener_opening_popup =

Arthur Sonzogni . resolved

nit: Add a description of the test:
```
// Open a same-origin popup with `opener_coop` header, then open a popup with
// `openee_coop` header. Check whether the opener can script the openee.
```

Yoav Weiss (@Shopify)

Done

Line 103, Patchset 4:const test_noopener_navigating_away = (popup_coop) => {

Arthur Sonzogni . resolved

nit: Add a description.
```
// Open a same-origin popup with `popup_coop` header, then navigate away toward
// one with `noopener-allow-popups`. Check the opener can't script the openee.
```

Yoav Weiss (@Shopify)

Done

Line 1, Patchset 4:const executor_path = '/common/dispatcher/executor.html?pipe=';

Arthur Sonzogni . resolved

Yoav Weiss (@Shopify)

Done! `git cl format --js` messed up the indentation, it seems..

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yoav Weiss (@Shopify) (Gerrit)

unread,

Jul 1, 2024, 10:33:54 AM (2 days ago) Jul 1

to Arthur Sonzogni, Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni

Yoav Weiss (@Shopify) added 2 comments

File content/browser/security/coop/cross_origin_opener_policy_browsertest.cc

Line 8723, Patchset 4: NavigationVirtualBrowsingContextGroupNoopener) {

Arthur Sonzogni . unresolved

Could you duplicate this test, but use COOP-RO instead of COOP?

Yoav Weiss (@Shopify)

Would you like a completely separate test, or should I just add the RO cases to this one?

Line 8739, Patchset 4:

          false,
          false,

Arthur Sonzogni . unresolved

Note: I am expecting those values to be updated once you land the implementation. You might want to add a TODO making it clear this is expected to be updated.

Yoav Weiss (@Shopify)

Adding a TODO

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yoav Weiss (@Shopify) (Gerrit)

unread,

Jul 1, 2024, 10:43:05 AM (2 days ago) Jul 1

to Arthur Sonzogni, Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni

Yoav Weiss (@Shopify) added 3 comments

File services/network/public/cpp/cross_origin_opener_policy_parser_unittest.cc

Line 23, Patchset 4: features::kCoopNoopenerAllowPopups},

Arthur Sonzogni . resolved

add a trailing "," to get a better formatting.
```
      {
          features::kCrossOriginOpenerPolicy,
          features::kCoopRestrictProperties,
          features::kCoopNoopenerAllowPopups,
      },
```

Yoav Weiss (@Shopify)

Done

Line 209, Patchset 4: {"noopener-allow-popups", kCoepNone, kNoHeader, kCoepNone, kNoEndpoint,

Arthur Sonzogni . resolved

Optional: You might want to add a TODO about fixing landing the implementation to fix this.

Yoav Weiss (@Shopify)

Done

File services/network/public/cpp/features.cc

Line 98, Patchset 4:// severe its opener relationship with the document that opened it.

Arthur Sonzogni . resolved

Could you add a link to the proposal?

Yoav Weiss (@Shopify)

Done

satisfied_requirement

unsatisfied_requirement

open

diffy

Yoav Weiss (@Shopify) (Gerrit)

unread,

Jul 1, 2024, 10:43:35 AM (2 days ago) Jul 1

to Arthur Sonzogni, Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni

Yoav Weiss (@Shopify) added 1 comment

File content/browser/security/coop/cross_origin_opener_policy_browsertest.cc

Line 8739, Patchset 4:

          false,
          false,

Arthur Sonzogni . resolved

Note: I am expecting those values to be updated once you land the implementation. You might want to add a TODO making it clear this is expected to be updated.

Yoav Weiss (@Shopify)

Adding a TODO

Yoav Weiss (@Shopify)

Done

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yoav Weiss (@Shopify) (Gerrit)

unread,

Jul 1, 2024, 10:49:14 AM (2 days ago) Jul 1

to Arthur Sonzogni, Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni

Yoav Weiss (@Shopify) added 1 comment

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/coop-noopener-allow-popups-restrict-properties.tentative.https.html

Line 7, Patchset 2:<script src=/resources/testharness.js></script>

Arthur Sonzogni . resolved

optional nit: add " around every attributes value.

Yoav Weiss (@Shopify)

Done

Arthur Sonzogni

(re-open, because it is not done)

Yoav Weiss (@Shopify)

Oops!! Should be good now

satisfied_requirement

unsatisfied_requirement

open

diffy

Yoav Weiss (@Shopify) (Gerrit)

unread,

Jul 1, 2024, 10:59:30 AM (2 days ago) Jul 1

to Arthur Sonzogni, Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni

Yoav Weiss (@Shopify) added 2 comments

File content/browser/isolated_origin_browsertest.cc

Line 6788, Patchset 4: EXPECT_EQ(popup_rfh->GetSiteInstance(), coop_instance);

Arthur Sonzogni . resolved

Can we check if the SiteInstance::GetProcess() are different?

Yoav Weiss (@Shopify)

Added a check (that currently fails)

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/resources/noopener-helper.js

Line 35, Patchset 7: assert_equals(popup.window, null, 'Opener popup.window is null');

Yoav Weiss (@Shopify) . unresolved

I'm thinking of removing this check, as I'm not sure it's guaranteed to be true (without races). Thoughts?

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Arthur Sonzogni (Gerrit)

unread,

Jul 1, 2024, 12:27:34 PM (2 days ago) Jul 1

to Yoav Weiss (@Shopify), Arthur Sonzogni, Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni and Yoav Weiss (@Shopify)

Arthur Sonzogni added 1 comment

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/resources/noopener-helper.js

Line 35, Patchset 7: assert_equals(popup.window, null, 'Opener popup.window is null');

Yoav Weiss (@Shopify) . unresolved

I'm thinking of removing this check, as I'm not sure it's guaranteed to be true (without races). Thoughts?

Arthur Sonzogni

Indeed, I see there are no causality chains preventing this race from happening:
```
 ┌────┐                   ┌───────┐                                 ┌─────────────┐┌─────────────┐
 │test│                   │browser│                                 │popup_old_doc││popup_new_doc│
 └─┬──┘                   └───┬───┘                                 └──────┬──────┘└──────┬──────┘
   │                          │                                            │              │       
   │       window.open        │                                            │              │       
   │─────────────────────────>│                                            │              │       
   │                          │                                            │              │       
   │                          │Create window and the initial empty document│              │       
   │                          │───────────────────────────────────────────>│              │       
   │                          │                                            │              │       
   │                          │               CommitNavigation in a new doc│              │       
   │                          │──────────────────────────────────────────────────────────>│       
   │                          │                                            │              │       
   │                          │               SwapIn (DidCommitNavigation) │              │       
   │                          │<──────────────────────────────────────────────────────────│       
   │                          │                                            │              │       
   │                          │        Destroy RenderFrameHostProxy        │              │       
   │                          │<───────────────────────────────────────────│              │       
   │                          │                                            │              │       
   │                          │              Ping                          │              │       
   │─────────────────────────────────────────────────────────────────────────────────────>│       
   │                          │              Pong                          │              │       
   │<─────────────────────────────────────────────────────────────────────────────────────│       
   │                          │                                            │              │       
   │                          │                  SwapOut                   │              │       
   │                          │───────────────────────────────────────────>│              │       
   │                          │         Remove old_doc RenderFrameHost     │              │       
   │                          │<───────────────────────────────────────────│              │       
   │Remove old WindowProxy    │                                            │              │       
   │<─────────────────────────│                                            │              │       
 ┌─┴──┐                   ┌───┴───┐                                 ┌──────┴──────┐┌──────┴──────┐
 │test│                   │browser│                                 │popup_old_doc││popup_new_doc│
 └────┘                   └───────┘                                 └─────────────┘└─────────────┘

```

It means this process (1) can talk to the process (2) handling creating the popup's new document, while the process (3) is destroying the popup's old document.

Could you wait for this process (1) to see the destruction of the old document using:
https://web-platform-tests.org/writing-tests/testharness-api.html#timers-in-tests

e.g. `step_wait()`?

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni
Yoav Weiss (@Shopify)

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yoav Weiss (@Shopify) (Gerrit)

unread,

Jul 1, 2024, 12:47:40 PM (2 days ago) Jul 1

to Arthur Sonzogni, Arthur Sonzogni, Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni

Yoav Weiss (@Shopify) voted and added 1 comment

Votes added by Yoav Weiss (@Shopify)

Commit-Queue

+1

1 comment

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/resources/noopener-helper.js

Line 35, Patchset 7: assert_equals(popup.window, null, 'Opener popup.window is null');

Yoav Weiss (@Shopify) . resolved

I'm thinking of removing this check, as I'm not sure it's guaranteed to be true (without races). Thoughts?

Yoav Weiss (@Shopify)

@arthurs...@chromium.org confirmed that this is racy, so removed.

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Arthur Sonzogni (Gerrit)

unread,

Jul 1, 2024, 12:52:31 PM (2 days ago) Jul 1

to Yoav Weiss (@Shopify), Arthur Sonzogni, Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni and Yoav Weiss (@Shopify)

Arthur Sonzogni voted and added 1 comment

Votes added by Arthur Sonzogni

Code-Review

+1

1 comment

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/resources/noopener-helper.js

Line 35, Patchset 7: assert_equals(popup.window, null, 'Opener popup.window is null');

Yoav Weiss (@Shopify) . unresolved

I'm thinking of removing this check, as I'm not sure it's guaranteed to be true (without races). Thoughts?

Yoav Weiss (@Shopify)

@arthurs...@chromium.org confirmed that this is racy, so removed.

Arthur Sonzogni

What about `step_wait()` conditions?
```
await t.step_wait(() => popup.window == null, 'Opener popup.window becomes null')
```

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni
Yoav Weiss (@Shopify)

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yoav Weiss (@Shopify) (Gerrit)

unread,

Jul 2, 2024, 1:45:13 AM (yesterday) Jul 2

to Arthur Sonzogni, Arthur Sonzogni, Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni and Arthur Sonzogni

Yoav Weiss (@Shopify) added 1 comment

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/resources/noopener-helper.js

Line 35, Patchset 7: assert_equals(popup.window, null, 'Opener popup.window is null');

Yoav Weiss (@Shopify) . unresolved

I'm thinking of removing this check, as I'm not sure it's guaranteed to be true (without races). Thoughts?

Yoav Weiss (@Shopify)

@arthurs...@chromium.org confirmed that this is racy, so removed.

Arthur Sonzogni

What about `step_wait()` conditions?
```
await t.step_wait(() => popup.window == null, 'Opener popup.window becomes null')
```

Yoav Weiss (@Shopify)

Added an await (with a 1 second timeout) and it seems flaky..

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni
Arthur Sonzogni

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Arthur Sonzogni (Gerrit)

unread,

Jul 2, 2024, 3:43:28 AM (yesterday) Jul 2

to Yoav Weiss (@Shopify), Arthur Sonzogni, Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni and Yoav Weiss (@Shopify)

Arthur Sonzogni added 1 comment

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/resources/noopener-helper.js

Line 35, Patchset 7: assert_equals(popup.window, null, 'Opener popup.window is null');

Yoav Weiss (@Shopify) . unresolved

I'm thinking of removing this check, as I'm not sure it's guaranteed to be true (without races). Thoughts?

Yoav Weiss (@Shopify)

@arthurs...@chromium.org confirmed that this is racy, so removed.

Arthur Sonzogni

What about `step_wait()` conditions?
```
await t.step_wait(() => popup.window == null, 'Opener popup.window becomes null')
```

Yoav Weiss (@Shopify)

Added an await (with a 1 second timeout) and it seems flaky..

Arthur Sonzogni

Why are you talking about a timeout? Did you try `step_awaiŧ`?

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni
Yoav Weiss (@Shopify)

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yoav Weiss (@Shopify) (Gerrit)

unread,

Jul 2, 2024, 4:01:18 AM (yesterday) Jul 2

to Arthur Sonzogni, Arthur Sonzogni, Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni and Arthur Sonzogni

Yoav Weiss (@Shopify) added 1 comment

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/resources/noopener-helper.js

Line 35, Patchset 7: assert_equals(popup.window, null, 'Opener popup.window is null');

Yoav Weiss (@Shopify) . unresolved

I'm thinking of removing this check, as I'm not sure it's guaranteed to be true (without races). Thoughts?

Yoav Weiss (@Shopify)

@arthurs...@chromium.org confirmed that this is racy, so removed.

Arthur Sonzogni

What about `step_wait()` conditions?
```
await t.step_wait(() => popup.window == null, 'Opener popup.window becomes null')
```

Yoav Weiss (@Shopify)

Added an await (with a 1 second timeout) and it seems flaky..

Arthur Sonzogni

Why are you talking about a timeout? Did you try `step_awaiŧ`?

Yoav Weiss (@Shopify)

I added `await t.step_wait(() => popup.window == null, 'Opener popup.window becomes null', 1000)`, and (so a timeout of 1 second) and I see it timing out occasionally

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni
Arthur Sonzogni

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Arthur Sonzogni (Gerrit)

unread,

Jul 2, 2024, 4:02:18 AM (yesterday) Jul 2

to Yoav Weiss (@Shopify), Arthur Sonzogni, Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni and Yoav Weiss (@Shopify)

Arthur Sonzogni added 1 comment

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/resources/noopener-helper.js

Line 35, Patchset 7: assert_equals(popup.window, null, 'Opener popup.window is null');

Yoav Weiss (@Shopify) . unresolved

I'm thinking of removing this check, as I'm not sure it's guaranteed to be true (without races). Thoughts?

Yoav Weiss (@Shopify)

@arthurs...@chromium.org confirmed that this is racy, so removed.

Arthur Sonzogni

What about `step_wait()` conditions?
```
await t.step_wait(() => popup.window == null, 'Opener popup.window becomes null')
```

Yoav Weiss (@Shopify)

Added an await (with a 1 second timeout) and it seems flaky..

Arthur Sonzogni

Why are you talking about a timeout? Did you try `step_awaiŧ`?

Yoav Weiss (@Shopify)

I added `await t.step_wait(() => popup.window == null, 'Opener popup.window becomes null', 1000)`, and (so a timeout of 1 second) and I see it timing out occasionally

Arthur Sonzogni

And without a timeout?

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni
Yoav Weiss (@Shopify)

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yoav Weiss (@Shopify) (Gerrit)

unread,

Jul 2, 2024, 7:39:42 AM (23 hours ago) Jul 2

to Arthur Sonzogni, Arthur Sonzogni, Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni and Arthur Sonzogni

Yoav Weiss (@Shopify) added 1 comment

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/resources/noopener-helper.js

Line 35, Patchset 7: assert_equals(popup.window, null, 'Opener popup.window is null');

Yoav Weiss (@Shopify) . unresolved

I'm thinking of removing this check, as I'm not sure it's guaranteed to be true (without races). Thoughts?

Yoav Weiss (@Shopify)

@arthurs...@chromium.org confirmed that this is racy, so removed.

Arthur Sonzogni

What about `step_wait()` conditions?
```
await t.step_wait(() => popup.window == null, 'Opener popup.window becomes null')
```

Yoav Weiss (@Shopify)

Added an await (with a 1 second timeout) and it seems flaky..

Arthur Sonzogni

Why are you talking about a timeout? Did you try `step_awaiŧ`?

Yoav Weiss (@Shopify)

I added `await t.step_wait(() => popup.window == null, 'Opener popup.window becomes null', 1000)`, and (so a timeout of 1 second) and I see it timing out occasionally

Arthur Sonzogni

And without a timeout?

Yoav Weiss (@Shopify)

Trying now..

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni
Arthur Sonzogni

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yoav Weiss (@Shopify) (Gerrit)

unread,

Jul 2, 2024, 11:39:15 AM (19 hours ago) Jul 2

to Arthur Sonzogni, Arthur Sonzogni, Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni and Arthur Sonzogni

Yoav Weiss (@Shopify) added 1 comment

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/resources/noopener-helper.js

Line 35, Patchset 7: assert_equals(popup.window, null, 'Opener popup.window is null');

Yoav Weiss (@Shopify) . unresolved

I'm thinking of removing this check, as I'm not sure it's guaranteed to be true (without races). Thoughts?

Yoav Weiss (@Shopify)

@arthurs...@chromium.org confirmed that this is racy, so removed.

Arthur Sonzogni

What about `step_wait()` conditions?
```
await t.step_wait(() => popup.window == null, 'Opener popup.window becomes null')
```

Yoav Weiss (@Shopify)

Added an await (with a 1 second timeout) and it seems flaky..

Arthur Sonzogni

Why are you talking about a timeout? Did you try `step_awaiŧ`?

Yoav Weiss (@Shopify)

I added `await t.step_wait(() => popup.window == null, 'Opener popup.window becomes null', 1000)`, and (so a timeout of 1 second) and I see it timing out occasionally

Arthur Sonzogni

And without a timeout?

Yoav Weiss (@Shopify)

Trying now..

Yoav Weiss (@Shopify)

I think the entire test was timing out, so added a meta for a long timeout. I'm still not sure this would work on the webkit side of things though..

satisfied_requirement

unsatisfied_requirement

open

diffy

Arthur Sonzogni (Gerrit)

unread,

4:18 AM (2 hours ago) 4:18 AM

to Yoav Weiss (@Shopify), Arthur Sonzogni, Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni and Yoav Weiss (@Shopify)

Arthur Sonzogni voted and added 1 comment

Votes added by Arthur Sonzogni

Code-Review

+1

1 comment

File content/browser/security/coop/cross_origin_opener_policy_browsertest.cc

Line 8723, Patchset 4: NavigationVirtualBrowsingContextGroupNoopener) {

Arthur Sonzogni . unresolved

Could you duplicate this test, but use COOP-RO instead of COOP?

Yoav Weiss (@Shopify)

Would you like a completely separate test, or should I just add the RO cases to this one?

Arthur Sonzogni

It is up to you.

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni
Yoav Weiss (@Shopify)

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Arthur Sonzogni (Gerrit)

unread,

6:01 AM (22 minutes ago) 6:01 AM

to Yoav Weiss (@Shopify), Arthur Sonzogni, Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Yoav Weiss (@Shopify)

Arthur Sonzogni added 2 comments

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/resources/noopener-helper.js

Line 35, Patchset 7: assert_equals(popup.window, null, 'Opener popup.window is null');

Yoav Weiss (@Shopify) . resolved

I'm thinking of removing this check, as I'm not sure it's guaranteed to be true (without races). Thoughts?

Yoav Weiss (@Shopify)

@arthurs...@chromium.org confirmed that this is racy, so removed.

Arthur Sonzogni

What about `step_wait()` conditions?
```
await t.step_wait(() => popup.window == null, 'Opener popup.window becomes null')
```

Yoav Weiss (@Shopify)

Added an await (with a 1 second timeout) and it seems flaky..

Arthur Sonzogni

Why are you talking about a timeout? Did you try `step_awaiŧ`?

Yoav Weiss (@Shopify)

I added `await t.step_wait(() => popup.window == null, 'Opener popup.window becomes null', 1000)`, and (so a timeout of 1 second) and I see it timing out occasionally

Arthur Sonzogni

And without a timeout?

Yoav Weiss (@Shopify)

Trying now..

Yoav Weiss (@Shopify)

I think the entire test was timing out, so added a meta for a long timeout. I'm still not sure this would work on the webkit side of things though..

Arthur Sonzogni

ACK

Line 36, Patchset 16 (Latest): 'Opener popup.window becomes null', 1000)

Arthur Sonzogni . unresolved

What about omitting this and keep the default value? (3000)

I would like to avoid the test to fail, because the web browser is slow.

Open in Gerrit

Related details

Attention is currently required from:

Yoav Weiss (@Shopify)

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yoav Weiss (@Shopify) (Gerrit)

unread,

6:08 AM (15 minutes ago) 6:08 AM

to Arthur Sonzogni, Arthur Sonzogni, Vladimir Levin, Chromium LUCI CQ, Reilly Grant, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, kinuko...@chromium.org, network-ser...@chromium.org

Attention needed from Arthur Sonzogni

Yoav Weiss (@Shopify) voted and added 1 comment

Votes added by Yoav Weiss (@Shopify)

Commit-Queue

+1

1 comment

File third_party/blink/web_tests/external/wpt/html/cross-origin-opener-policy/resources/noopener-helper.js

Line 36, Patchset 16: 'Opener popup.window becomes null', 1000)

Arthur Sonzogni . resolved

What about omitting this and keep the default value? (3000)
I would like to avoid the test to fail, because the web browser is slow.

Yoav Weiss (@Shopify)

I didn't know there's a default. Done!

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

Add failing COOP noopener-allow-popups tests and flags [chromium/src : main]

Yoav Weiss (@Shopify) (Gerrit)

Yoav Weiss (@Shopify) added 1 comment

Related details

Reilly Grant (Gerrit)

Reilly Grant voted and added 1 comment

Votes added by Reilly Grant

1 comment

Related details

Arthur Sonzogni (Gerrit)

Arthur Sonzogni added 8 comments

Related details

Yoav Weiss (@Shopify) (Gerrit)

Yoav Weiss (@Shopify) added 7 comments

Related details

Vladimir Levin (Gerrit)

Vladimir Levin voted and added 1 comment

Votes added by Vladimir Levin

1 comment

Related details

Vladimir Levin (Gerrit)

Vladimir Levin voted and added 1 comment

Votes added by Vladimir Levin

1 comment

Related details

Arthur Sonzogni (Gerrit)

Arthur Sonzogni voted and added 13 comments

Votes added by Arthur Sonzogni

13 comments

Related details

Yoav Weiss (@Shopify) (Gerrit)

Yoav Weiss (@Shopify) added 5 comments

Related details

Yoav Weiss (@Shopify) (Gerrit)

Yoav Weiss (@Shopify) added 2 comments

Related details

Yoav Weiss (@Shopify) (Gerrit)

Yoav Weiss (@Shopify) added 3 comments

Yoav Weiss (@Shopify) (Gerrit)

Yoav Weiss (@Shopify) added 1 comment

Related details

Yoav Weiss (@Shopify) (Gerrit)

Yoav Weiss (@Shopify) added 1 comment

Yoav Weiss (@Shopify) (Gerrit)

Yoav Weiss (@Shopify) added 2 comments

Related details

Arthur Sonzogni (Gerrit)

Arthur Sonzogni added 1 comment

Related details

Yoav Weiss (@Shopify) (Gerrit)

Yoav Weiss (@Shopify) voted and added 1 comment

Votes added by Yoav Weiss (@Shopify)

1 comment

Related details

Arthur Sonzogni (Gerrit)

Arthur Sonzogni voted and added 1 comment

Votes added by Arthur Sonzogni

1 comment

Related details

Yoav Weiss (@Shopify) (Gerrit)

Yoav Weiss (@Shopify) added 1 comment

Related details

Arthur Sonzogni (Gerrit)

Arthur Sonzogni added 1 comment

Related details

Yoav Weiss (@Shopify) (Gerrit)

Yoav Weiss (@Shopify) added 1 comment

Related details

Arthur Sonzogni (Gerrit)

Arthur Sonzogni added 1 comment

Related details

Yoav Weiss (@Shopify) (Gerrit)

Yoav Weiss (@Shopify) added 1 comment

Related details

Yoav Weiss (@Shopify) (Gerrit)

Yoav Weiss (@Shopify) added 1 comment

Arthur Sonzogni (Gerrit)

Arthur Sonzogni voted and added 1 comment

Votes added by Arthur Sonzogni

1 comment