Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2807333003:
[api] Add DefineProperty() method that skips interceptors.
Description[api] Add DefineProperty() method that skips interceptors.
This extension to the API accommodates
scenarios, which require setting own
properties on an object without triggering
interceptors.
BUG=
Patch Set 1 #
Total comments: 4
Patch Set 2 : Extend DefineProperty API #
Total comments: 9
Patch Set 3 : Change flag from bool to enum #Patch Set 4 : Previous commit with rebasing #
Total comments: 1
Patch Set 5 : Change enum to enum class #
Messages
Total messages: 42 (24 generated)
Description was changed from ========== Add DefinePropertyWithoutInterceptor() to the API This extension of the API is motivated by the Node.js vm module. DefineProperty() sets properties on the Node.js sandbox, but fails to do it on the global object in the vm. GenericNamedPropertyDefinerCallback triggers GenericNamedPropertyDescriptorCallback, which reads the property off the sandbox and, when un-intercepted, does not continue with setting it on the global object. This solution defines a separate API function to set ES6-style properties on the global object. BUG= ========== to ========== Add DefinePropertyWithoutInterceptor() to the API This extension of the API is motivated by the Node.js vm module. DefineProperty() sets properties on the Node.js sandbox, but fails to do it on the global object in the vm. GenericNamedPropertyDefinerCallback triggers GenericNamedPropertyDescriptorCallback, which reads the property off the sandbox and, when un-intercepted, does not continue with setting it on the global object. This solution defines a separate API function to set ES6-style properties on the global object. BUG= ==========
anna.mag.kedzierska@gmail.com changed reviewers: + franzih@chromium.org
anna.mag.kedzierska@gmail.com changed required reviewers: + franzih@chromium.org
The CQ bit was checked by anna.mag.kedzierska@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: All required reviewers (with asterisk prefixes) have not yet approved this CL. No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! Good work. I'm not a fan of the code duplication. Can we use a flag instead that we thread through the functions? https://codereview.chromium.org/2807333003/diff/1/include/v8.h File include/v8.h (right): https://codereview.chromium.org/2807333003/diff/1/include/v8.h#newcode3009 include/v8.h:3009: V8_WARN_UNUSED_RESULT Maybe<bool> DefinePropertyWithoutInterceptors( Can you add a comment please. Maybe make the function name consistent with the other functions that bypass interceptors. https://codereview.chromium.org/2807333003/diff/1/src/builtins/builtins-objec... File src/builtins/builtins-object.cc (right): https://codereview.chromium.org/2807333003/diff/1/src/builtins/builtins-objec... src/builtins/builtins-object.cc:82: BUILTIN(ObjectDefinePropertyWithoutInterceptors) { Do we need this builtin? https://codereview.chromium.org/2807333003/diff/1/src/objects.h File src/objects.h (right): https://codereview.chromium.org/2807333003/diff/1/src/objects.h#newcode1984 src/objects.h:1984: PropertyDescriptor* desc, ShouldThrow should_throw); I think you call this function only with dont_throw, so we don't need to carry this parameter. https://codereview.chromium.org/2807333003/diff/1/test/cctest/test-api.cc File test/cctest/test-api.cc (right): https://codereview.chromium.org/2807333003/diff/1/test/cctest/test-api.cc#new... test/cctest/test-api.cc:15968: +TEST(DefinePropertyWithoutInterceptors) { I think the tests should be in test-api-interceptors.cc - and there should be interceptors installed that are bypassed by the new function.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Yes, the flag makes much more sense- the duplication here is too extensive. So it'd be an extended API DefineProperty() with a default valued that maintains current behaviour, e.g. DefineProperty(..., not_intercepted) (default: false) Is that what you mean? Thanks!
On 2017/04/12 09:14:11, AnnaMag wrote: > Yes, the flag makes much more sense- the duplication here is too extensive. > > So it'd be an extended API DefineProperty() with a default valued that > maintains current behaviour, e.g. > DefineProperty(..., not_intercepted) (default: false) > > Is that what you mean? > > Thanks! yes, maybe bool bypass_interceptors = false;
On 2017/04/12 09:20:43, Franzi wrote: > On 2017/04/12 09:14:11, AnnaMag wrote: > > Yes, the flag makes much more sense- the duplication here is too extensive. > > > > So it'd be an extended API DefineProperty() with a default valued that > > maintains current behaviour, e.g. > > DefineProperty(..., not_intercepted) (default: false) > > > > Is that what you mean? > > > > Thanks! > > yes, maybe > bool bypass_interceptors = false; This is a re-write of the previous patch which was heavy on code duplication. A flag bypass_interceptor in DefineProperty is false by default and maintains the current behavior. bypass_interceptor = true sets own property on an object bypassing interceptors. The test was included in the test-api-interceptor.cc, and previous tests removed from the test-api.cc. Could you please have a look? Thank you!
Here are my comments. I think there was a rebase issue with one of the tests files. Can you double check? Thanks, Franzi https://codereview.chromium.org/2807333003/diff/20001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2807333003/diff/20001/src/api.cc#newcode4272 src/api.cc:4272: isolate, self, key_obj, &desc, i::AccessCheckInfo::DONT_THROW, Why did this change? https://codereview.chromium.org/2807333003/diff/20001/src/builtins/builtins-o... File src/builtins/builtins-object.cc (right): https://codereview.chromium.org/2807333003/diff/20001/src/builtins/builtins-o... src/builtins/builtins-object.cc:117: isolate, receiver, name, &desc, Object::DONT_THROW, false); I'd prefer an enum instead of a bool. "false" is just harder to understand than e.g., Object::DONT_SKIP_INTERCEPTORS. https://codereview.chromium.org/2807333003/diff/20001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2807333003/diff/20001/src/objects.cc#newcode6252 src/objects.cc:6252: LookupIterator::Configuration ItBase = LookupIterator::OWN; Can we name this iterator_config or config? https://codereview.chromium.org/2807333003/diff/20001/src/objects.cc#newcode6271 src/objects.cc:6271: I think if OWN_SKIP_INTERCEPTOR was used, the state is never INTERCEPTOR? Could you check if removing that if breaks the tests? https://codereview.chromium.org/2807333003/diff/20001/src/objects.cc#newcode6604 src/objects.cc:6604: &new_desc, should_throw, false); Do we need the last parameter? I thought it has a default value. https://codereview.chromium.org/2807333003/diff/20001/test/cctest/test-api-in... File test/cctest/test-api-interceptors.cc (right): https://codereview.chromium.org/2807333003/diff/20001/test/cctest/test-api-in... test/cctest/test-api-interceptors.cc:1819: // Check that the descriptor callback is not triggered for DefineProperty Is this description correct? I thought we're skipping the definer callback. https://codereview.chromium.org/2807333003/diff/20001/test/cctest/test-api-in... test/cctest/test-api-interceptors.cc:1828: NotInterceptingPropertyDefineCallback)); This tests seems to make more sense if you set an interceptor that actually intercepts. And then check that the interceptor is not triggered. If the interceptor doesn't do anything it's hard to show that it was not called. https://codereview.chromium.org/2807333003/diff/20001/test/cctest/test-api-in... test/cctest/test-api-interceptors.cc:1850: p = v8_str("v2"); Why are you using a different p here? https://codereview.chromium.org/2807333003/diff/20001/test/cctest/test-api.cc File test/cctest/test-api.cc (right): https://codereview.chromium.org/2807333003/diff/20001/test/cctest/test-api.cc... test/cctest/test-api.cc:459: + CcTest::CollectAllGarbage(); Is this a rebase problem?
Description was changed from ========== Add DefinePropertyWithoutInterceptor() to the API This extension of the API is motivated by the Node.js vm module. DefineProperty() sets properties on the Node.js sandbox, but fails to do it on the global object in the vm. GenericNamedPropertyDefinerCallback triggers GenericNamedPropertyDescriptorCallback, which reads the property off the sandbox and, when un-intercepted, does not continue with setting it on the global object. This solution defines a separate API function to set ES6-style properties on the global object. BUG= ========== to ========== Add a flag that bypasses interceptors to DefineProperty() This extension to the API accommodates scenarios, which require setting own properties on an object without triggering interceptors. BUG= ==========
On 2017/05/10 09:17:53, Franzi wrote: > Here are my comments. I think there was a rebase issue with one of the tests > files. Can you double check? Thanks, > > Franzi > > https://codereview.chromium.org/2807333003/diff/20001/src/api.cc > File src/api.cc (right): > > https://codereview.chromium.org/2807333003/diff/20001/src/api.cc#newcode4272 > src/api.cc:4272: isolate, self, key_obj, &desc, i::AccessCheckInfo::DONT_THROW, > Why did this change? > > https://codereview.chromium.org/2807333003/diff/20001/src/builtins/builtins-o... > File src/builtins/builtins-object.cc (right): > > https://codereview.chromium.org/2807333003/diff/20001/src/builtins/builtins-o... > src/builtins/builtins-object.cc:117: isolate, receiver, name, &desc, > Object::DONT_THROW, false); > I'd prefer an enum instead of a bool. "false" is just harder to understand than > e.g., Object::DONT_SKIP_INTERCEPTORS. > > https://codereview.chromium.org/2807333003/diff/20001/src/objects.cc > File src/objects.cc (right): > > https://codereview.chromium.org/2807333003/diff/20001/src/objects.cc#newcode6252 > src/objects.cc:6252: LookupIterator::Configuration ItBase = LookupIterator::OWN; > Can we name this iterator_config or config? > > https://codereview.chromium.org/2807333003/diff/20001/src/objects.cc#newcode6271 > src/objects.cc:6271: > I think if OWN_SKIP_INTERCEPTOR was used, the state is never INTERCEPTOR? Could > you check if removing that if breaks the tests? > > https://codereview.chromium.org/2807333003/diff/20001/src/objects.cc#newcode6604 > src/objects.cc:6604: &new_desc, should_throw, false); > Do we need the last parameter? I thought it has a default value. > > https://codereview.chromium.org/2807333003/diff/20001/test/cctest/test-api-in... > File test/cctest/test-api-interceptors.cc (right): > > https://codereview.chromium.org/2807333003/diff/20001/test/cctest/test-api-in... > test/cctest/test-api-interceptors.cc:1819: // Check that the descriptor callback > is not triggered for DefineProperty > Is this description correct? > > I thought we're skipping the definer callback. > > https://codereview.chromium.org/2807333003/diff/20001/test/cctest/test-api-in... > test/cctest/test-api-interceptors.cc:1828: > NotInterceptingPropertyDefineCallback)); > This tests seems to make more sense if you set an interceptor that actually > intercepts. And then check that the interceptor is not triggered. If the > interceptor doesn't do anything it's hard to show that it was not called. > > https://codereview.chromium.org/2807333003/diff/20001/test/cctest/test-api-in... > test/cctest/test-api-interceptors.cc:1850: p = v8_str("v2"); > Why are you using a different p here? > > https://codereview.chromium.org/2807333003/diff/20001/test/cctest/test-api.cc > File test/cctest/test-api.cc (right): > > https://codereview.chromium.org/2807333003/diff/20001/test/cctest/test-api.cc... > test/cctest/test-api.cc:459: + CcTest::CollectAllGarbage(); > Is this a rebase problem? I believe I addressed all the comments and modified the code accordingly. Thanks! I added enum to the v8 namespace, not to the Object. I might be missing on important point here, so if that should belong to Object please let me know and I modify it. Thank you for reviewing!
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
Can you rebase so we can run the trybots? Thanks!
On 2017/05/23 14:22:23, Franzi wrote: > Can you rebase so we can run the trybots? Thanks! Done. Thank you
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
franzih@chromium.org changed reviewers: + jochen@chromium.org
Looks good from my side. Anna, can you change the commit message title like so, please: [api] Add DefineProperty() method that skips interceptors. Jochen, can you have a look? Thanks.
Description was changed from ========== Add a flag that bypasses interceptors to DefineProperty() This extension to the API accommodates scenarios, which require setting own properties on an object without triggering interceptors. BUG= ========== to ========== [api] Add DefineProperty() method that skips interceptors. This extension to the API accommodates scenarios, which require setting own properties on an object without triggering interceptors. BUG= ==========
hey, thanks for the patch. Could you explain the use case for this API a bit? Ideally, the C++ API would be pretty close to capabilities exposed to javascript, and script doesn't get to skip interceptors either. https://codereview.chromium.org/2807333003/diff/60001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/2807333003/diff/60001/include/v8.h#newcode2996 include/v8.h:2996: enum CallInterceptors { DONT_SKIP_INTERCEPTORS, SKIP_INTERCEPTORS }; please use enum class CallInterceptors { kSkip, kDontSkip } or similar
On 2017/05/29 19:45:38, jochen wrote: > hey, > > thanks for the patch. Could you explain the use case for this API a bit? > Ideally, the C++ API would be pretty close to capabilities exposed to > javascript, and script doesn't get to skip interceptors either. > > https://codereview.chromium.org/2807333003/diff/60001/include/v8.h > File include/v8.h (right): > > https://codereview.chromium.org/2807333003/diff/60001/include/v8.h#newcode2996 > include/v8.h:2996: enum CallInterceptors { DONT_SKIP_INTERCEPTORS, > SKIP_INTERCEPTORS }; > please use enum class CallInterceptors { kSkip, kDontSkip } or similar Thank you for reviewing! I have changed the definition. I see the point of keeping the API as close to the Javascript specs as possible. The use case comes from Node.js. Currently setting ES6 style properties is broken and we wrote a patch that uses the new DefineProperty() API. In Node.js, properties are synchronized between the sandbox and the global object. That is, they are defined (or modified in any way) on the sandbox with the help of callbacks, while the global mechanism sets them on the global object. In both scenarios, Descriptor callback is trigger to query property descriptors from the sandbox. In essence, it monitors what has changed (if anything) for a given property. This is where we faced a problem that this API extension solves. In particular, it holds for first-time definitions. We start by setting the property on the sandbox using DefineProperty(). When it comes to setting it on the global object, the Descriptor callback kicks in and queries the property from the sandbox. Since nothing has changed (we just defined it on the sandbox), it assumes there is nothing to do and does not proceed with the definition. As a result, the property is missing from the global object. The solution we implemented tweaks this mechanism. It sets the property on the sandbox and the global object explicitly and independently using DefineProperty, and intercepts the Node.js callbacks to stop the global mechanism from being executed. Without the flag, DefineProperty will trigger the Descriptor callback and the same issue will occur: it will look for definitions on the sandbox and stop. The flag allows us to separate property definition on the global object from setting it on the sandbox. The problem was brought to light with this patch (and not before), because current solution for property settings is a 'hacky' workaround and does not use callbacks. What I described is implemented here: https://github.com/nodejs/node/pull/13265/files#diff-0cf206672499c2f86db4ffb0... I hope it makes sense and shows a valid point for the patch. Apologies if I was explaining any obvious parts here. This patch has been tested in Node.js and works like magic, solving most of the current vm module issues. Franziska, would you have something to add/clarify/correct?
On 2017/05/31 12:17:06, AnnaMag wrote: > On 2017/05/29 19:45:38, jochen wrote: > > hey, > > > > thanks for the patch. Could you explain the use case for this API a bit? > > Ideally, the C++ API would be pretty close to capabilities exposed to > > javascript, and script doesn't get to skip interceptors either. > > > > https://codereview.chromium.org/2807333003/diff/60001/include/v8.h > > File include/v8.h (right): > > > > https://codereview.chromium.org/2807333003/diff/60001/include/v8.h#newcode2996 > > include/v8.h:2996: enum CallInterceptors { DONT_SKIP_INTERCEPTORS, > > SKIP_INTERCEPTORS }; > > please use enum class CallInterceptors { kSkip, kDontSkip } or similar > > Thank you for reviewing! > > I have changed the definition. I see the point of keeping the API as close to > the Javascript > specs as possible. > > The use case comes from Node.js. Currently setting ES6 style properties is > broken > and we wrote a patch that uses the new DefineProperty() API. > > In Node.js, properties are synchronized between the sandbox and the global > object. > That is, they are defined (or modified in any way) on the sandbox with the > help of callbacks, while the global mechanism sets them on the global object. > In both scenarios, Descriptor callback is trigger to query property descriptors > from the sandbox. In essence, it monitors what has changed (if anything) > for a given property. > > This is where we faced a problem that this API extension solves. > In particular, it holds for first-time definitions. > We start by setting the property on the sandbox using DefineProperty(). > When it comes to setting it on the global object, the Descriptor callback kicks > in > and queries the property from the sandbox. Since nothing has changed (we just > defined > it on the sandbox), it assumes there is nothing to do and does not proceed > with the definition. As a result, the property is missing from the global > object. > > The solution we implemented tweaks this mechanism. It sets the property on the > sandbox > and the global object explicitly and independently using DefineProperty, and > intercepts > the Node.js callbacks to stop the global mechanism from being executed. > Without the flag, DefineProperty will trigger the Descriptor callback > and the same issue will occur: it will look for definitions > on the sandbox and stop. The flag allows us to separate property definition > on the global object from setting it on the sandbox. > > The problem was brought to light with this patch (and not before), because > current solution > for property settings is a 'hacky' workaround and does not use callbacks. > > What I described is implemented here: > https://github.com/nodejs/node/pull/13265/files#diff-0cf206672499c2f86db4ffb0... > > I hope it makes sense and shows a valid point for the patch. Apologies if I was > explaining any obvious parts here. > > This patch has been tested in Node.js and works like magic, solving most > of the current vm module issues. > > Franziska, would you have something to add/clarify/correct? p.s. Apologies for weird text formatting above, not sure what has happened there:)
Thanks for the detailed response. After reading through your pull request, I wonder why you don't just not set the return value in the definer interceptor, so V8 would automatically set the property on the global object?
On 2017/06/01 13:27:22, jochen wrote: > Thanks for the detailed response. After reading through your pull request, I > wonder why you don't just not set the return value in the definer interceptor, > so V8 would automatically set the property on the global object? Thank you for taking time to look at the code! What you suggest is exactly how we started off and came across the problem. As you mentioned, without returning a value V8 goes on to define the property on the global object. As the first step, it checks property existence and attributes/descriptor wrt to the new descriptor calling GetOwnPropertyDescriptor. This triggers the Descriptor callback... and we are back to the same problem (querying property from the sandbox, which we have set in the previous step). In summary: in both scenarios (initial/your suggestion and DefineProperty without the flag), once the Descriptor callback is triggered, it looks up property on the sandbox. V8 thinks the property already exists and the descriptor matches, so it stops. If there is an alternative way of solving this, I'd be happy to test it. Thanks!
Ah, I see I wonder whether Toon has a different idea. I wonder whether we should also add the CallInterceptors parameter to all other methods where we right now have a "...RealNamed..." version instead? Of course, that should probably be a follow-up then.
jochen@chromium.org changed reviewers: + verwaest@chromium.org
actually adding Toon?
On 2017/06/02 12:41:55, jochen (gone - plz use gerrit) wrote: > actually adding Toon? Toon, any thoughts/ideas on this solution? Thanks!
franzih@chromium.org changed reviewers: + yangguo@chromium.org
Sorry for the late reply, I've been OOO for 2 months... Overall this looks good, but it seems like the flag spread further than it needs to? E.g., ArraySetLength surely doesn't need this flag, since arrays can't even have interceptors? Through which path do we reach that function? Can you reduce the number of modified methods to the absolute minimum? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
