[bindings] always return value from Window self-referential getters [chromium/src : main]

0 views
Skip to first unread message

Caitlin Potter (Gerrit)

unread,
Jun 23, 2022, 4:52:51 PMJun 23
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Toon Verwaest, Yuki Shiino.

View Change

2 comments:

  • Patchset:

    • Patch Set #2:

      I've put together a sort of hack solution as a sort of stop-gap while I try to work on a proper fix, so I mention some considerations in comments. To be clear, I don't think this is the right solution for this problem, but it does at least "work".

      However, this doesn't address the issue with Reflect.get() mentioned on the bug.

      I'm happy to add additional test cases when there's an approach we're happy with.

  • File third_party/blink/renderer/bindings/core/v8/custom/v8_window_custom.cc:

    • Patch Set #2, Line 307: V8SetReturnValue(info, v8_window);

      So the difference here with the non-custom behaviour, is returning the original v8 wrapper instead of Null when the window is detached --- This is probably not the best way to solve this problem

      Other things I've considered:

      • `DOMWindow::<accessor>` only return `null` when in a pre-init state. When detached, they return themselves.
      • `DOMWindow::Wrap()` uses a wrapper derived from DOMWindow's own WindowProxyManager, which is presumably === to the v8_receiver (WindowProxyManager likely needs to provide a way to get at this wrapper?)

      This is probably cleaner than using Custom getters, in my humble opinion. However, I wanted to prototype something which "works" first before trying to change stuff like that.

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
Gerrit-Comment-Date: Thu, 23 Jun 2022 20:52:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Toon Verwaest (Gerrit)

unread,
Jun 24, 2022, 6:25:36 AMJun 24
to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Caitlin Potter, Yuki Shiino.

View Change

2 comments:

  • Patchset:

    • Patch Set #2:

      Looks good to me as a blink-side fix, it probably makes sense to make sure that blink agrees with the cached property. Perhaps in debug mode we should call the accessor (in v8) to verify?

  • File third_party/blink/renderer/bindings/core/v8/custom/v8_window_custom.cc:

    • So the difference here with the non-custom behaviour, is returning the original v8 wrapper instead o […]

      This particular problem I'd just fully fix in V8. We already have the cached property there, which is the right value. We can just always return that in the accessor; but we'll need to move it to InvokeApiFunction. That's better anyway imho.

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Comment-Date: Fri, 24 Jun 2022 10:25:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Caitlin Potter <ca...@igalia.com>
Gerrit-MessageType: comment

Yuki Shiino (Gerrit)

unread,
Jun 24, 2022, 12:20:26 PMJun 24
to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Caitlin Potter.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      I'm sorry that I wasn't able to have time for this patch. Let me take a look at this patch next week.

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
Gerrit-Comment-Date: Fri, 24 Jun 2022 16:20:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Yuki Shiino (Gerrit)

unread,
Jun 27, 2022, 5:05:21 AMJun 27
to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Caitlin Potter.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      First of all, thank you for the fix idea!

      I read through the bug report and this patch. I think I understand the problem and needs of a fix (of this long standing issue). Essentially, I should fix my todos here, I think:
      https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/dom_window.cc;drc=5dc9d4d65c721e36aa8733fe5b95fd4cea0741d5;l=59

      I understand that this patch will work, but custom bindings ([Custom]) shouldn't be used.  I now have an idea of the right implementation being inspired by your fix as below:
      ```
      v8::MaybeLocal<v8::Value> DOMWindow::Wrap(...) {
      if (frame) { // It's (still) attached!
      return frame->GetWindowProxy()->GlobalProxy();
      }
        // If no longer attached, use the saved/own one.
      return window_proxy_manager_->WindowProxy(world)->global_proxy_;
      }
      ```
      Although some more fixes would be needed around WindowProxy.

      What do you think? If you're fine, I'm happy to work on this.

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
Gerrit-Comment-Date: Mon, 27 Jun 2022 09:05:08 +0000

Caitlin Potter (Gerrit)

unread,
Jun 27, 2022, 11:47:16 AMJun 27
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Yuki Shiino.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      First of all, thank you for the fix idea! […]

      Yes, exactly. And I agree 100% Custom isn't what we want here.

      I'm happy to adopt that approach and add some test cases to verify it

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Comment-Date: Mon, 27 Jun 2022 15:47:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Yuki Shiino <yukis...@chromium.org>
Gerrit-MessageType: comment

Caitlin Potter (Gerrit)

unread,
Jun 27, 2022, 11:50:00 AMJun 27
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Yuki Shiino.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      Yes, exactly. And I agree 100% Custom isn't what we want here. […]

      However, I believe when Wrap is invoked, we won't actually be in DOMwindow::Wrap, unless there are also changes to the DOMWindow self-referential accessors

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Comment-Date: Mon, 27 Jun 2022 15:49:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Caitlin Potter <ca...@igalia.com>

