[bindings] always return value from Window self-referential getters [chromium/src : main]
Caitlin Potter (Gerrit)
Attention is currently required from: Toon Verwaest, Yuki Shiino.
2 comments:
Patchset:
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.
Toon Verwaest (Gerrit)
Attention is currently required from: Caitlin Potter, Yuki Shiino.
2 comments:
Patchset:
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:
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 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.
Yuki Shiino (Gerrit)
Attention is currently required from: Caitlin Potter.
1 comment:
Patchset:
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.
Yuki Shiino (Gerrit)
Patchset:
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=59I 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.
Caitlin Potter (Gerrit)
Attention is currently required from: Yuki Shiino.
1 comment:
Patchset:
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.
Caitlin Potter (Gerrit)
Patchset:
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.
Yuki Shiino (Gerrit)
Attention is currently required from: Caitlin Potter.
1 comment:
Patchset:
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.
Caitlin Potter (Gerrit)
Attention is currently required from: Yuki Shiino.
1 comment:
Patchset:
> 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.
Toon Verwaest (Gerrit)
Attention is currently required from: Caitlin Potter, Yuki Shiino.
1 comment:
Patchset:
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.
Yuki Shiino (Gerrit)
Attention is currently required from: Caitlin Potter, Toon Verwaest.
1 comment:
Patchset:
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.
Caitlin Potter (Gerrit)
Attention is currently required from: Toon Verwaest, Yuki Shiino.
1 comment:
Patchset:
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.
Toon Verwaest (Gerrit)
Attention is currently required from: Caitlin Potter, Yuki Shiino.
1 comment:
Patchset:
It would be the window of the context of the getter, not the window of the receiver (which is why th […]
That's not what the spec says though: "The window, frames, and self getter steps are to return this's relevant Realm.[[GlobalEnv]].[[GlobalThisValue]]." (https://html.spec.whatwg.org/multipage/window-object.html#dom-window)
To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.
Caitlin Potter (Gerrit)
Attention is currently required from: Toon Verwaest, Yuki Shiino.
1 comment:
Patchset:
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.
Toon Verwaest (Gerrit)
Attention is currently required from: Caitlin Potter, Yuki Shiino.
1 comment:
Patchset:
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.
Caitlin Potter (Gerrit)
Attention is currently required from: Toon Verwaest, Yuki Shiino.
1 comment:
Patchset:
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.
Caitlin Potter (Gerrit)
Patchset:
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.
Toon Verwaest (Gerrit)
Attention is currently required from: Caitlin Potter, Yuki Shiino.
1 comment:
Patchset:
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.
Caitlin Potter (Gerrit)
Attention is currently required from: Toon Verwaest, Yuki Shiino.
1 comment:
Patchset:
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.
Toon Verwaest (Gerrit)
Attention is currently required from: Caitlin Potter, Yuki Shiino.
1 comment:
Patchset:
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.
Caitlin Potter (Gerrit)
Attention is currently required from: Toon Verwaest, Yuki Shiino.
1 comment:
Patchset:
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.
Yuki Shiino (Gerrit)
Attention is currently required from: Caitlin Potter, Toon Verwaest.
1 comment:
Patchset:
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.
Toon Verwaest (Gerrit)
Attention is currently required from: Caitlin Potter, Yuki Shiino.
1 comment:
Patchset:
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.
Caitlin Potter (Gerrit)
Attention is currently required from: Yuki Shiino.
Patch set 3:Commit-Queue +1
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:
Patch Set #3, Line 31:
"Object.getOwnPropertyDescriptor(%1, '%2').get.call(%1)",I think this is the only way to bypass the cached accessor, currently (testbed for it: https://codepen.io/caitp/pen/MWVwjjX)
To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.
Caitlin Potter (Gerrit)
Attention is currently required from: Toon Verwaest, Yuki Shiino.
Patch set 5:Commit-Queue +1
1 comment:
Patchset:
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.
Caitlin Potter (Gerrit)
Attention is currently required from: Toon Verwaest, Yuki Shiino.
Patch set 6:Commit-Queue +1
1 comment:
Patchset:
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.
Yuki Shiino (Gerrit)
Attention is currently required from: Caitlin Potter, Toon Verwaest.
7 comments:
File third_party/blink/renderer/bindings/core/v8/custom/v8_window_custom.cc:
nit: This empty line shouldn't be removed.
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.
Caitlin Potter (Gerrit)
Attention is currently required from: Toon Verwaest, Yuki Shiino.
8 comments:
Patchset:
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:
nit: This empty line shouldn't be removed.
Done
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? […]
I believe right now, we aren't guaranteed to have a same-origin object in these methods by v8. [CrossOrigin] getters without specifying CrossOrigin=Setter will set SetAcceptAnyReceiver(true) on the function template for these accessors (per https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/scripts/bind_gen/interface.py;l=4871?q=_make_property_entry_cross_origin_check&ss=chromium)
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 pr […]
Done
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<...>/ […]
Done
Patch Set #6, Line 400:
v8::Local<v8::Object>::Cast(value)nit: value.As<v8::Object>() is preferred.
removed the DCHECK, so type conversion no longer needed (parameter is now a Local<Object> already anyways)
Patch Set #6, Line 400:
DCHECK_EQ(ToScriptWrappable(v8::Local<v8::Object>::Cast(value)), receiver);
I think we don't need this DCHECK_EQ. […]
Done
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. […]
Ack
To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.
Yuki Shiino (Gerrit)
Attention is currently required from: Caitlin Potter, Toon Verwaest.
1 comment:
Patchset:
Caitlin, would you upload your latest patch set?
To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.
Caitlin Potter (Gerrit)
Attention is currently required from: Toon Verwaest, Yuki Shiino.
2 comments:
Patchset:
Caitlin, would you upload your latest patch set?
I wanted to have my question addressed first, hope thats ok
File third_party/blink/renderer/core/frame/dom_window.cc:
Patch Set #6, Line 122:
WebFeature::kWindowProxyCrossOriginAccessFromOtherPageSelf);
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.
Yuki Shiino (Gerrit)
Attention is currently required from: Caitlin Potter, Toon Verwaest.
2 comments:
Patchset:
I'm sorry that I didn't realize that you're waiting my answer.
File third_party/blink/renderer/core/frame/dom_window.cc:
Patch Set #6, Line 122:
WebFeature::kWindowProxyCrossOriginAccessFromOtherPageSelf);
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.
Caitlin Potter (Gerrit)
Attention is currently required from: Toon Verwaest, Yuki Shiino.
1 comment:
File third_party/blink/renderer/core/frame/dom_window.cc:
Patch Set #6, Line 122:
WebFeature::kWindowProxyCrossOriginAccessFromOtherPageSelf);
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.
Yuki Shiino (Gerrit)
Attention is currently required from: Caitlin Potter, Camille Lamy, Toon Verwaest.
2 comments:
Patchset:
+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:
Patch Set #6, Line 122:
WebFeature::kWindowProxyCrossOriginAccessFromOtherPageSelf);
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.
Blink W3C Test Autoroller (Gerrit)
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
Camille Lamy (Gerrit)
Attention is currently required from: Caitlin Potter, Toon Verwaest, Yuki Shiino.
1 comment:
File third_party/blink/renderer/core/frame/dom_window.cc:
Patch Set #6, Line 122:
WebFeature::kWindowProxyCrossOriginAccessFromOtherPageSelf);
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.
Caitlin Potter (Gerrit)
Attention is currently required from: Camille Lamy, Toon Verwaest, Yuki Shiino.
1 comment:
File third_party/blink/renderer/core/frame/dom_window.cc:
Patch Set #6, Line 122:
WebFeature::kWindowProxyCrossOriginAccessFromOtherPageSelf);
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.
Yuki Shiino (Gerrit)
Attention is currently required from: Caitlin Potter, Camille Lamy, Toon Verwaest.
Patch set 8:Code-Review +1
1 comment:
Patchset:
LGTM. Thanks for working on this so patiently. :)
To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.
Yuki Shiino (Gerrit)
Attention is currently required from: Caitlin Potter, Camille Lamy, Kentaro Hara, Toon Verwaest.
1 comment:
Patchset:
+R=haraken@ for blink/renderer/core/frame/
To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
Attention is currently required from: Caitlin Potter, Camille Lamy, Toon Verwaest.
Patch set 8:Code-Review +1
1 comment:
Patchset:
LGTM
To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.
Caitlin Potter (Gerrit)
Attention is currently required from: Camille Lamy, Toon Verwaest.
Patch set 8:Commit-Queue +2
1 comment:
Patchset:
Here goes
To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.
Caitlin Potter (Gerrit)
Attention is currently required from: Camille Lamy, Toon Verwaest.
1 comment:
Patchset:
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.
Caitlin Potter (Gerrit)
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.
Caitlin Potter (Gerrit)
Attention is currently required from: Camille Lamy, Toon Verwaest.
Patch set 9:Commit-Queue +2
1 comment:
Patchset:
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.
Yuki Shiino (Gerrit)
Attention is currently required from: Caitlin Potter, Camille Lamy, Toon Verwaest.
Patch set 9:Code-Review +1
2 comments:
Patchset:
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.
LGTM.
To view, visit change 3715140. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
[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(-)