656828 - WeakMap: Prevent mutation of prototypical WeakMaps.

Status: RESOLVED DUPLICATE of bug 1055473

(Core :: JavaScript Engine, defect)

Description

[Mark S. Miller]

From https://bugzilla.mozilla.org/show_bug.cgi?id=547941#c108

AFAIK, there are only two differences between what Andreas implemented and what I'm planning to propose:

1) Dave Herman suggested (what became a separate) lookup method which is like get, but, if its first key argument isn't found, in returns the result of calling its second argument. (An earlier proposal had both behaviors in one get method, dynamically switched based on whether the 2nd argument was a function. I think this was too error prone.)

2) WeakMap.prototype.set(k,v) and WeakMap.prototype.delete(k) both mutate the WeakMap.prototype itself. Freezing it doesn't prevent this mutation since the mutable state is in internal properties, not freezable properties. Instead, we agreed that the WeakMap methods, when given a WeakMap.prototype object from any frame, should throw a TypeError rather than mutate it. 

At http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5shim.js I monkey patch the current FF6.01a Nightly implementation to fix problem #2. Search for "PATCH_MUTABLE_FROZEN_WEAKMAP_PROTO" on that page. As you can see, getting this right cross frame by monkey patching is needlessly expensive.

Comment 1

[Brendan Eich [:brendan]]

This needs attention, and the lookup issue is separate (and I think without a proposal for ES6 no longer wanted).

/be

Comment 2

[Brendan Eich [:brendan]]

See bug 797686, which if patched should fix this bug.

/be

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Part 1 - Add JSCLASS_PLAIN_PROTO and use it in Debugger.cpp and MapObject.cpp

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Part 2 - change WeakMap to use JSCLASS_PLAIN_PROTO, v1

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 669694 [details] [diff] [review]
Part 1 - Add JSCLASS_PLAIN_PROTO and use it in Debugger.cpp and MapObject.cpp

Review of attachment 669694 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsobj.cpp
@@ +3038,5 @@
>       * it uses WithProto::Given.  FIXME: Undo dependencies on this parentage
>       * [which already needs to happen for bug 638316], figure out nicer
>       * semantics for null-protoProto, and use createBlankPrototype.)
>       */
> +    RootedObject proto(cx, obj->global().createBlankPrototype(cx, clasp));

Erm, don't do this (or the singleton-removal below it).  protoProto is supposed to be actually used as the [[Prototype]] of the newly-created prototype object for the class being initialized.  You're just ignoring it and creating a prototype that chains unconditionally to Object.prototype.  I'm no fan of the API this method is used to implement, and we need to design something better, but probably we shouldn't be changing semantics like this before then.

::: js/src/vm/Debugger.cpp
@@ +4569,2 @@
>          return false;
> +    RootedAtom atom(cx, Atomize(cx, Debugger::jsclass.name, strlen(Debugger::jsclass.name)));

Er.  If we don't have cx->names().Debugger, we surely should!

@@ +4617,5 @@
> +    debuggerProto->setReservedSlot(Debugger::JSSLOT_DEBUG_ENV_PROTO, ObjectValue(*envProto));
> +
> +    RootedId id(cx, AtomToId(atom));
> +    RootedValue v(cx, ObjectValue(*debuggerCtor));
> +    return JSObject::defineGeneric(cx, obj, id, v, JS_PropertyStub, JS_StrictPropertyStub, 0);

Which would convert this to JSObject::defineProperty.

::: js/src/vm/GlobalObject.cpp
@@ +571,5 @@
>      JS_ASSERT(clasp != &ObjectClass);
>      JS_ASSERT(clasp != &FunctionClass);
>  
> +    if (clasp->flags & JSCLASS_PLAIN_PROTO)
> +        clasp = &ObjectClass;

Not opposed to this in principle, but.  It would be clearer at the internal class-init sites if we saw "plain" somewhere.  Any chance some sort of CreatePlainProto helper method might be used to make that more obvious, somehow?

::: js/src/vm/GlobalObject.h
@@ +449,5 @@
>   */
>  extern bool
>  LinkConstructorAndPrototype(JSContext *cx, JSObject *ctor, JSObject *proto);
>  
> +/* Define properties, then functions, on the given object. */

but but but tracing benefits!

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 669696 [details] [diff] [review]
Part 2 - change WeakMap to use JSCLASS_PLAIN_PROTO, v1

Review of attachment 669696 [details] [diff] [review]:
-----------------------------------------------------------------

Tests?

Comment 7

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> >       * it uses WithProto::Given.  FIXME: Undo dependencies on this parentage
> >       * [which already needs to happen for bug 638316], figure out nicer
> >       * semantics for null-protoProto, and use createBlankPrototype.)
> >       */
> > +    RootedObject proto(cx, obj->global().createBlankPrototype(cx, clasp));
> 
> Erm, don't do this (or the singleton-removal below it).

Changed to honor protoProto. That was a silly oversight.

I removed the comment above this line of code. It looked obsolete to me.

> Er.  If we don't have cx->names().Debugger, we surely should!

Added.

> > +    if (clasp->flags & JSCLASS_PLAIN_PROTO)
> > +        clasp = &ObjectClass;
> 
> Not opposed to this in principle, but.  It would be clearer at the internal
> class-init sites if we saw "plain" somewhere.  Any chance some sort of
> CreatePlainProto helper method might be used to make that more obvious,
> somehow?

I looked at this, but can't see a way to do it that wouldn't add redundant code. Wherever it factors to is the right place for it, right?

Sorry if I'm being dense -- I'll be happy to change it if you have a specific idea.

Comment 8

[Jason Orendorff [:jorendorff]]

These patches make the try server very angry.

  https://tbpl.mozilla.org/?tree=Try&rev=23c72ce697c4

Looking into it with a local build.

Comment 9

[Jason Orendorff [:jorendorff]]

I think the implementation is just wrong. Now that there's a spec I can make a better patch. I can do this today.

Comment 10

[Mark S. Miller]

The ES6 spec now clearly states that WeakMap.prototype should be (in ES5 terms) of [[Class]] "Object" rather than [[Class]] "WeakMap" and should not have the internal properties expected of WeakMap instances. Doing so would also fix this bug and remove significant overhead from the SES-on-FF implementation.

Comment 11

[Mark S. Miller]

When should we expect WeakMap.prototype to be of [[Class]] "Object" per ES6?

Comment 12

[Claude Pache]

Dupe of Bug 1055473 ?

Comment 13

[Tom S]

I think so.