No new snapshot, but need some input on which direction to go (see both
long comments).
https://codereview.appspot.com/44260044/diff/1/src/com/google/caja/ses/WeakMap.js
File src/com/google/caja/ses/WeakMap.js (right):
https://codereview.appspot.com/44260044/diff/1/src/com/google/caja/ses/WeakMap.js#newcode157
src/com/google/caja/ses/WeakMap.js:157: // IE 11 bug: WeakMaps silently
fail to store frozen objects.
On 2013/12/20 01:00:26, MarkM wrote:
For this and the other tests in this file that check for expected
patterns of
breakage, does it make any sense to place the tests themselves in
repairES5
and just check the outcome of the tests here?
Yes, it very much does; I am uncomfortable with these ad-hoc tests. The
problem is that repairES5 cannot _perform the repair_ and so it would
have to either inappropriately consider the problem safe (which it
isn't; this is very much an UNSAFE_SPEC_VIOLATION except insofar as
there is not yet any ratified specification for WeakMap) or consider it
unsafe and then end up aborting due to ses.ok checks.
We have accumulated other cases similar to this, like the repairs that
can't be verified until startSES has frozen things. I propose to leave
this alone for now and then _in a separate change_, refactor repairES5
to enable these sorts of operations.
Specifically, repairES5 will be able to understand repairs and tests
being performed after its own top-level execution has finished — so e.g.
WeakMap.js can perform the repair of the problem repairES5 detected, and
then when we're all done we can ask repairES5 to re-run a full set of
tests.
https://codereview.appspot.com/44260044/diff/1/src/com/google/caja/ses/WeakMap.js#newcode165
src/com/google/caja/ses/WeakMap.js:165: return;
On 2013/12/20 01:00:26, MarkM wrote:
codereview is rendering a red dot in the first column of this line,
but I have
no idea what this means. Is this something other than a plain ascii
space?
It's a tab, rendered as 8 spaces. My fault, was using an
insufficiently-configured emacs to edit this. Fixed.
https://codereview.appspot.com/44260044/diff/1/src/com/google/caja/ses/WeakMap.js#newcode581
src/com/google/caja/ses/WeakMap.js:581: if (!hmap.has(key)) {
On 2013/12/20 01:00:26, MarkM wrote:
What does this do on IE11:
var k = {};
hmap.set(k, 1);
Object.freeze(k);
hmap.set(k, 2);
alert(hmap.get(k));
If it alerts 1 because the second set silently failed, then your code
won't
work, since the has test above will be fooled into thinking the set
worked.
Good question! I've tested the above case, as well as deleting and
readding k or putting k in a different WeakMap, and the behavior seems
to be that an object can live in a WeakMap iff it has been put in any
WeakMap before it has been frozen (so the above code is good).
Also, preventExtensions triggers the same behavior.
This suggests that we can perform a purely-in-repairES5 repair instead:
patch freeze and preventExtensions to insert every object in a dummy
WeakMap before actually freezing. This seems like a superior approach to
the current one.
I wonder which way would have better performance — having every object
having been in a WeakMap _might_ trigger some kind of permanent
slow-path behavior (possibly affecting GC traversal) of the object.
Do you think we should pursue the alternative or commit this now to get
things working on trunk?
https://codereview.appspot.com/44260044/
--
---
You received this message because you are subscribed to the Google Groups "Google Caja Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
