Issue 124480043: Test array generic for readonly property violations and repair some (Closed)
|
Created:
11 years, 6 months ago by MarkM Modified:
11 years, 6 months ago CC:
google-caja-discuss_googlegroups.com Base URL:
http://google-caja.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionTest array generic for readonly property violations and repair some
Patch Set 1 #
Total comments: 6
Patch Set 2 : Test array generic for readonly property violations and repair some #
Total comments: 10
Patch Set 3 : Test array generic for readonly property violations and repair some #
Total comments: 2
Patch Set 4 : Test array generic for readonly property violations and repair some #MessagesTotal messages: 7
https://codereview.appspot.com/124480043/diff/1/src/com/google/caja/ses/repai... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/124480043/diff/1/src/com/google/caja/ses/repai... src/com/google/caja/ses/repairES5.js:2176: function test_SPLICE_IGNORES_READONLY() { Because these have such uniform structure, please shorten them and remove possibilities of cut-and-paste/partial-update errors by abstracting the common parts. See arrayMutatorProblem() for a closely related example. (I don't think arrayMutatorProblem itself can be adapted for this problem cleanly, but if you see a good readable way to do so, please do.) https://codereview.appspot.com/124480043/diff/1/src/com/google/caja/ses/repai... src/com/google/caja/ses/repairES5.js:4573: preSeverity: severities.UNSAFE_SPEC_VIOLATION, If these are UNSAFE_SPEC_VIOLATIONs, then we need to do something about them for Caja. In line with our previous decisions, I think they should be added to the acceptableProblems (if I have the name right) list since Caja does not rely on less-than-completely-frozen objects. Verify the settings using, say, the generic-host-page: it should succeed in running guest content when the allowable severity is set to NO_KNOWN_EXPLOIT_SPEC_VIOLATION.
Sign in to reply to this message.
PTAL https://codereview.appspot.com/124480043/diff/1/src/com/google/caja/ses/repai... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/124480043/diff/1/src/com/google/caja/ses/repai... src/com/google/caja/ses/repairES5.js:2176: function test_SPLICE_IGNORES_READONLY() { On 2014/08/16 01:35:53, kpreid2 wrote: > Because these have such uniform structure, please shorten them and remove > possibilities of cut-and-paste/partial-update errors by abstracting the common > parts. > > See arrayMutatorProblem() for a closely related example. (I don't think > arrayMutatorProblem itself can be adapted for this problem cleanly, but if you > see a good readable way to do so, please do.) Done. https://codereview.appspot.com/124480043/diff/1/src/com/google/caja/ses/repai... src/com/google/caja/ses/repairES5.js:4573: preSeverity: severities.UNSAFE_SPEC_VIOLATION, On 2014/08/16 01:35:53, kpreid2 wrote: > If these are UNSAFE_SPEC_VIOLATIONs, then we need to do something about them for > Caja. In line with our previous decisions, I think they should be added to the > acceptableProblems (if I have the name right) list since Caja does not rely on > less-than-completely-frozen objects. > > Verify the settings using, say, the generic-host-page: it should succeed in > running guest content when the allowable severity is set to > NO_KNOWN_EXPLOIT_SPEC_VIOLATION. On all browsers, all tests that they initially fail are repaired successfully. In particular: On v8 (Chrome, Opera), all the shift/unshift tests initial fail. On IE11, unshift initially doesn't throw or either readonly or non-writable. In both cases, redefining shift/unshift in terms of splice fixes all problems. Given that, should I still add all these to acceptableProblems ?
Sign in to reply to this message.
https://codereview.appspot.com/124480043/diff/1/src/com/google/caja/ses/repai... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/124480043/diff/1/src/com/google/caja/ses/repai... src/com/google/caja/ses/repairES5.js:4573: preSeverity: severities.UNSAFE_SPEC_VIOLATION, On 2014/08/18 14:00:00, MarkM wrote: > On all browsers, all tests that they initially fail are repaired successfully. OK, that's fine then. I didn't realize you were adding tests for problems beyond the ones we're actually seeing. https://codereview.appspot.com/124480043/diff/20001/src/com/google/caja/ses/r... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/124480043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:3283: splice.apply(this, concat.call([0, 0], items)); In general, I think we shouldn't use a top-level captured function, but use one you capture when the repair function executes. This ensures that in the future if we have a repair affecting splice, they "layer" rather than bypassing each other. (This still might be a bad result depending on which order the repairs execute in, but I claim that this pattern is more robust in general.) https://codereview.appspot.com/124480043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:3295: if (len <= 0) { return void 0; } Does this do the right thing if len is NaN? https://codereview.appspot.com/124480043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:3297: splice.apply(this, concat.call([0, 1], [])); capturing again https://codereview.appspot.com/124480043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:3726: function arrayMutatorProblem(destination, prop, testArgs) { The name arrayMutatorProblem is now too generic and ought to be changed; please add a TODO to note the need for a better one (I don't have any good ideas). https://codereview.appspot.com/124480043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:4747: arrayPutProblem(supportedProblems, Please add comments giving the computed names of these problems, as done for arrayMutatorProblem below. This allows searching the source file for a problem's definition.
Sign in to reply to this message.
https://codereview.appspot.com/124480043/diff/1/src/com/google/caja/ses/repai... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/124480043/diff/1/src/com/google/caja/ses/repai... src/com/google/caja/ses/repairES5.js:4573: preSeverity: severities.UNSAFE_SPEC_VIOLATION, On 2014/08/18 19:14:03, kpreid_google wrote: > On 2014/08/18 14:00:00, MarkM wrote: > > On all browsers, all tests that they initially fail are repaired successfully. > > OK, that's fine then. I didn't realize you were adding tests for problems beyond > the ones we're actually seeing. Acknowledged. https://codereview.appspot.com/124480043/diff/20001/src/com/google/caja/ses/r... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/124480043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:3283: splice.apply(this, concat.call([0, 0], items)); On 2014/08/18 19:14:03, kpreid_google wrote: > In general, I think we shouldn't use a top-level captured function, but use one > you capture when the repair function executes. This ensures that in the future > if we have a repair affecting splice, they "layer" rather than bypassing each > other. > > (This still might be a bad result depending on which order the repairs execute > in, but I claim that this pattern is more robust in general.) Done. https://codereview.appspot.com/124480043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:3295: if (len <= 0) { return void 0; } On 2014/08/18 19:14:03, kpreid_google wrote: > Does this do the right thing if len is NaN? No. Good catch. Done. https://codereview.appspot.com/124480043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:3297: splice.apply(this, concat.call([0, 1], [])); On 2014/08/18 19:14:03, kpreid_google wrote: > capturing again Done. https://codereview.appspot.com/124480043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:3726: function arrayMutatorProblem(destination, prop, testArgs) { On 2014/08/18 19:14:03, kpreid_google wrote: > The name arrayMutatorProblem is now too generic and ought to be changed; please > add a TODO to note the need for a better one (I don't have any good ideas). Done. Rather than TODO, I changed it arraySealProblem. https://codereview.appspot.com/124480043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:4747: arrayPutProblem(supportedProblems, On 2014/08/18 19:14:03, kpreid_google wrote: > Please add comments giving the computed names of these problems, as done for > arrayMutatorProblem below. This allows searching the source file for a problem's > definition. Done.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/124480043/diff/40001/src/com/google/caja/ses/r... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/124480043/diff/40001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:3299: baseSplice.apply(this, baseConcat.call([0, 1], [])); Isn't this equivalent to baseSplice.call(this, 0, 1)?
Sign in to reply to this message.
https://codereview.appspot.com/124480043/diff/40001/src/com/google/caja/ses/r... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/124480043/diff/40001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:3299: baseSplice.apply(this, baseConcat.call([0, 1], [])); On 2014/08/18 21:03:38, kpreid_google wrote: > Isn't this equivalent to baseSplice.call(this, 0, 1)? Doh! Yes. Done.
Sign in to reply to this message.
|