Yuki Shiino (Gerrit)

unread,
Jun 27, 2022, 10:27:08 PMJun 27
to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Caitlin Potter.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      unless there are also changes to the DOMWindow self-referential accessors

      Right.

      And, now I think we can probably do the following.
      ```
      DOMWindow::Wrap(script_state) {
      return script_state->GetContext()->Global();
      }
      ```

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
Gerrit-Comment-Date: Tue, 28 Jun 2022 02:26:58 +0000

Caitlin Potter (Gerrit)

unread,
Jun 28, 2022, 9:13:09 AMJun 28
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Yuki Shiino.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      > unless there are also changes to the DOMWindow self-referential accessors […]

      https://codepen.io/caitp/pen/mdxdrxq illustrates a compat issue with this --- the line `Object.getOwnPropertyDescriptor(window, 'window').get.call(w).x` should get `x` from the iframe window and return 2, but in this case, the context is the outer window, so we return the global object associated with that.

      Unfortunately, when the window is detached, we don't have a way to get the global proxy out of WindowProxyManager. Would it make sense to give DOMWindow a weak reference to the v8 wrapper, and use that if the frame is detached?

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Comment-Date: Tue, 28 Jun 2022 13:13:00 +0000

Toon Verwaest (Gerrit)

unread,
Jun 28, 2022, 9:52:39 AMJun 28
to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Caitlin Potter, Yuki Shiino.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      https://codepen.io/caitp/pen/mdxdrxq illustrates a compat issue with this --- the line `Object. […]

      As I mentioned on the issue, we can, and will, fix this on the V8 side (too). That means we likely won't ever even call into blink anymore. Currently there's a performance issue (getOwnPropertyDescriptors breaks the optimisation) but also a secondary correctness fix (Reflect.get uses the lookup start instead of the receiver to return the value).

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Comment-Date: Tue, 28 Jun 2022 13:52:30 +0000

Yuki Shiino (Gerrit)

unread,
Jun 29, 2022, 4:27:27 AMJun 29
to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Caitlin Potter, Toon Verwaest.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      As I mentioned on the issue, we can, and will, fix this on the V8 side (too). […]

      My understanding is that a) the original issue is / will be fixed on the V8 side as Toon described, so no fix on the Blink side is necessary. However, b) it's nice to fix the Blink's implementation anyway (e.g. resolving the TODOs). If [CachedAccessor] works nicely, it would be an option to make DOMWindow::Wrap NOTREACHED() / CHECK(false).

      The ScriptState that DOMWindow::Wrap takes is the script state of the receiver object's creation context, so I think script_state->GetContext()->Global() must be the correct global proxy to be returned regardless of whether the window is attached or detached.

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
Gerrit-Comment-Date: Wed, 29 Jun 2022 08:27:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Caitlin Potter <ca...@igalia.com>
Comment-In-Reply-To: Yuki Shiino <yukis...@chromium.org>
Comment-In-Reply-To: Toon Verwaest <verw...@chromium.org>
Gerrit-MessageType: comment

Caitlin Potter (Gerrit)

unread,
Jun 29, 2022, 6:27:49 AMJun 29
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Toon Verwaest, Yuki Shiino.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      My understanding is that a) the original issue is / will be fixed on the V8 side as Toon described, […]

      It would be the window of the context of the getter, not the window of the receiver (which is why this line returns the incorrect result) -- but it's true that it's a moot point if fixed on the v8 side

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
Gerrit-Comment-Date: Wed, 29 Jun 2022 10:27:32 +0000

Toon Verwaest (Gerrit)

unread,
Jun 29, 2022, 7:31:10 AMJun 29
to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Caitlin Potter, Yuki Shiino.

View Change

1 comment:

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Comment-Date: Wed, 29 Jun 2022 11:30:59 +0000

Caitlin Potter (Gerrit)

