remove uses of deprecated v8::PropertyCallbackInfo::This() #6003
Conversation
|
@kentonv ptal - i'm unsure what the consequences of the changes to worker-rpc.c++ are |
Merging this PR will degrade performance by 18.11%
Performance Changes
Comparing 
Footnotes
|
|
ok, this latest version seems to work ok. the properties won't show up as own properties. capnweb tests seems to run except for something related to a websocket connection, which i assume is unrelated |
| Evaluator<InterceptContext, InterceptIsolate> e(v8System); | ||
| e.expectEval("p = new ProxyImpl; p.bar", "number", "123"); | ||
| e.expectEval("p = new ProxyImpl; Reflect.has(p, 'foo')", "boolean", "true"); | ||
| e.expectEval("p = new ProxyImpl; Reflect.has(p, 'foo')", "boolean", "false"); |
There was a problem hiding this comment.
This would be my only hesitation here as an observable change. I highly doubt it'll actually break anything so consider it non-blocking but it at least warrants pointing out.
There was a problem hiding this comment.
from what i understand, this should have been the behaviour all along. if anyone relies on this, it's a bit crazy, but yes, it's observable and i'm not sure there's much we can do about it
There was a problem hiding this comment.
Hmm, actually this worries me. If someone is checking for the existence of properties in an RPC stub, today they'd get true for everything, but after this change they get false?
I could imagine someone being broken.
Could we solve this by placing an interceptor on the prototype as well, and having that one return true for these queries? (I think we discussed such a thing at the meeting last week.)
There was a problem hiding this comment.
Unfortunately I believe only one interceptor is supported and a second interceptor up the chain will just be ignored. We were talking about having different portions of the interception on different portions of the chain (like the query in on spot and the getter in another), but I don't think that works here. I'll experiment.
There was a problem hiding this comment.
Can we distinguish somehow whether the caller called Reflect.has() vs Object.hasOwn()? We really want the former to return true but the latter to return false.
If we can't distinguish that, maybe it wouldn't be too hard to change V8 so that we can?
There was a problem hiding this comment.
Well, Relect.has(...) will hit the has proxy trap, while Object.hasOwn(...) will hit the getOwnPropertyDescriptor() proxy trap... not sure if that helps us here.. but that's one way we can differentiate if we are using Proxy
There was a problem hiding this comment.
Yeah if we switch to Proxy then we don't have any problems... but that's a big change, hoping to avoid it. :)
| "JsType.prototype = new NumberBox(123);\n" | ||
| "new JsType().value", | ||
| "throws", kIllegalInvocation); | ||
| "number", "123"); |
There was a problem hiding this comment.
@kentonv there's another observable change here. in theory it shouldn't matter? certainly moving from throw an exception to not is unlikely to break much but just checking
There was a problem hiding this comment.
Yeah I'm not too worried about this.
We might also want to add a test that uses Reflect.get() with an explicit receiver (third argument) to verify that the property still ends up being read from the holder and not the receiver somehow.
dcarney-cf
changed the title
4 participants
No description provided.