Issue 12801043: Whitelist/tame Typed Arrays and use them in 2D Canvas taming. (Closed)
|
Created:
12 years, 7 months ago by kpreid2 Modified:
12 years, 4 months ago CC:
google-caja-discuss_googlegroups.com Base URL:
http://google-caja.googlecode.com/svn/trunk/ Visibility:
Public. |
Description* Whitelist typed-array objects in SES:
* ArrayBuffer
* Int8Array
* Uint8Array
* Uint8ClampedArray
* Int16Array
* Uint16Array
* Int32Array
* Uint32Array
* Float32Array
* Float64Array
* DataView
* Taming membrane copies such objects. Note that this copying
breaks sharing of ArrayBuffers among ArrayBufferViews.
* ImageData's data property is now an Uint8ClampedArray as is standard
rather than a plain array. Byte-by-byte copying is no longer used.
* Repair one DataView method throwing DOMException on Firefox and
*Array.prototype.set throwing DOMException on Safari.
* Repair typed array prototypes appearing unfrozen on Safari.
@r5621
Patch Set 1 #
Total comments: 10
Patch Set 2 : Whitelist/tame Typed Arrays and use them in 2D Canvas taming. #Patch Set 3 : Whitelist/tame Typed Arrays and use them in 2D Canvas taming. #
Total comments: 3
Patch Set 4 : Whitelist/tame Typed Arrays and use them in 2D Canvas taming. #
Total comments: 3
Patch Set 5 : Whitelist/tame Typed Arrays and use them in 2D Canvas taming. #Patch Set 6 : Whitelist/tame Typed Arrays and use them in 2D Canvas taming. #
Total comments: 6
Patch Set 7 : Whitelist/tame Typed Arrays and use them in 2D Canvas taming. #
Total comments: 2
Patch Set 8 : Whitelist/tame Typed Arrays and use them in 2D Canvas taming. #
Total comments: 6
Patch Set 9 : Whitelist/tame Typed Arrays and use them in 2D Canvas taming. #
Total comments: 3
Patch Set 10 : Whitelist/tame Typed Arrays and use them in 2D Canvas taming. #
MessagesTotal messages: 35
ses/* stuff LGTM. I am not expert on either typed arrays or DOMExceptions, so my LGTM should be interpreted accordingly. https://codereview.appspot.com/12801043/diff/1/src/com/google/caja/ses/repair... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/12801043/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:2617: new DataView(new ArrayBuffer(1)).getInt8(-1); Should we also be testing what a[-1] indexing does? (Obviously, if it does something bad we can't repair it. But at least we'd know.) https://codereview.appspot.com/12801043/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:4069: // DOMException is poisonous to WeakMaps on FF so we choose not to Do we test for that? What bug is filed for that? Is it worth case splitting on whether DOMException can be safe on a given browser? https://codereview.appspot.com/12801043/diff/1/src/com/google/caja/ses/whitel... File src/com/google/caja/ses/whitelist.js (right): https://codereview.appspot.com/12801043/diff/1/src/com/google/caja/ses/whitel... src/com/google/caja/ses/whitelist.js:449: length: t, // does not inherit Function on Chrome inherit from Function. That's weird and worth filing a bug on. Are these currently Chrome issues or v8 issues? Is any of this typed array stuff available in v8 outside the browser (e.g., NodeJS)? https://codereview.appspot.com/12801043/diff/1/src/com/google/caja/ses/whitel... src/com/google/caja/ses/whitelist.js:453: byteLength: 'accessor', From the description of "accessor" in the doccomment at the top of this file, on browsers where these are instead data properties, this will not whitelist them. Is that what you want? What browsers have you tested this on? https://codereview.appspot.com/12801043/diff/1/src/com/google/caja/ses/whitel... src/com/google/caja/ses/whitelist.js:458: length: t, // does not inherit Function on Chrome likewise https://codereview.appspot.com/12801043/diff/1/src/com/google/caja/ses/whitel... src/com/google/caja/ses/whitelist.js:480: length: t, // does not inherit Function on Chrome Likewise
Sign in to reply to this message.
* Whitelist typed-array objects in ES5/3 and SES: * ArrayBuffer * Int8Array * Uint8Array * Uint8ClampedArray * Int16Array * Uint16Array * Int32Array * Uint32Array * Float32Array * Float64Array * DataView * Taming membrane copies such objects. Note that this copying breaks sharing of ArrayBuffers among ArrayBufferViews. * ImageData's data property is now an Uint8ClampedArray as is standard rather than a plain array. Byte-by-byte copying is no longer used. * Repair one DataView method throwing DOMException on Firefox. Supporting changes: * es53.js supports whitelisting native accessor properties. * Remove unused function isError from es53.js.
Sign in to reply to this message.
https://codereview.appspot.com/12801043/diff/1/src/com/google/caja/ses/repair... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/12801043/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:2617: new DataView(new ArrayBuffer(1)).getInt8(-1); On 2013/08/12 21:05:24, MarkM wrote: > Should we also be testing what a[-1] indexing does? (Obviously, if it does > something bad we can't repair it. But at least we'd know.) DataView has no numeric indexing; it is specifically for accessing heterogeneous data, so the answers depend on the type specified by the choice of method. https://codereview.appspot.com/12801043/diff/1/src/com/google/caja/ses/repair... src/com/google/caja/ses/repairES5.js:4069: // DOMException is poisonous to WeakMaps on FF so we choose not to On 2013/08/12 21:05:24, MarkM wrote: > Do we test for that? What bug is filed for that? If I recall correctly, this is another instance of Firefox's WeakMaps not accepting some objects (the original problem was event objects). Or it might be our emulated WeakMaps — I don't quite recall. > Is it worth case splitting on whether DOMException can be safe on a given > browser? No, because the future is Typed Arrays as specified in ES6, which naturally do not throw DOMException. This is also consistent with https://codereview.appspot.com/12694044/ which passes DOMExceptions across the membrane losing their type (it is not possible to do otherwise). https://codereview.appspot.com/12801043/diff/1/src/com/google/caja/ses/whitel... File src/com/google/caja/ses/whitelist.js (right): https://codereview.appspot.com/12801043/diff/1/src/com/google/caja/ses/whitel... src/com/google/caja/ses/whitelist.js:449: length: t, // does not inherit Function on Chrome On 2013/08/12 21:05:24, MarkM wrote: > inherit from Function. > That's weird and worth filing a bug on. This is a general tendency of objects whose implementation is "DOM API" rather than "JavaScript". I believe it will go away when implementations follow the lead of ES6. > Is any of this typed array stuff available in v8 outside the browser > (e.g., NodeJS)? Yes, node.js supports typed arrays. https://codereview.appspot.com/12801043/diff/1/src/com/google/caja/ses/whitel... src/com/google/caja/ses/whitelist.js:453: byteLength: 'accessor', On 2013/08/12 21:05:24, MarkM wrote: > From the description of "accessor" in the doccomment at the top of this file, on > browsers where these are instead data properties, this will not whitelist them. > Is that what you want? On browsers where they are data properties, they are also on instances rather than prototypes. Therefore the whitelist does not affect them. > What browsers have you tested this on? Firefox, Chrome, and Safari as of now. ...and we have a bigger problem: typeof ArrayBuffer === 'object' on Safari, and ES5/3 specifically rejects marking a non-function as a function. This change may have to wait until that bug is fixed or ES5/3 is gone. Or, we could have a hook to override the typeof check in the finite set of problematic cases. https://bugs.webkit.org/show_bug.cgi?id=114457 might be the relevant report.
Sign in to reply to this message.
* Whitelist typed-array objects in SES: * ArrayBuffer * Int8Array * Uint8Array * Uint8ClampedArray * Int16Array * Uint16Array * Int32Array * Uint32Array * Float32Array * Float64Array * DataView * Taming membrane copies such objects. Note that this copying breaks sharing of ArrayBuffers among ArrayBufferViews. * ImageData's data property is now an Uint8ClampedArray as is standard rather than a plain array. Byte-by-byte copying is no longer used. * Repair one DataView method throwing DOMException on Firefox and *Array.prototype.set throwing DOMException on Safari. * Repair typed array prototypes appearing unfrozen on Safari.
Sign in to reply to this message.
New snapshot. ES5/3 support is gone, of course, and there's a new hairy repair for a Safari bug where prototypes are frozen but look like they aren't.
Sign in to reply to this message.
https://codereview.appspot.com/12801043/diff/12001/src/com/google/caja/ses/wh... File src/com/google/caja/ses/whitelist.js (right): https://codereview.appspot.com/12801043/diff/12001/src/com/google/caja/ses/wh... src/com/google/caja/ses/whitelist.js:447: length: t, // does not inherit Function on Chrome Should that be "does not inherit from Function.prototype on Chrome"? (and below)
Sign in to reply to this message.
* Whitelist typed-array objects in SES: * ArrayBuffer * Int8Array * Uint8Array * Uint8ClampedArray * Int16Array * Uint16Array * Int32Array * Uint32Array * Float32Array * Float64Array * DataView * Taming membrane copies such objects. Note that this copying breaks sharing of ArrayBuffers among ArrayBufferViews. * ImageData's data property is now an Uint8ClampedArray as is standard rather than a plain array. Byte-by-byte copying is no longer used. * Repair one DataView method throwing DOMException on Firefox and *Array.prototype.set throwing DOMException on Safari. * Repair typed array prototypes appearing unfrozen on Safari.
Sign in to reply to this message.
https://codereview.appspot.com/12801043/diff/12001/src/com/google/caja/ses/wh... File src/com/google/caja/ses/whitelist.js (right): https://codereview.appspot.com/12801043/diff/12001/src/com/google/caja/ses/wh... src/com/google/caja/ses/whitelist.js:447: length: t, // does not inherit Function on Chrome On 2013/10/09 22:22:05, MarkM wrote: > Should that be "does not inherit from Function.prototype on Chrome"? (and below) I don't see that we need to be that precise, but done.
Sign in to reply to this message.
https://codereview.appspot.com/12801043/diff/12001/src/com/google/caja/ses/re... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/12801043/diff/12001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:3421: origMethod.apply(this, arguments); Having a forward reference to a var declaration appearing later in the function is way too hard on readers. Please declare origMethod above the try.
Sign in to reply to this message.
https://codereview.appspot.com/12801043/diff/19001/src/com/google/caja/ses/re... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/12801043/diff/19001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:2774: } else if (e instanceof Error && typeof DOMException !== 'undefined' && If there is no DOMException, as one would expect outside the browser, and e is instanceof Error, then this test should return false. Perhaps (e instanceof Error && (typeof DOMException === 'underfined' || !(e instanceof DOMException)))
Sign in to reply to this message.
https://codereview.appspot.com/12801043/diff/19001/src/com/google/caja/ses/re... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/12801043/diff/19001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:2774: } else if (e instanceof Error && typeof DOMException !== 'undefined' && On 2013/10/09 22:37:11, MarkM wrote: > If there is no DOMException, as one would expect outside the browser, and > e is instanceof Error, then this test should return false. If there is no DOMException, this test is almost meaningless. Changed to simply exit early. (The more general version of this test would be "threw an error whose [[Prototype]] is not in the SES whitelist", but that would be more hairy layer-mixing.)
Sign in to reply to this message.
https://codereview.appspot.com/12801043/diff/19001/src/com/google/caja/ses/re... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/12801043/diff/19001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:2774: } else if (e instanceof Error && typeof DOMException !== 'undefined' && On 2013/10/09 23:25:46, kpreid2 wrote: > On 2013/10/09 22:37:11, MarkM wrote: > > If there is no DOMException, as one would expect outside the browser, and > > e is instanceof Error, then this test should return false. > > If there is no DOMException, this test is almost meaningless. Changed to simply > exit early. I don't see the change. Forget to snapshot?
Sign in to reply to this message.
* Whitelist typed-array objects in SES: * ArrayBuffer * Int8Array * Uint8Array * Uint8ClampedArray * Int16Array * Uint16Array * Int32Array * Uint32Array * Float32Array * Float64Array * DataView * Taming membrane copies such objects. Note that this copying breaks sharing of ArrayBuffers among ArrayBufferViews. * ImageData's data property is now an Uint8ClampedArray as is standard rather than a plain array. Byte-by-byte copying is no longer used. * Repair one DataView method throwing DOMException on Firefox and *Array.prototype.set throwing DOMException on Safari. * Repair typed array prototypes appearing unfrozen on Safari.
Sign in to reply to this message.
On 2013/10/10 00:04:12, MarkM wrote: > I don't see the change. Forget to snapshot? Oops, done.
Sign in to reply to this message.
On 2013/10/10 00:05:31, kpreid2 wrote: > On 2013/10/10 00:04:12, MarkM wrote: > > I don't see the change. Forget to snapshot? > > Oops, done. I still don't see it
Sign in to reply to this message.
* Whitelist typed-array objects in SES: * ArrayBuffer * Int8Array * Uint8Array * Uint8ClampedArray * Int16Array * Uint16Array * Int32Array * Uint32Array * Float32Array * Float64Array * DataView * Taming membrane copies such objects. Note that this copying breaks sharing of ArrayBuffers among ArrayBufferViews. * ImageData's data property is now an Uint8ClampedArray as is standard rather than a plain array. Byte-by-byte copying is no longer used. * Repair one DataView method throwing DOMException on Firefox and *Array.prototype.set throwing DOMException on Safari. * Repair typed array prototypes appearing unfrozen on Safari.
Sign in to reply to this message.
On 2013/10/10 00:07:54, MarkM wrote: > I still don't see it ...OK, now it actually is there. I checked. (I should figure out how to make this less than a three-step process...)
Sign in to reply to this message.
LGTM https://codereview.appspot.com/12801043/diff/32001/src/com/google/caja/ses/re... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/12801043/diff/32001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:3436: var repair_TYPED_ARRAY_PROTOS_LOOK_UNFROZEN_wasApplied = false; Since the test occurs first textually, please declare this variable above the test instead. https://codereview.appspot.com/12801043/diff/32001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:3454: // is used without startSES.) More weird but more safe: Add a ses.flag that startSES can use to communicate back to repairES5. After startSES freezes the primordials, it sets the flag. Here, the wrapper function would report freeze or not depending on whether the flag was set. Also for isFrozen below. https://codereview.appspot.com/12801043/diff/32001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:4501: // DOMException is poisonous to WeakMaps on FF so we choose not to I don't understand this comment.
Sign in to reply to this message.
* Whitelist typed-array objects in SES: * ArrayBuffer * Int8Array * Uint8Array * Uint8ClampedArray * Int16Array * Uint16Array * Int32Array * Uint32Array * Float32Array * Float64Array * DataView * Taming membrane copies such objects. Note that this copying breaks sharing of ArrayBuffers among ArrayBufferViews. * ImageData's data property is now an Uint8ClampedArray as is standard rather than a plain array. Byte-by-byte copying is no longer used. * Repair one DataView method throwing DOMException on Firefox and *Array.prototype.set throwing DOMException on Safari. * Repair typed array prototypes appearing unfrozen on Safari.
Sign in to reply to this message.
This change still needs review of its not-SES parts, I assume. https://codereview.appspot.com/12801043/diff/32001/src/com/google/caja/ses/re... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/12801043/diff/32001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:3436: var repair_TYPED_ARRAY_PROTOS_LOOK_UNFROZEN_wasApplied = false; On 2013/10/11 00:17:49, MarkM wrote: > Since the test occurs first textually, please declare this variable above the > test instead. Done. https://codereview.appspot.com/12801043/diff/32001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:3454: // is used without startSES.) On 2013/10/11 00:17:49, MarkM wrote: > More weird but more safe: > > Add a ses.flag that startSES can use to communicate back to repairES5. After > startSES freezes the primordials, it sets the flag. Here, the wrapper function > would report freeze or not depending on whether the flag was set. Also for > isFrozen below. Done. https://codereview.appspot.com/12801043/diff/32001/src/com/google/caja/ses/re... src/com/google/caja/ses/repairES5.js:4501: // DOMException is poisonous to WeakMaps on FF so we choose not to On 2013/10/11 00:17:49, MarkM wrote: > I don't understand this comment. Rewritten.
Sign in to reply to this message.
ses/* parts LGTM On 2013/10/15 22:57:33, kpreid2 wrote: > This change still needs review of its not-SES parts, I assume. yes
Sign in to reply to this message.
lgtm++ https://codereview.appspot.com/12801043/diff/33007/src/com/google/caja/plugin... File src/com/google/caja/plugin/taming-membrane.js (right): https://codereview.appspot.com/12801043/diff/33007/src/com/google/caja/plugin... src/com/google/caja/plugin/taming-membrane.js:220: // that's in line with our general policy of copying. Perhaps add a test or 2 of these cases, just for kix?
Sign in to reply to this message.
* Whitelist typed-array objects in SES: * ArrayBuffer * Int8Array * Uint8Array * Uint8ClampedArray * Int16Array * Uint16Array * Int32Array * Uint32Array * Float32Array * Float64Array * DataView * Taming membrane copies such objects. Note that this copying breaks sharing of ArrayBuffers among ArrayBufferViews. * ImageData's data property is now an Uint8ClampedArray as is standard rather than a plain array. Byte-by-byte copying is no longer used. * Repair one DataView method throwing DOMException on Firefox and *Array.prototype.set throwing DOMException on Safari. * Repair typed array prototypes appearing unfrozen on Safari.
Sign in to reply to this message.
https://codereview.appspot.com/12801043/diff/33007/src/com/google/caja/plugin... File src/com/google/caja/plugin/taming-membrane.js (right): https://codereview.appspot.com/12801043/diff/33007/src/com/google/caja/plugin... src/com/google/caja/plugin/taming-membrane.js:220: // that's in line with our general policy of copying. On 2013/10/18 19:15:07, ihab.awad wrote: > Perhaps add a test or 2 of these cases, just for kix? Done, and a good thing too — the code for ArrayBuffer and DataView was broken. Please have a look at whether the tests I've added are sufficient coverage (in particular, I tested only the guest-to-host case and not host-to-guest on the grounds that this is symmetric).
Sign in to reply to this message.
https://codereview.appspot.com/12801043/diff/70001/tests/com/google/caja/plug... File tests/com/google/caja/plugin/test-taming-inout-guest.js (right): https://codereview.appspot.com/12801043/diff/70001/tests/com/google/caja/plug... tests/com/google/caja/plugin/test-taming-inout-guest.js:189: tamedApi.tamedHostPureFunction('assertEquals("2", 3, a.byteLength);', buf); Was the assertion label supposed to be "3"? https://codereview.appspot.com/12801043/diff/70001/tests/com/google/caja/plug... tests/com/google/caja/plugin/test-taming-inout-guest.js:191: 'assertEquals("3", 2, new Uint8Array(a)[1]);', buf); Similar -- assertion label supposed to be "2"? https://codereview.appspot.com/12801043/diff/70001/tests/com/google/caja/plug... tests/com/google/caja/plugin/test-taming-inout-guest.js:200: 'assertNotEquals("5", a.buffer, b.buffer);', a, b); I don't understand assertion label "5". https://codereview.appspot.com/12801043/diff/70001/tests/com/google/caja/plug... tests/com/google/caja/plugin/test-taming-inout-guest.js:204: tamedApi.tamedHostPureFunction('assertEquals("not mutated", 1, b[0]);', a, b); I don't understand the invariant that the above 2 lines are trying to maintain. I'm even confused that they work! :) In line 201, we mutate a feral clone of "a". Then in line 203, we assert that a *distinct* feral clone of "a" -- created by a *distinct* pass through untame() -- still carries the mutation? In line 204, we assert that yet another *distinct* feral clone, of "b" this time, is *not* mutated. But should (feral clones of) "a" and "b" not behave exactly the same, since they share a buffer?
Sign in to reply to this message.
https://codereview.appspot.com/12801043/diff/70001/tests/com/google/caja/plug... File tests/com/google/caja/plugin/test-taming-inout-guest.js (right): https://codereview.appspot.com/12801043/diff/70001/tests/com/google/caja/plug... tests/com/google/caja/plugin/test-taming-inout-guest.js:189: tamedApi.tamedHostPureFunction('assertEquals("2", 3, a.byteLength);', buf); On 2013/10/22 22:40:48, ihab.awad wrote: > Was the assertion label supposed to be "3"? Sorry, those were placeholders to just be able to distinguish the asserts. All will have mutable labels in the next snapshot (not now due to unresolved design question below). https://codereview.appspot.com/12801043/diff/70001/tests/com/google/caja/plug... tests/com/google/caja/plugin/test-taming-inout-guest.js:204: tamedApi.tamedHostPureFunction('assertEquals("not mutated", 1, b[0]);', a, b); On 2013/10/22 22:40:48, ihab.awad wrote: > I don't understand the invariant that the above 2 lines are trying to > maintain. I'm even confused that they work! :) > > In line 201, we mutate a feral clone of "a". Then in line 203, we assert that > a *distinct* feral clone of "a" -- created by a *distinct* pass through > untame() -- still carries the mutation? I misread the original taming membrane code. I thought that untame() *always* entered the feral twins it constructs in the membrane (which is why the behavior tested here occurs), but I missed that there is a special case for Arrays. Should these be treated like Arrays and copied anew each time? For: - Users think of them as arrays, so this would be consistent - It's plausible that they might be mutated and re-passed (reuse of allocated storage), in which case we want to make copies with fresh contents Against: - Very large buffers might be passed and be slow to copy repeatedly; e.g. consider a 'parsing' API which returns many small slices of a large buffer - Copying each time would destroy all sharing that might be expected by the recipient; e.g. a buffer/array provided for the callee's use that's passed repeatedly rather than closed over. The main browser API that works with typed arrays is WebGL, and its uses are all of the sort "please grab a snapshot of this array contents and upload it to the GPU", so it contains no useful precedent for mutability/sharing choices. What do you think the membrane should do? > In line 204, we assert that yet another *distinct* feral clone, of "b" this > time, is *not* mutated. But should (feral clones of) "a" and "b" not behave > exactly the same, since they share a buffer? They don't share, because copyBuiltin copies the buffer when the array is copied. We could untame() the buffer and get the behavior you propose, unless we decide to copy every time as discussed above.
Sign in to reply to this message.
I recommend we copy every time, consistent with all other arrays. If we find later that we need to revisit, we can revisit for all arrays. The copying of arrays is an artifact of ES5/3 -- * ES5/3 forced us to always have distinct tame-side vs. feral-side objects because the tame-side objects must have the "___" properties and the feral-side objects should not be polluted by them; * ES5/3 did not allow us to create virtual properties on the feral side; and * Anyway, to properly virtualize an array, we'd need wildcard properties (proxies) rather than just individual property descriptors.
Sign in to reply to this message.
* Whitelist typed-array objects in SES: * ArrayBuffer * Int8Array * Uint8Array * Uint8ClampedArray * Int16Array * Uint16Array * Int32Array * Uint32Array * Float32Array * Float64Array * DataView * Taming membrane copies such objects. Note that this copying breaks sharing of ArrayBuffers among ArrayBufferViews. * ImageData's data property is now an Uint8ClampedArray as is standard rather than a plain array. Byte-by-byte copying is no longer used. * Repair one DataView method throwing DOMException on Firefox and *Array.prototype.set throwing DOMException on Safari. * Repair typed array prototypes appearing unfrozen on Safari.
Sign in to reply to this message.
On 2013/10/25 21:21:41, ihab.awad wrote: > I recommend we copy every time, consistent with all other arrays. If we find > later that we need to revisit, we can revisit for all arrays. Done; see new snapshot. Note a small change to the semantics of the taming membrane: in order to avoid testing for N kinds of objects before checking the feralByTame/tameByFeral maps, I have moved that check first. This means that if tamesTo is applied to an array object (on either side) it will now have an effect where it did not before. I see no harm in this. I have also refactored the taming membrane code to reduce duplication in array processing. > The copying of arrays is an artifact of ES5/3 -- > > * ES5/3 forced us to always have distinct tame-side vs. feral-side objects > because the tame-side objects must have the "___" properties and the feral-side > objects should not be polluted by them; Even in ES5+WeakMaps+proxies we still must have distinct feral-side objects: a guest function must not be given any feral object (due to mutable/powerful prototypes) and so the feral side must be given argument-taming wrappers for guest functions.
Sign in to reply to this message.
lgtm++ but please consider the comments. https://codereview.appspot.com/12801043/diff/170001/src/com/google/caja/plugi... File src/com/google/caja/plugin/taming-membrane.js (right): https://codereview.appspot.com/12801043/diff/170001/src/com/google/caja/plugi... src/com/google/caja/plugin/taming-membrane.js:172: * copyUnmemoized performs the analogous role. It's confusing to me that "memoized" versus "unmemoized" is used as a distinction in these functions even though memoization is dealt with elsewhere. I'd leave out the memoization stuff from comments and function names. https://codereview.appspot.com/12801043/diff/170001/src/com/google/caja/plugi... src/com/google/caja/plugin/taming-membrane.js:247: function copyUnmemoized(o, recursor) { Anything that does not mention memoization -- e.g. "copyBulkDataType" or "copyArrayAndFriends" or whatever -- would imho be better than introducing the idea of memoization here.
Sign in to reply to this message.
lgtm++ but please consider the comments. https://codereview.appspot.com/12801043/diff/170001/src/com/google/caja/plugi... File src/com/google/caja/plugin/taming-membrane.js (right): https://codereview.appspot.com/12801043/diff/170001/src/com/google/caja/plugi... src/com/google/caja/plugin/taming-membrane.js:172: * copyUnmemoized performs the analogous role. It's confusing to me that "memoized" versus "unmemoized" is used as a distinction in these functions even though memoization is dealt with elsewhere. I'd leave out the memoization stuff from comments and function names. https://codereview.appspot.com/12801043/diff/170001/src/com/google/caja/plugi... src/com/google/caja/plugin/taming-membrane.js:247: function copyUnmemoized(o, recursor) { Anything that does not mention memoization -- e.g. "copyBulkDataType" or "copyArrayAndFriends" or whatever -- would imho be better than introducing the idea of memoization here.
Sign in to reply to this message.
https://codereview.appspot.com/12801043/diff/170001/src/com/google/caja/plugi... File src/com/google/caja/plugin/taming-membrane.js (right): https://codereview.appspot.com/12801043/diff/170001/src/com/google/caja/plugi... src/com/google/caja/plugin/taming-membrane.js:247: function copyUnmemoized(o, recursor) { On 2013/10/25 22:53:12, ihab.awad wrote: > Anything that does not mention memoization -- e.g. "copyBulkDataType" > or "copyArrayAndFriends" or whatever -- would imho be better than > introducing the idea of memoization here. I don't particularly like copyUnmemoized myself, but I haven't come up with something clearly better here. The distinction we are actually making here is _builtin types which are likely to be mutated_, which are not memoized for that reason. (Also 'and which can contain references', but that's incidental.) The fact that they are both array-like is accidental. I have no good ideas for names which convey this distinction, and your suggestions are pointing at incidental rather than fundamental aspects. 'copyMutable' / 'copyImmutable' would be short and clear, but wrong because Date and friends are not immutable; we're just acting as if they are. How about 'copyTreatedMutable' / 'copyTreatedImmutable'?
Sign in to reply to this message.
I think "pick one and submit with confidence" would be my feedback at this point. :)
Sign in to reply to this message.
* Whitelist typed-array objects in SES: * ArrayBuffer * Int8Array * Uint8Array * Uint8ClampedArray * Int16Array * Uint16Array * Int32Array * Uint32Array * Float32Array * Float64Array * DataView * Taming membrane copies such objects. Note that this copying breaks sharing of ArrayBuffers among ArrayBufferViews. * ImageData's data property is now an Uint8ClampedArray as is standard rather than a plain array. Byte-by-byte copying is no longer used. * Repair one DataView method throwing DOMException on Firefox and *Array.prototype.set throwing DOMException on Safari. * Repair typed array prototypes appearing unfrozen on Safari.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