unread,
Jun 29, 2022, 7:39:37 AMJun 29
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Toon Verwaest, Yuki Shiino.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      That's not what the spec says though: "The window, frames, and self getter steps are to return this' […]

      In practice, other browsers return the receiver (and throw if the receiver isn't a global object)

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
Gerrit-Comment-Date: Wed, 29 Jun 2022 11:39:21 +0000

Toon Verwaest (Gerrit)

unread,
Jun 29, 2022, 7:56:56 AMJun 29
to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Caitlin Potter, Yuki Shiino.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      In practice, other browsers return the receiver (and throw if the receiver isn't a global object)

      That's indeed what the spec says? I at least read your comment differently. Anyway, this is what's supposed to happen (global_from_realm2 is imho the "receiver" here): `getter_from_realm1.call(global_from_realm2)` will return `global_from_realm2`, not `global_from_realm1`. V8 currently does that wrong (partially):

      ```
      console.log(Reflect.get(this, "window", this[0]) === this[0]); // false (v8 is wrong)
      Object.getOwnPropertyDescriptor(window, "window"); // materializes the JSFunction
      console.log(Reflect.get(this, "window", this[0]) === this[0]); // true (blink is right)```

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Comment-Date: Wed, 29 Jun 2022 11:56:43 +0000

Caitlin Potter (Gerrit)

unread,
Jun 29, 2022, 8:10:00 AMJun 29
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Toon Verwaest, Yuki Shiino.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      That's indeed what the spec says? I at least read your comment differently. […]

      Blink is right while the window is attached (it returns the global proxy associated with the window proxy for the receiver) -- but if we change Wrap to return the global from the ScriptState's context, we'll end up with the global associated with the getter rather than the receiver, which can be wrong in the case I posted above

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
Gerrit-Comment-Date: Wed, 29 Jun 2022 12:09:49 +0000

Caitlin Potter (Gerrit)

unread,
Jun 29, 2022, 8:16:54 AMJun 29
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Toon Verwaest, Yuki Shiino.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      Blink is right while the window is attached (it returns the global proxy associated with the window […]

      What I'm getting at is, DOMWindow needs to hang onto a reference to a v8 wrapper for itself, it can't depend on Frame as it needs to work when detached also, and using the global from context from ScriptState in DOMWindow::Wrap() will produce incorrect results

      so, I think WindowProxyManager can sort of do this, but currently it looks like it drops the wrapper when detached, so it isn't strictly ideal (I assume there's no real need for it to release when detached, just during navigation?)

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
Gerrit-Comment-Date: Wed, 29 Jun 2022 12:16:42 +0000

Toon Verwaest (Gerrit)

unread,
Jun 29, 2022, 8:21:09 AMJun 29
to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Caitlin Potter, Yuki Shiino.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      What I'm getting at is, DOMWindow needs to hang onto a reference to a v8 wrapper for itself, it can' […]

      What I'm confused about is: `window` just always returns the `this` if it's a DOMWindow. Why don't we?

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Comment-Date: Wed, 29 Jun 2022 12:20:58 +0000

Caitlin Potter (Gerrit)

unread,
Jun 29, 2022, 8:33:39 AMJun 29
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Toon Verwaest, Yuki Shiino.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      What I'm confused about is: `window` just always returns the `this` if it's a DOMWindow. […]

      currently, we return the DOMWindow from the Frame's WindowProxy. Are there cases where that doesn't return `this`? I assume this relates to the way global proxies are swapped during navigation, but I'm not positive. I expect there must be cases where it makes a difference, but people with more DOM knowledge should comment on that.

      the issue is currently when the window is detached, this approach does the wrong thing (no way to get at the v8 wrapper), and using ScriptState's context global would also be wrong when the receiver is not the object the accessors installed on.

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
Gerrit-Comment-Date: Wed, 29 Jun 2022 12:33:21 +0000

Toon Verwaest (Gerrit)

unread,
Jun 29, 2022, 8:53:42 AMJun 29
to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Caitlin Potter, Yuki Shiino.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      currently, we return the DOMWindow from the Frame's WindowProxy. […]

      Because of the way to the split object model works, I don't think `V8SetReturnValue(info, info.This())` is ever wrong (assuming we've checked that that's a DOMWindow)

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Comment-Date: Wed, 29 Jun 2022 12:53:29 +0000

Caitlin Potter (Gerrit)

unread,
Jun 29, 2022, 9:18:30 AMJun 29
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Toon Verwaest, Yuki Shiino.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      Because of the way to the split object model works, I don't think `V8SetReturnValue(info, info. […]

      But, to do that, we still need either [Custom] or a new extended attribute to tell the bindings generator to do this, as far as I can tell.

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
Gerrit-Comment-Date: Wed, 29 Jun 2022 13:18:16 +0000

Yuki Shiino (Gerrit)

unread,
Jun 30, 2022, 1:37:44 AMJun 30
to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Caitlin Potter, Toon Verwaest.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      But, to do that, we still need either [Custom] or a new extended attribute to tell the bindings gene […]

      First of all, I'm sorry that I was wrong about the context of the ScriptState argument of DOMWindow::Wrap. Caitlin is correct.


      Question to Toon:
      Toon wrote: Because of the way to the split object model works, ...
      Did you mean by "the split object model" that, when Blink returns a global object, V8 automatically converts the global object to the global proxy? Am I understanding correctly?


      Then, my recommendation is now the following:
      ```
      // window.idl
      [Replaceable, CrossOrigin, CachedAccessor=kWindowProxy,
      CallWith=ThisValue, ImplementedAs=selfForBinding] readonly attribute Window self;
      // dom_window.cc
      ScripValue DOMWindow::selfForBinding(ScriptValue this_value) const {
      return this_value;
      }

      v8::Local<v8::Value> DOMWindow::Wrap() { CHECK(false); }
      ```

      We'd need some more minor tweaks, but I think the above idea will work. [CallWith=ThisValue] is what Toon suggested, and [ImplementedAs=selfForBinding] for keeping the backward compatibility of DOMWindow::self().


      Or, if things will be much simpler with [Custom], then [Custom] is an option, I think.

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
Gerrit-Comment-Date: Thu, 30 Jun 2022 05:37:36 +0000

Toon Verwaest (Gerrit)

unread,
Jun 30, 2022, 4:56:37 AMJun 30
to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Caitlin Potter, Yuki Shiino.

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      First of all, I'm sorry that I was wrong about the context of the ScriptState argument of DOMWindow: […]

      What I meant is that we convert the global object to the global proxy _before_ we call into blink. So This() is already the global proxy of the context (realm) that we're trying to read the global proxy of, independent of whether that global proxy is still attached to that context (realm).

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 2
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Comment-Date: Thu, 30 Jun 2022 08:56:23 +0000

Caitlin Potter (Gerrit)

unread,
Jul 1, 2022, 11:09:57 PMJul 1
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Yuki Shiino.

Patch set 3:Commit-Queue +1

View Change

2 comments:

  • File third_party/blink/renderer/platform/bindings/v8_set_return_value.h:

    • Patch Set #3, Line 398: if (value->IsObject()) {

      not clear that any of this is really needed, pretty sure the wrapper from the cache should always be the same as `value`, but I thought just in case I'd leave it here.

  • File third_party/blink/web_tests/fast/js/window-self-referential.html:

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 3
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Comment-Date: Sat, 02 Jul 2022 03:09:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Caitlin Potter (Gerrit)

unread,
Jul 2, 2022, 7:13:16 AMJul 2
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Toon Verwaest, Yuki Shiino.

Patch set 5:Commit-Queue +1

View Change

1 comment:

  • Patchset:

    • Patch Set #2:

      What I meant is that we convert the global object to the global proxy _before_ we call into blink. […]

      So basically, the `DOMDataStore::SetReturnValueFast(info.GetReturnValue(), wrappable, V8ReturnValue::CreationContext(info), receiver))` is not needed, and hasn't actually been needed upstream for some time?

      If that's the case, I should be able to remove those lines safely and replace DOMWindow::Wrap() with a CHECK(false); as suggested by Yuki Shiino

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 5
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
Gerrit-Comment-Date: Sat, 02 Jul 2022 11:13:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes

Caitlin Potter (Gerrit)

unread,
Jul 2, 2022, 12:10:15 PMJul 2
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Toon Verwaest, Yuki Shiino.

Patch set 6:Commit-Queue +1

View Change

1 comment:

  • Patchset:

    • Patch Set #6:

      Okay, everything should be ready to go. We just return `ThisValue`. I am keeping the CrossOrigin IDL attribute, in order to get the cross origin access reports and stuff. Keeping it required a new implementation of V8SetReturnValue for v8::Local<v8::Value>, so I've included a DCHECK to assert that it's === to the receiver, and a note.

      Between that and the test, it seems ready to go.

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 6
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
Gerrit-Comment-Date: Sat, 02 Jul 2022 16:10:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Yuki Shiino (Gerrit)

unread,
Jul 4, 2022, 7:33:06 AMJul 4
to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Caitlin Potter, Toon Verwaest.

View Change

7 comments:

  • File third_party/blink/renderer/bindings/core/v8/custom/v8_window_custom.cc:

  • File third_party/blink/renderer/core/frame/dom_window.cc:

    • Patch Set #6, Line 122: WebFeature::kWindowProxyCrossOriginAccessFromOtherPageSelf);

      Should we (re)move this recording out of here?

      c.f. https://chromium-review.googlesource.com/c/chromium/src/+/3398912
      The goal is to record (possibly) cross origin access, then it should be meaningless to record it here.

    • Patch Set #6, Line 153: return receiver.V8Value();

      I confirmed that this works fine locally, but I think it's better to explicitly return the global proxy rather than something that could be the global object (by accident or by a future bug). Because of [CachedAccessor], we don't need to care much about the performance, let's return the global proxy explicitly.

      ```
      v8::Local<v8::Object> v8_receiver = receiver.V8Value().As<v8::Object>();
      v8::Local<v8::Context> v8_context = v8_receiver->GetCreationContext().ToLocalChecked();
      return v8_context->Global();
      ```

      Then, there must be no chance to return the global object instead of the global proxy.

      Ditto for others.

  • File third_party/blink/renderer/platform/bindings/v8_set_return_value.h:

    • Patch Set #6, Line 394: v8::Local<v8::Value>&

      nit: s/v8::Local<...>&/v8::Local<...>/

      v8::Local is essentially a pointer (to pointer), so no need of `&`.

    • Patch Set #6, Line 400: v8::Local<v8::Object>::Cast(value)

      nit: value.As<v8::Object>() is preferred.

    • Patch Set #6, Line 400: DCHECK_EQ(ToScriptWrappable(v8::Local<v8::Object>::Cast(value)), receiver);

      I think we don't need this DCHECK_EQ. It's okay that the page navigated away and the global proxy no longer points to this DOMWindow instance. It's a possible case and normal.

      Thank you for having this check in your mind (being very careful), but I think we don't need this check in this case.

  • File third_party/blink/web_tests/fast/js/window-self-referential.html:

    • Patch Set #6, Line 1: <!DOCTYPE html>

      It's really much nicer if we could turn this test into a WPT test. IIUC, nothing is Blink-specific here, so it must be possible to make this into part of WPTs.

      IIUC, it's okay (or even welcome) to put corner case tests (as part of bug fixes) in WPTs and let other user agents pass them.

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 6
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
Gerrit-Comment-Date: Mon, 04 Jul 2022 11:32:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Caitlin Potter (Gerrit)

unread,
Jul 4, 2022, 10:01:59 AMJul 4
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Toon Verwaest, Yuki Shiino.

View Change

8 comments:

  • Patchset:

    • Patch Set #6:

      most of this has been taken care of, just wanted to clarify a bit about the "everything here should be same-origin" point, because based on my read, we aren't invoking the v8 API in a way which would permit this

  • File third_party/blink/renderer/bindings/core/v8/custom/v8_window_custom.cc:

    • Done

  • File third_party/blink/renderer/core/frame/dom_window.cc:

    • I confirmed that this works fine locally, but I think it's better to explicitly return the global pr […]

      Done

  • File third_party/blink/renderer/platform/bindings/v8_set_return_value.h:

    • nit: s/v8::Local<...>&/v8::Local<...>/ […]

      Done

    • removed the DCHECK, so type conversion no longer needed (parameter is now a Local<Object> already anyways)

    • I think we don't need this DCHECK_EQ. […]

      Done

  • File third_party/blink/web_tests/fast/js/window-self-referential.html:

    • It's really much nicer if we could turn this test into a WPT test. […]

      Ack

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 6
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
Gerrit-Comment-Date: Mon, 04 Jul 2022 14:01:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No

Yuki Shiino (Gerrit)

unread,
Jul 5, 2022, 3:36:26 AMJul 5
to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Caitlin Potter, Toon Verwaest.

View Change

1 comment:

  • Patchset:

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 6
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
Gerrit-Comment-Date: Tue, 05 Jul 2022 07:36:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Caitlin Potter (Gerrit)

unread,
Jul 5, 2022, 8:23:17 AMJul 5
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Toon Verwaest, Yuki Shiino.

View Change

2 comments:

  • Patchset:

    • I wanted to have my question addressed first, hope thats ok

  • File third_party/blink/renderer/core/frame/dom_window.cc:

    • I believe right now, we aren't guaranteed to have a same-origin object in these methods by v8. […]

      I wanted to get an answer to this first, before uploading

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 6
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
Gerrit-Comment-Date: Tue, 05 Jul 2022 12:22:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Caitlin Potter <ca...@igalia.com>

Yuki Shiino (Gerrit)

unread,
Jul 5, 2022, 8:57:18 AMJul 5
to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Caitlin Potter, Toon Verwaest.

View Change

2 comments:

  • Patchset:

    • Patch Set #6:

      I'm sorry that I didn't realize that you're waiting my answer.

  • File third_party/blink/renderer/core/frame/dom_window.cc:

    • I wanted to get an answer to this first, before uploading

      Yes, these IDL attributes are annotated with [CrossOrigin] and it's possible to be accessed across origins. IIUC, RecordWindowProxyAccessMetrics records "possibly cross origin access". It's fine that it's not guaranteed to have a same origin object.

      Combined with [CachedAccessor], this function is (mostly) not called in the case of the same origin-domain. So, RecordWindowProxyAccessMetrics looks rather expecting a cross origin access.

      Does this make sense?

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 6
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
Gerrit-Comment-Date: Tue, 05 Jul 2022 12:57:07 +0000

Caitlin Potter (Gerrit)

unread,
Jul 5, 2022, 9:21:18 AMJul 5
to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Toon Verwaest, Yuki Shiino.

View Change

1 comment:

  • File third_party/blink/renderer/core/frame/dom_window.cc:

    • Yes, these IDL attributes are annotated with [CrossOrigin] and it's possible to be accessed across o […]

      I mean, I think we still want to increase the use count, and the RecordWindowProxyAccessMetrics() stuff will do the bail out if it isn't needed, so it seems good to keep it, I think we'd only remove it if we were guaranteed to be same-origin, in which case it wouldn't actually do anything helpful

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 6
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
Gerrit-Comment-Date: Tue, 05 Jul 2022 13:21:07 +0000

Yuki Shiino (Gerrit)

unread,
Jul 5, 2022, 9:32:29 AMJul 5
to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Camille Lamy, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Caitlin Potter, Camille Lamy, Toon Verwaest.

View Change

2 comments:

  • Patchset:

    • Patch Set #6:

      +R=clamy@, would you answer the following question about whether we'd like to keep calling RecordWindowProxyAccessMetrics even when the function will never be called from author JS code?

  • File third_party/blink/renderer/core/frame/dom_window.cc:

    • I mean, I think we still want to increase the use count, and the RecordWindowProxyAccessMetrics() st […]

      clamy@ should be the best person to answer the question.

      As we introduce selfForBinding() as JS exposed `self`, DOMWindow::self() will never be called directly from JS. It's still possible that Blink internal implementation calls DOMWindow::self(), but not author JS code.

      Do we want to keep this recording here?

To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
Gerrit-Change-Number: 3715140
Gerrit-PatchSet: 6
Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
Gerrit-Reviewer: Camille Lamy <cl...@chromium.org>
Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Camille Lamy <cl...@chromium.org>
Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
Gerrit-Comment-Date: Tue, 05 Jul 2022 13:32:15 +0000

Blink W3C Test Autoroller (Gerrit)

unread,
Jul 5, 2022, 12:49:27 PMJul 5
to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Camille Lamy, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Caitlin Potter, Camille Lamy, Toon Verwaest.

Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/34709.

When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.

WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process

View Change

    To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
    Gerrit-Change-Number: 3715140
    Gerrit-PatchSet: 7
    Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Camille Lamy <cl...@chromium.org>
    Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
    Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: Kentaro Hara <har...@chromium.org>
    Gerrit-Attention: Camille Lamy <cl...@chromium.org>
    Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
    Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
    Gerrit-Comment-Date: Tue, 05 Jul 2022 16:49:10 +0000
    Gerrit-HasComments: No
    Gerrit-Has-Labels: No
    Gerrit-MessageType: comment

    Camille Lamy (Gerrit)

    unread,
    Jul 7, 2022, 8:58:29 AMJul 7
    to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Blink W3C Test Autoroller, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

    Attention is currently required from: Caitlin Potter, Toon Verwaest, Yuki Shiino.

    View Change

    1 comment:

    • File third_party/blink/renderer/core/frame/dom_window.cc:

      • clamy@ should be the best person to answer the question. […]

        We're tracking these metrics to check which websites would break if we were to remove cross-origin accesses to these properties. So I think having it just in bindings should be fine?

    To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
    Gerrit-Change-Number: 3715140
    Gerrit-PatchSet: 7
    Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Camille Lamy <cl...@chromium.org>
    Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
    Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: Kentaro Hara <har...@chromium.org>
    Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
    Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
    Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
    Gerrit-Comment-Date: Thu, 07 Jul 2022 12:58:10 +0000

    Caitlin Potter (Gerrit)

    unread,
    Jul 7, 2022, 9:16:47 AMJul 7
    to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Blink W3C Test Autoroller, Camille Lamy, Yuki Shiino, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

    Attention is currently required from: Camille Lamy, Toon Verwaest, Yuki Shiino.

    View Change

    1 comment:

    • File third_party/blink/renderer/core/frame/dom_window.cc:

      • We're tracking these metrics to check which websites would break if we were to remove cross-origin a […]

        Done (I originally misunderstood this comment as being about removing the recording from the ForBindings methods, my apologies for making this take so long!)

    To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
    Gerrit-Change-Number: 3715140
    Gerrit-PatchSet: 7
    Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Camille Lamy <cl...@chromium.org>
    Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
    Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: Kentaro Hara <har...@chromium.org>
    Gerrit-Attention: Camille Lamy <cl...@chromium.org>
    Gerrit-Attention: Yuki Shiino <yukis...@chromium.org>
    Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
    Gerrit-Comment-Date: Thu, 07 Jul 2022 13:16:36 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Camille Lamy <cl...@chromium.org>

    Yuki Shiino (Gerrit)

    unread,
    Jul 7, 2022, 9:42:38 AMJul 7
    to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Blink W3C Test Autoroller, Camille Lamy, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara

    Attention is currently required from: Caitlin Potter, Camille Lamy, Toon Verwaest.

    Patch set 8:Code-Review +1

    View Change

    1 comment:

    • Patchset:

    To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
    Gerrit-Change-Number: 3715140
    Gerrit-PatchSet: 8
    Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Camille Lamy <cl...@chromium.org>
    Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
    Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: Kentaro Hara <har...@chromium.org>
    Gerrit-Attention: Camille Lamy <cl...@chromium.org>
    Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
    Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
    Gerrit-Comment-Date: Thu, 07 Jul 2022 13:42:26 +0000

    Yuki Shiino (Gerrit)

    unread,
    Jul 7, 2022, 9:44:01 AMJul 7
    to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Kentaro Hara, Yuki Shiino, Blink W3C Test Autoroller, Camille Lamy, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org

    Attention is currently required from: Caitlin Potter, Camille Lamy, Kentaro Hara, Toon Verwaest.

    View Change

    1 comment:

    • Patchset:

    To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
    Gerrit-Change-Number: 3715140
    Gerrit-PatchSet: 8
    Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Camille Lamy <cl...@chromium.org>
    Gerrit-Reviewer: Kentaro Hara <har...@chromium.org>
    Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
    Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-Attention: Kentaro Hara <har...@chromium.org>
    Gerrit-Attention: Camille Lamy <cl...@chromium.org>
    Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
    Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
    Gerrit-Comment-Date: Thu, 07 Jul 2022 13:43:51 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Gerrit-MessageType: comment

    Kentaro Hara (Gerrit)

    unread,
    Jul 10, 2022, 8:15:45 PMJul 10
    to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Yuki Shiino, Blink W3C Test Autoroller, Camille Lamy, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org

    Attention is currently required from: Caitlin Potter, Camille Lamy, Toon Verwaest.

    Patch set 8:Code-Review +1

    View Change

    1 comment:

    To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
    Gerrit-Change-Number: 3715140
    Gerrit-PatchSet: 8
    Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Camille Lamy <cl...@chromium.org>
    Gerrit-Reviewer: Kentaro Hara <har...@chromium.org>
    Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
    Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-Attention: Camille Lamy <cl...@chromium.org>
    Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
    Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
    Gerrit-Comment-Date: Mon, 11 Jul 2022 00:15:34 +0000

    Caitlin Potter (Gerrit)

    unread,
    Jul 11, 2022, 7:38:51 AMJul 11
    to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Kentaro Hara, Yuki Shiino, Blink W3C Test Autoroller, Camille Lamy, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org

    Attention is currently required from: Camille Lamy, Toon Verwaest.

    Patch set 8:Commit-Queue +2

    View Change

    1 comment:

    To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
    Gerrit-Change-Number: 3715140
    Gerrit-PatchSet: 8
    Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Camille Lamy <cl...@chromium.org>
    Gerrit-Reviewer: Kentaro Hara <har...@chromium.org>
    Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
    Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-Attention: Camille Lamy <cl...@chromium.org>
    Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
    Gerrit-Comment-Date: Mon, 11 Jul 2022 11:38:41 +0000

    Caitlin Potter (Gerrit)

    unread,
    Jul 11, 2022, 8:58:44 AMJul 11
    to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Kentaro Hara, Yuki Shiino, Blink W3C Test Autoroller, Camille Lamy, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org

    Attention is currently required from: Camille Lamy, Toon Verwaest.

    View Change

    1 comment:

    • Patchset:

      • Patch Set #8:

        hmm, seems like I can't depend on either casting the receiver to an Object, or casting the creation context to an object, in all cases.

        I think we avoid this by just returning the receiver when we don't have a creation context, or if preferred, could return null. I think returning the receiver should be fine since the bindings layer has already done the type check and stuff.

    To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
    Gerrit-Change-Number: 3715140
    Gerrit-PatchSet: 8
    Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Camille Lamy <cl...@chromium.org>
    Gerrit-Reviewer: Kentaro Hara <har...@chromium.org>
    Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
    Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-Attention: Camille Lamy <cl...@chromium.org>
    Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
    Gerrit-Comment-Date: Mon, 11 Jul 2022 12:58:33 +0000

    Caitlin Potter (Gerrit)

    unread,
    Jul 11, 2022, 10:27:14 AMJul 11
    to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Kentaro Hara, Yuki Shiino, Blink W3C Test Autoroller, Camille Lamy, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org

    Attention is currently required from: Camille Lamy, Toon Verwaest.

    View Change

    1 comment:

    • File third_party/blink/renderer/core/frame/dom_window.cc:

      • Patch Set #6, Line 153: return receiver.V8Value();

        Done

        So as seen when setting CQ=+2, this causes some issues in unit tests. My immediate fix is to do this when GetCreationContext() returns a non-empty MaybeLocal, and otherwise return the receiver.

        However, I think in practice, we're fine to always return the receiver, as the type check has verified that the wrapper is the correct type, and this could avoid the testing of GetCreationContext.

        One alternative is returning v8::Null() or using a MaybeHandle<Object> or something, and using SetNull() if the MaybeHandle is empty, whatever -- the point is there's a bit of plumbing depending on preferences of reviewers, but the most straightforward thing is just returning the receiver.

        The newest patchiest still returns GetCreationContext()->Global() when the creation context is available, and otherwise returns the receiver, which should be equivalent anyways.

    To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
    Gerrit-Change-Number: 3715140
    Gerrit-PatchSet: 8
    Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Camille Lamy <cl...@chromium.org>
    Gerrit-Reviewer: Kentaro Hara <har...@chromium.org>
    Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
    Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-Attention: Camille Lamy <cl...@chromium.org>
    Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
    Gerrit-Comment-Date: Mon, 11 Jul 2022 14:27:03 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No

    Caitlin Potter (Gerrit)

    unread,
    Jul 11, 2022, 10:30:42 AMJul 11
    to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Kentaro Hara, Yuki Shiino, Blink W3C Test Autoroller, Camille Lamy, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org

    Attention is currently required from: Camille Lamy, Toon Verwaest.

    Patch set 9:Commit-Queue +2

    View Change

    1 comment:

    • Patchset:

      • Patch Set #9:

        I'm going to CQ+2 this to avoid wasting too much time, as this is conceptually equivalent to the previous iteration, just a bit safer in this case with the unit test.

        But it might be worth changing how this is handled in a followup?

    To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
    Gerrit-Change-Number: 3715140
    Gerrit-PatchSet: 9
    Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Camille Lamy <cl...@chromium.org>
    Gerrit-Reviewer: Kentaro Hara <har...@chromium.org>
    Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
    Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-Attention: Camille Lamy <cl...@chromium.org>
    Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
    Gerrit-Comment-Date: Mon, 11 Jul 2022 14:30:32 +0000

    Yuki Shiino (Gerrit)

    unread,
    Jul 11, 2022, 11:34:23 AMJul 11
    to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Kentaro Hara, Yuki Shiino, Blink W3C Test Autoroller, Camille Lamy, Toon Verwaest, Chromium LUCI CQ, chromium...@chromium.org

    Attention is currently required from: Caitlin Potter, Camille Lamy, Toon Verwaest.

    Patch set 9:Code-Review +1

    View Change

    2 comments:

    • Patchset:

      • Patch Set #9:

        I'm going to CQ+2 this to avoid wasting too much time, as this is conceptually equivalent to the pre […]

        This time, this change is fine (because a remote global proxy doesn't have a global object and it's guaranteed to be a global proxy, plus, there is no other cases that GetCreationContext fails), however, I'd highly recommend to wait for reviews from experts especially when you're changing the security sensitive code like this case. Breaking security is much much worse than crashing, so we shouldn't be in hurry when touching security sensitive code.

      • Patch Set #9:

        LGTM.

    To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
    Gerrit-Change-Number: 3715140
    Gerrit-PatchSet: 9
    Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Camille Lamy <cl...@chromium.org>
    Gerrit-Reviewer: Kentaro Hara <har...@chromium.org>
    Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
    Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-Attention: Camille Lamy <cl...@chromium.org>
    Gerrit-Attention: Caitlin Potter <ca...@igalia.com>
    Gerrit-Attention: Toon Verwaest <verw...@chromium.org>
    Gerrit-Comment-Date: Mon, 11 Jul 2022 15:34:12 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    Comment-In-Reply-To: Caitlin Potter <ca...@igalia.com>
    Gerrit-MessageType: comment

    Chromium LUCI CQ (Gerrit)

    unread,
    Jul 11, 2022, 11:57:27 AMJul 11
    to Caitlin Potter, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Kentaro Hara, Yuki Shiino, Blink W3C Test Autoroller, Camille Lamy, Toon Verwaest, chromium...@chromium.org

    Chromium LUCI CQ submitted this change.

    View Change


    Approvals: Yuki Shiino: Looks good to me Kentaro Hara: Looks good to me Caitlin Potter: Commit
    [bindings] always return value from Window self-referential getters

    For whatever reason, some operations seem to invalidate the CacheAccessor unnecessarily. Rather than marking a bunch of operations as being side-effect free incorrectly, or finding another work around, it seemed the simplest solution was to simply return the v8 wrapper from the accessor. To achieve this, the wrapper object the method is invoked on is returned. The [CrossOrigin] IDL attribute remains, and the previous method of looking up the correct wrapper for a window in a detached frame is still used.

    BUG=1305302

    Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
    Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3715140
    Commit-Queue: Caitlin Potter <ca...@igalia.com>
    Reviewed-by: Yuki Shiino <yukis...@chromium.org>
    Reviewed-by: Kentaro Hara <har...@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#1022719}
    ---
    M third_party/blink/renderer/core/frame/dom_window.cc
    M third_party/blink/renderer/core/frame/dom_window.h
    M third_party/blink/renderer/core/frame/window.idl
    M third_party/blink/renderer/platform/bindings/v8_set_return_value.h
    M third_party/blink/web_tests/external/wpt/html/browsers/the-window-object/window-properties.https.html
    5 files changed, 105 insertions(+), 12 deletions(-)


    To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I2fb0f0fb9d59511debe9e009a6f3194b48ca29b5
    Gerrit-Change-Number: 3715140
    Gerrit-PatchSet: 10
    Gerrit-Owner: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Caitlin Potter <ca...@igalia.com>
    Gerrit-Reviewer: Camille Lamy <cl...@chromium.org>
    Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
    Gerrit-Reviewer: Kentaro Hara <har...@chromium.org>
    Gerrit-Reviewer: Toon Verwaest <verw...@chromium.org>
    Gerrit-Reviewer: Yuki Shiino <yukis...@chromium.org>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-MessageType: merged