|
|
Created:
7 years, 4 months ago by adamk Modified:
7 years, 3 months ago Reviewers:
rossberg, abarth-chromium CC:
v8-dev, rafaelw Visibility:
Public. |
DescriptionAdd access check for observed objects
This change is mostly straightforward: for 'normal' sorts of change records,
simply don't deliver a changeRecord to a given observer callback if an access
the callback's Context is not allowed to "GET" or "HAS" changeRecord.name on
changeRecord.object, or if ACCESS_KEYS is disallowed.
For 'splice' records, the question of whether to hand it to an observer is trickier, since
there are multiple properties involved, and multiple types of possible information leakage.
Given that access-checked objects are very rare (only two in Blink, Window and Location),
and that they are not normally used as Arrays, it seems better to simply not emit any splice
records for such objects rather than spending lots of logic to attempt to avoid information
leakage for something that may never happen.
BUG=v8:2778
R=rossberg@chromium.org
Committed: https://code.google.com/p/v8/source/detail?r=16663
Patch Set 1 #Patch Set 2 : Working with test #Patch Set 3 : Nearly complete #
Total comments: 14
Patch Set 4 : Add ACCESS_KEYS check, add tests using a blacklist #Patch Set 5 : Added defineProperty code paths #Patch Set 6 : Add test for modification via API #Patch Set 7 : Add ACCESS_HAS checks, a few more tests #Patch Set 8 : Added some test refactoring #Patch Set 9 : Merged to trunk #
Created: 7 years, 3 months ago
Messages
Total messages: 19 (0 generated)
This looks good so far, but unfortunately, I don't think it's enough to just check GET access. For example, HAS or KEYS might be restricted, while GET is allowed. I think the accurate semantics would be something like this: - if an observer does not have GET access to a property, it should not see update changes for that property (but can still see delete or reconfiguration events); - if an observer does not have HAS access to a property, it should not see update, delete or reconfiguration changes for that property; - if an observer does not have KEYS access to an object, it should not see any changes for that object. Maybe it's not worth distinguishing the first and second, but you still need to check for either access type. The last one applies to the whole object, as opposed to individual properties, which is something that we need to distinguish in the tests. It's annoying that the API does not allow to ask about all access types in one call. But I don't see much we can do about that. Fortunately, observe performance on access-checked objects should not matter in practice, AFAICS. https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob... File test/cctest/test-object-observe.cc (right): https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob... test/cctest/test-object-observe.cc:479: TEST(ObserveNamedAccessCheck) { Would be good to refine the test such that only individual properties are blocked, while others pass (for both named and indexed). E.g. by replacing your blockAccess mechanics with a black list. https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob... test/cctest/test-object-observe.cc:488: CompileRun("records = null;" Nit: can we make these proper 'var' declarations? https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob... test/cctest/test-object-observe.cc:499: CompileRun("records2 = null;" Same here. https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob... test/cctest/test-object-observe.cc:503: "obj.foo = 'bar';" Please add other forms of mutation, in particular, via Object.defineProperty and via the API. https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob... test/cctest/test-object-observe.cc:585: "[].pop.call(obj);" Can we check some more complex array functions, too? https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob... test/cctest/test-object-observe.cc:587: // TODO(adamk): Support splice records What is this TODO about?
On 2013/08/16 13:51:54, rossberg wrote: > This looks good so far, but unfortunately, I don't think it's enough to just > check GET access. For example, HAS or KEYS might be restricted, while GET is > allowed. > > I think the accurate semantics would be something like this: > > - if an observer does not have GET access to a property, it should not see > update changes for that property (but can still see delete or reconfiguration > events); > > - if an observer does not have HAS access to a property, it should not see > update, delete or reconfiguration changes for that property; > > - if an observer does not have KEYS access to an object, it should not see any > changes for that object. > > Maybe it's not worth distinguishing the first and second, but you still need to > check for either access type. The last one applies to the whole object, as > opposed to individual properties, which is something that we need to distinguish > in the tests. > > It's annoying that the API does not allow to ask about all access types in one > call. But I don't see much we can do about that. Fortunately, observe > performance on access-checked objects should not matter in practice, AFAICS. Thanks for mentioning ACCESS_KEYS. With that as a model, I think the above all argues for just adding a new ACCESS_OBSERVE enum (as the one API change) and following ACCESS_KEYS's lead of checking access via calling MayNamedAccess() with undefined as the "key" to check for whether observation is allowed. We could check both at the time of initial observation and when enqueueing. It simply doesn't seem worthwhile to allow observation of only some changes to an object (which is what it seems you're proposing with the set of complex rules above), especially since in practice the only observed objects are Window and Location, and moreover the only properties where access _is_ allowed tend to be non-observable (they're provided by native accessors). Basically, if you're in a place where an ACCESS_GET would fail, providing Object.observe doesn't provide value. What are your thoughts on that (minimal) change to the API?
On 2013/08/16 21:48:47, adamk wrote: > On 2013/08/16 13:51:54, rossberg wrote: > > This looks good so far, but unfortunately, I don't think it's enough to just > > check GET access. For example, HAS or KEYS might be restricted, while GET is > > allowed. > > > > I think the accurate semantics would be something like this: > > > > - if an observer does not have GET access to a property, it should not see > > update changes for that property (but can still see delete or reconfiguration > > events); > > > > - if an observer does not have HAS access to a property, it should not see > > update, delete or reconfiguration changes for that property; > > > > - if an observer does not have KEYS access to an object, it should not see any > > changes for that object. > > > > Maybe it's not worth distinguishing the first and second, but you still need > to > > check for either access type. The last one applies to the whole object, as > > opposed to individual properties, which is something that we need to > distinguish > > in the tests. > > > > It's annoying that the API does not allow to ask about all access types in one > > call. But I don't see much we can do about that. Fortunately, observe > > performance on access-checked objects should not matter in practice, AFAICS. > > Thanks for mentioning ACCESS_KEYS. With that as a model, I think the above all > argues for just adding a new ACCESS_OBSERVE enum (as the one API change) and > following ACCESS_KEYS's lead of checking access via calling MayNamedAccess() > with undefined as the "key" to check for whether observation is allowed. We > could check both at the time of initial observation and when enqueueing. > > It simply doesn't seem worthwhile to allow observation of only some changes to > an object (which is what it seems you're proposing with the set of complex rules > above), especially since in practice the only observed objects are Window and > Location, and moreover the only properties where access _is_ allowed tend to be > non-observable (they're provided by native accessors). Basically, if you're in a > place where an ACCESS_GET would fail, providing Object.observe doesn't provide > value. > > What are your thoughts on that (minimal) change to the API? No, as we discussed before, this won't fly. It is not sufficient because we would still have to correctly deal with the cases where ACCESS_OBSERVE was not filtered, but e.g. GET was -- which includes all existing code in all embedders! The API allows these combinations (supposedly for a reason), so we haven't won anything by adding a new flag, we'd still need to check the other flags. It is not necessary, because if we properly check the other flags then ACCESS_OBSERVE would essentially be subsumed by ACCESS_KEYS. That is, we don't provide any new useful functionality to users of the API. It is not an option anyway because requiring everybody to set and handle ACCESS_OBSERVE is a serious breaking change to the API at this point. It breaks the security model of all existing embedders. And it probably breaks a lot of them functionally, because their access callbacks do not expect the new value. I acknowledge that proper access checking for Object.observe is an annoyance. But the reason for that is not an incomplete API, but simply the fact that Object.observe is such a big potential leak, for so many different pieces of information! We can't cheat our way out of that, I'm afraid.
On 2013/08/19 10:19:59, rossberg wrote: > On 2013/08/16 21:48:47, adamk wrote: > > On 2013/08/16 13:51:54, rossberg wrote: > > > This looks good so far, but unfortunately, I don't think it's enough to just > > > check GET access. For example, HAS or KEYS might be restricted, while GET is > > > allowed. > > > > > > I think the accurate semantics would be something like this: > > > > > > - if an observer does not have GET access to a property, it should not see > > > update changes for that property (but can still see delete or > reconfiguration > > > events); > > > > > > - if an observer does not have HAS access to a property, it should not see > > > update, delete or reconfiguration changes for that property; > > > > > > - if an observer does not have KEYS access to an object, it should not see > any > > > changes for that object. > > > > > > Maybe it's not worth distinguishing the first and second, but you still need > > to > > > check for either access type. The last one applies to the whole object, as > > > opposed to individual properties, which is something that we need to > > distinguish > > > in the tests. > > > > > > It's annoying that the API does not allow to ask about all access types in > one > > > call. But I don't see much we can do about that. Fortunately, observe > > > performance on access-checked objects should not matter in practice, AFAICS. > > > > Thanks for mentioning ACCESS_KEYS. With that as a model, I think the above all > > argues for just adding a new ACCESS_OBSERVE enum (as the one API change) and > > following ACCESS_KEYS's lead of checking access via calling MayNamedAccess() > > with undefined as the "key" to check for whether observation is allowed. We > > could check both at the time of initial observation and when enqueueing. > > > > It simply doesn't seem worthwhile to allow observation of only some changes to > > an object (which is what it seems you're proposing with the set of complex > rules > > above), especially since in practice the only observed objects are Window and > > Location, and moreover the only properties where access _is_ allowed tend to > be > > non-observable (they're provided by native accessors). Basically, if you're in > a > > place where an ACCESS_GET would fail, providing Object.observe doesn't provide > > value. > > > > What are your thoughts on that (minimal) change to the API? > > No, as we discussed before, this won't fly. > > It is not sufficient because we would still have to correctly deal with the > cases where ACCESS_OBSERVE was not filtered, but e.g. GET was -- which includes > all existing code in all embedders! The API allows these combinations > (supposedly for a reason), so we haven't won anything by adding a new flag, we'd > still need to check the other flags. > > It is not necessary, because if we properly check the other flags then > ACCESS_OBSERVE would essentially be subsumed by ACCESS_KEYS. That is, we don't > provide any new useful functionality to users of the API. > > It is not an option anyway because requiring everybody to set and handle > ACCESS_OBSERVE is a serious breaking change to the API at this point. It breaks > the security model of all existing embedders. And it probably breaks a lot of > them functionally, because their access callbacks do not expect the new value. Do you know of any other embedders that use access checks, besides Blink? I would be surprised to learn that Node.js makes use of them. Basically I'm asking: is this a theoretical concern (given that you don't know which embedders are out there) or is it concrete?
On 2013/08/19 17:15:59, adamk wrote: > Do you know of any other embedders that use access checks, besides Blink? I > would be surprised to learn that Node.js makes use of them. Basically I'm > asking: is this a theoretical concern (given that you don't know which embedders > are out there) or is it concrete? To flesh this out a little more: it seems to me that the current API is not a good fit for its use in Blink. Some mismatches: - The optional data arg, though at first glance appearing to be used, is never accessed in our security check callbacks. - The actually useful "data", the current context, is passed only implicitly (which makes the code in this patch a little weird, as it has to switch contexts before calling the callback). - Only ACCESS_HAS and ACCESS_GET are of interest to Blink. That is, ACCESS_SET and ACCESS_KEYS are not distinguished at all (both use the same check, which doesn't look at the key). Thus, if Blink were to be the only embedder that uses the access check API, I might be inclined to suggest fitting it more closely to the web security use-case.
On 2013/08/19 17:15:59, adamk wrote: > On 2013/08/19 10:19:59, rossberg wrote: > > On 2013/08/16 21:48:47, adamk wrote: > > > On 2013/08/16 13:51:54, rossberg wrote: > > > > This looks good so far, but unfortunately, I don't think it's enough to > just > > > > check GET access. For example, HAS or KEYS might be restricted, while GET > is > > > > allowed. > > > > > > > > I think the accurate semantics would be something like this: > > > > > > > > - if an observer does not have GET access to a property, it should not see > > > > update changes for that property (but can still see delete or > > reconfiguration > > > > events); > > > > > > > > - if an observer does not have HAS access to a property, it should not see > > > > update, delete or reconfiguration changes for that property; > > > > > > > > - if an observer does not have KEYS access to an object, it should not see > > any > > > > changes for that object. > > > > > > > > Maybe it's not worth distinguishing the first and second, but you still > need > > > to > > > > check for either access type. The last one applies to the whole object, as > > > > opposed to individual properties, which is something that we need to > > > distinguish > > > > in the tests. > > > > > > > > It's annoying that the API does not allow to ask about all access types in > > one > > > > call. But I don't see much we can do about that. Fortunately, observe > > > > performance on access-checked objects should not matter in practice, > AFAICS. > > > > > > Thanks for mentioning ACCESS_KEYS. With that as a model, I think the above > all > > > argues for just adding a new ACCESS_OBSERVE enum (as the one API change) and > > > following ACCESS_KEYS's lead of checking access via calling MayNamedAccess() > > > with undefined as the "key" to check for whether observation is allowed. We > > > could check both at the time of initial observation and when enqueueing. > > > > > > It simply doesn't seem worthwhile to allow observation of only some changes > to > > > an object (which is what it seems you're proposing with the set of complex > > rules > > > above), especially since in practice the only observed objects are Window > and > > > Location, and moreover the only properties where access _is_ allowed tend to > > be > > > non-observable (they're provided by native accessors). Basically, if you're > in > > a > > > place where an ACCESS_GET would fail, providing Object.observe doesn't > provide > > > value. > > > > > > What are your thoughts on that (minimal) change to the API? > > > > No, as we discussed before, this won't fly. > > > > It is not sufficient because we would still have to correctly deal with the > > cases where ACCESS_OBSERVE was not filtered, but e.g. GET was -- which > includes > > all existing code in all embedders! The API allows these combinations > > (supposedly for a reason), so we haven't won anything by adding a new flag, > we'd > > still need to check the other flags. > > > > It is not necessary, because if we properly check the other flags then > > ACCESS_OBSERVE would essentially be subsumed by ACCESS_KEYS. That is, we don't > > provide any new useful functionality to users of the API. > > > > It is not an option anyway because requiring everybody to set and handle > > ACCESS_OBSERVE is a serious breaking change to the API at this point. It > breaks > > the security model of all existing embedders. And it probably breaks a lot of > > them functionally, because their access callbacks do not expect the new value. > > Do you know of any other embedders that use access checks, besides Blink? I > would be surprised to learn that Node.js makes use of them. Basically I'm > asking: is this a theoretical concern (given that you don't know which embedders > are out there) or is it concrete? There are several browsers and browser-like environments besides Chrome that embed either V8 or Blink+V8, e.g. Android, Opera, the tool chain Adobe is working on, and some projects I know less about. And AFAIK even Blink embedders typically add their own additional native objects to the runtime on top of what Blink provides. So yes, AFAICT, this is a real issue. In particular, since it is easy to introduce security holes unnoticed with such a change. Even if it wasn't, I still think the current access API is the correct abstraction (or it least close to it). It filters by kind of observable information and its possible modifications, not by what language operations invoke those. That is an important abstraction, because you don't want to make the API and its uses dependent on the ever-changing zoo of operations the language might provide. (Although admittedly it feels rather ES3 in its current form.) Is there any use case you envision that would care whether observe is 10x vs just 3x slower on objects with access checks?
On 2013/08/19 17:51:53, rossberg wrote: > On 2013/08/19 17:15:59, adamk wrote: > > On 2013/08/19 10:19:59, rossberg wrote: > > > On 2013/08/16 21:48:47, adamk wrote: > > > > On 2013/08/16 13:51:54, rossberg wrote: > > > > > This looks good so far, but unfortunately, I don't think it's enough to > > just > > > > > check GET access. For example, HAS or KEYS might be restricted, while > GET > > is > > > > > allowed. > > > > > > > > > > I think the accurate semantics would be something like this: > > > > > > > > > > - if an observer does not have GET access to a property, it should not > see > > > > > update changes for that property (but can still see delete or > > > reconfiguration > > > > > events); > > > > > > > > > > - if an observer does not have HAS access to a property, it should not > see > > > > > update, delete or reconfiguration changes for that property; > > > > > > > > > > - if an observer does not have KEYS access to an object, it should not > see > > > any > > > > > changes for that object. > > > > > > > > > > Maybe it's not worth distinguishing the first and second, but you still > > need > > > > to > > > > > check for either access type. The last one applies to the whole object, > as > > > > > opposed to individual properties, which is something that we need to > > > > distinguish > > > > > in the tests. > > > > > > > > > > It's annoying that the API does not allow to ask about all access types > in > > > one > > > > > call. But I don't see much we can do about that. Fortunately, observe > > > > > performance on access-checked objects should not matter in practice, > > AFAICS. > > > > > > > > Thanks for mentioning ACCESS_KEYS. With that as a model, I think the above > > all > > > > argues for just adding a new ACCESS_OBSERVE enum (as the one API change) > and > > > > following ACCESS_KEYS's lead of checking access via calling > MayNamedAccess() > > > > with undefined as the "key" to check for whether observation is allowed. > We > > > > could check both at the time of initial observation and when enqueueing. > > > > > > > > It simply doesn't seem worthwhile to allow observation of only some > changes > > to > > > > an object (which is what it seems you're proposing with the set of complex > > > rules > > > > above), especially since in practice the only observed objects are Window > > and > > > > Location, and moreover the only properties where access _is_ allowed tend > to > > > be > > > > non-observable (they're provided by native accessors). Basically, if > you're > > in > > > a > > > > place where an ACCESS_GET would fail, providing Object.observe doesn't > > provide > > > > value. > > > > > > > > What are your thoughts on that (minimal) change to the API? > > > > > > No, as we discussed before, this won't fly. > > > > > > It is not sufficient because we would still have to correctly deal with the > > > cases where ACCESS_OBSERVE was not filtered, but e.g. GET was -- which > > includes > > > all existing code in all embedders! The API allows these combinations > > > (supposedly for a reason), so we haven't won anything by adding a new flag, > > we'd > > > still need to check the other flags. > > > > > > It is not necessary, because if we properly check the other flags then > > > ACCESS_OBSERVE would essentially be subsumed by ACCESS_KEYS. That is, we > don't > > > provide any new useful functionality to users of the API. > > > > > > It is not an option anyway because requiring everybody to set and handle > > > ACCESS_OBSERVE is a serious breaking change to the API at this point. It > > breaks > > > the security model of all existing embedders. And it probably breaks a lot > of > > > them functionally, because their access callbacks do not expect the new > value. > > > > Do you know of any other embedders that use access checks, besides Blink? I > > would be surprised to learn that Node.js makes use of them. Basically I'm > > asking: is this a theoretical concern (given that you don't know which > embedders > > are out there) or is it concrete? > > There are several browsers and browser-like environments besides Chrome that > embed either V8 or Blink+V8, e.g. Android, Opera, the tool chain Adobe is > working on, and some projects I know less about. And AFAIK even Blink embedders > typically add their own additional native objects to the runtime on top of what > Blink provides. So yes, AFAICT, this is a real issue. In particular, since it is > easy to introduce security holes unnoticed with such a change. > > Even if it wasn't, I still think the current access API is the correct > abstraction (or it least close to it). It filters by kind of observable > information and its possible modifications, not by what language operations > invoke those. That is an important abstraction, because you don't want to make > the API and its uses dependent on the ever-changing zoo of operations the > language might provide. (Although admittedly it feels rather ES3 in its current > form.) Thanks for the quick response, and for explaining your concerns in more detail. > Is there any use case you envision that would care whether observe is 10x vs > just 3x slower on objects with access checks? It wasn't a performance concern I had in mind, only a complexity concern (e.g., that we might end up introducing more bugs by trying to follow the rules you laid out in your first response). But I take your point that at least the API provides abstraction between the concrete security concern (leaks between different-origin frames) and the details of the language (how one can get access to properties of an object). So, to go back to your initial response: would you be OK with being minimally permissive, and simply checking all three access types and dropping the record if any of HAS, GET, or KEYS fails the access check? This would also happen to avoid any performance change in practice in Blink, since the KEYS check will fail fast.
On 2013/08/19 17:51:53, rossberg wrote: > There are several browsers and browser-like environments besides Chrome that > embed either V8 or Blink+V8, Of the ones you've mentioned, I don't know of any other than Blink that turn on access checks. It's a super odd-ball thing to do. Even Blink only does it for two objects, and then only because of legacy HTML content.
https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob... File test/cctest/test-object-observe.cc (right): https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob... test/cctest/test-object-observe.cc:479: TEST(ObserveNamedAccessCheck) { On 2013/08/16 13:51:54, rossberg wrote: > Would be good to refine the test such that only individual properties are > blocked, while others pass (for both named and indexed). E.g. by replacing your > blockAccess mechanics with a black list. Done. https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob... test/cctest/test-object-observe.cc:488: CompileRun("records = null;" On 2013/08/16 13:51:54, rossberg wrote: > Nit: can we make these proper 'var' declarations? Done. https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob... test/cctest/test-object-observe.cc:499: CompileRun("records2 = null;" On 2013/08/16 13:51:54, rossberg wrote: > Same here. Done. https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob... test/cctest/test-object-observe.cc:503: "obj.foo = 'bar';" On 2013/08/16 13:51:54, rossberg wrote: > Please add other forms of mutation, in particular, via Object.defineProperty and > via the API. Happy to expand this a little bit, but I'd be interested in what you think counts as good coverage; see my question below about complex array methods. https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob... test/cctest/test-object-observe.cc:585: "[].pop.call(obj);" On 2013/08/16 13:51:54, rossberg wrote: > Can we check some more complex array functions, too? How complex do you want? It's not clear to me what stuff should be tested here, vs just testing access checks in general and testing record enqueueing in general. We have tests in mjsunit for testing that we get the right records in Object.observe; as long as there's a reasonably tight bottleneck in src/object-observe.js for where we enqueue records, I wouldn't expect there to be any increase in test coverage from testing, say, 2 array methods compared to 5 array methods. I ask because writing tests in this style (as API tests) is super-verbose and time consuming, compared to writing tests in JS, so if I can get more coverage out of the JS tests I'd much rather do that. https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob... test/cctest/test-object-observe.cc:587: // TODO(adamk): Support splice records On 2013/08/16 13:51:54, rossberg wrote: > What is this TODO about? Made it longer.
Added some more tests, too; still interested in your thoughts on how much coverage is enough here.
On 2013/08/19 18:24:29, adamk wrote: > So, to go back to your initial response: would you be OK with being minimally > permissive, and simply checking all three access types and dropping the record > if any of HAS, GET, or KEYS fails the access check? This would also happen to > avoid any performance change in practice in Blink, since the KEYS check will > fail fast. Yes, that sounds perfectly reasonable to me.
On 2013/08/19 18:58:57, abarth wrote: > On 2013/08/19 17:51:53, rossberg wrote: > > There are several browsers and browser-like environments besides Chrome that > > embed either V8 or Blink+V8, > > Of the ones you've mentioned, I don't know of any other than Blink that turn on > access checks. It's a super odd-ball thing to do. Even Blink only does it for > two objects, and then only because of legacy HTML content. Do you know if there is a chance that Blink might get along without that some day? We could probably eliminate thousands of lines of messy V8-internal code if we could rip out access checking.
If you also add checking for ACCESS_HAS then I think the CL is good to go. https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob... File test/cctest/test-object-observe.cc (right): https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob... test/cctest/test-object-observe.cc:585: "[].pop.call(obj);" On 2013/08/20 21:38:16, adamk wrote: > On 2013/08/16 13:51:54, rossberg wrote: > > Can we check some more complex array functions, too? > > How complex do you want? It's not clear to me what stuff should be tested here, > vs just testing access checks in general and testing record enqueueing in > general. > > We have tests in mjsunit for testing that we get the right records in > Object.observe; as long as there's a reasonably tight bottleneck in > src/object-observe.js for where we enqueue records, I wouldn't expect there to > be any increase in test coverage from testing, say, 2 array methods compared to > 5 array methods. > > I ask because writing tests in this style (as API tests) is super-verbose and > time consuming, compared to writing tests in JS, so if I can get more coverage > out of the JS tests I'd much rather do that. Doesn't have to be crazy. I'd be content with one more method that creates more than just a single straightforward splice record.
On 2013/08/21 10:45:51, rossberg wrote: > If you also add checking for ACCESS_HAS then I think the CL is good to go. This just means another call to May*Access(), right? Will add that, and rename the runtime method to reflect new behavior.
Okay, added everything I think we needed. The tests have gotten a little verbose; let me know if you'd like them refactored before commit. https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob... File test/cctest/test-object-observe.cc (right): https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob... test/cctest/test-object-observe.cc:585: "[].pop.call(obj);" On 2013/08/21 10:45:52, rossberg wrote: > On 2013/08/20 21:38:16, adamk wrote: > > On 2013/08/16 13:51:54, rossberg wrote: > > > Can we check some more complex array functions, too? > > > > How complex do you want? It's not clear to me what stuff should be tested > here, > > vs just testing access checks in general and testing record enqueueing in > > general. > > > > We have tests in mjsunit for testing that we get the right records in > > Object.observe; as long as there's a reasonably tight bottleneck in > > src/object-observe.js for where we enqueue records, I wouldn't expect there to > > be any increase in test coverage from testing, say, 2 array methods compared > to > > 5 array methods. > > > > I ask because writing tests in this style (as API tests) is super-verbose and > > time consuming, compared to writing tests in JS, so if I can get more coverage > > out of the JS tests I'd much rather do that. > > Doesn't have to be crazy. I'd be content with one more method that creates more > than just a single straightforward splice record. The nature of splice records is that there aren't any array operations that create more than one record, but I've updated the test to do more than one operation on the object.
> Okay, added everything I think we needed. The tests have gotten a little > verbose; let me know if you'd like them refactored before commit. LGTM, except that, yes, it would be good if you could refactor the tests to avoid the repetition. Can't you just wrap the test body into a loop over an array from which you take pointers to different AccessCheck functions? (Please keep in mind that the tree is still not open for landing, though.) > The nature of splice records is that there aren't any array operations that > create more than one record, but I've updated the test to do more than one > operation on the object. OK. I thought there were some that can create multiple splice records under certain conditions, but I probably misremembered. :)
On 2013/08/22 11:14:09, rossberg wrote: > > Okay, added everything I think we needed. The tests have gotten a little > > verbose; let me know if you'd like them refactored before commit. > > LGTM, except that, yes, it would be good if you could refactor the tests to > avoid the repetition. Can't you just wrap the test body into a loop over an > array from which you take pointers to different AccessCheck functions? Okay, refactored to avoid some repetition. Named and Indexed checks have a single TEST block each, and I've added a global static to mark the type to block. I also factored out a helper for generating the access-checked object in the first place. Landing now (test coverage is the same as before), happy to do more work on the tests if you have any follow-up comments. > (Please keep in mind that the tree is still not open for landing, though.) > > > > The nature of splice records is that there aren't any array operations that > > create more than one record, but I've updated the test to do more than one > > operation on the object. > > OK. I thought there were some that can create multiple splice records under > certain conditions, but I probably misremembered. :)
Message was sent while issue was closed.
Committed patchset #9 manually as r16663 (presubmit successful). |