688277 - add way to examine WeakMap keys in browser tests

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

WeakMaps are designed to not expose GC behavior, but in order to test the interaction of the GC/CC with WeakMaps, we need a way to examine their bindings, to make sure things are freed when they should be.  The prototype v8 implementation uses C++ unit tests, but exposing this to JS for tests would be nice for fuzzing etc.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: add a function to return the keys of a weak map. WIP.

Comment 3

[Andrew McCreight [:mccr8]]

My current plan is to take the low level NondeterministicGetWeakMapKeys and wrap it up with schedulePreciseGC and some GC+CC'ing to produce a deterministic function that takes a callback.  We wait until JS has an empty stack, do some GC+CC, then pass the keys to the callback.

Comment 4

[Andrew McCreight [:mccr8]]

Using this and schedulePreciseGC you can write tests to make sure that weak maps are actually weak:

  let m = WeakMap();
  let liveKeys = new Array();

  let add_elements = function () {
    let k1 = {};
    m.set(k1, "live1");
    liveKeys.push(k1);
    let k2 = {};
    m.set(k2, "dead1");
    let k = {};
    m.set(k, k); // simple loop
  };

  add_elements();

  Components.utils.schedulePreciseGC(function () {
    let Ci = Components.interfaces;
    let keys = window.QueryInterface(Ci.nsIInterfaceRequestor)
      .getInterface(Ci.nsIDOMWindowUtils)
      .nondeterministicGetWeakMapKeys(m);
    is(liveKeys.length, 1, "Wrong number of live keys.");
    is(keys.length, 1, "Should have one weak map key.");
    is(m.get(keys[0]), "live1", "live1 should be live");
    SimpleTest.finish();
  });
  SimpleTest.waitForExplicitFinish();

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 1: add JS friend API for getting weak map keys, in a JS array

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 2: expose the keys function to JS, plus a test

Part 1 adds a new JS friend API that takes an object that is a weak map and pushes all keys into an array, which it returns.  For non-weak maps, it returned undefined.

Part 2 exposes this to JS, in Components.utils, which is only usable from Chrome or something like that.  This part also adds some JS tests, including some basic unit tests, and a few simple other tests.  I think this is the first time we test that WeakMaps are actually Weak, eg, that if the key goes away, the entry goes away.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 2: expose the keys function to JS, plus a test

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: part 1: add JS friend API for getting weak map keys, in a JS array

Comment 9

[Andrew McCreight [:mccr8]]

Part 1: This adds a new C++ function that returns the list of keys of a WeakMap, as a JS array.  We only expose this functionality to write tests of the interaction of the GC/CC and weak maps, so it is only used for the particular case of weak maps that are exposed to JS.  That's why I used a friend function instead of just exposing it in the class.  I made a typedef public to keep the type of the iterator from being insanely huge.

Part 2: This exposes the C++ function from part 1 to JS, in Components.utils, which should only be usable by Chrome.  We handle a few error cases by returning undefined.  This part also includes some basic tests of the new function, as well as the first test in the test suite that checks that WeakMap entries for dead keys are actually cleared by a GC.

The test also provides a nice example of how to use schedulePreciseGC to write a test involving weak maps.

Comment 10

[Andrew McCreight [:mccr8]]

Linux Debug looks okay on Try: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=953d406d1b09

I'll do a complete Try run before pushing, in case of weird link errors or whatever.

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 562516 [details] [diff] [review]
part 1: add JS friend API for getting weak map keys, in a JS array

I'm going to update the patch to address some comments from mrbkap.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 562480 [details] [diff] [review]
part 2: expose the keys function to JS, plus a test

I talked to Andrew on IRC about cleaning up the error handling from the new function. He'll change the API to reflect that the function can fail in a way that will throw an exception.

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: part 2: expose the keys function to JS, plus a test

Fixed up, per discussion with Blake.  The JS API function returns a boolean, in addition to a JS Object*.  If the API function returns false, then we're OOM.  We "throw" an OOM error on the JS side and on the XPConnect error.  If the API function return true, then the argument still could be junky (not a weak map object), but in that case we just silently return undefined.

Comment 14

[Andrew McCreight [:mccr8]]

Attachment: part 1: add JS friend API for getting weak map keys, in a JS array

Comment 15

[Andrew McCreight [:mccr8]]

jorendorff: review ping.  Let me know if you don't have time this week and I can find somebody else.  Thanks.

Comment 16

[Jason Orendorff [:jorendorff]]

Comment on attachment 562818 [details] [diff] [review]
part 1: add JS friend API for getting weak map keys, in a JS array


In jsweakmap.cpp:
>+    JSObject *arr = NewDenseEmptyArray(cx);
>+    if (!arr) {
>+        *ret = NULL;
>+        JS_ReportOutOfMemory(cx);
>+        return false;

On failure, NewDenseEmptyArray reports an error, so this JS_ReportOutOfMemory call is unnecessary.

I think it would be a little better not to assign to *ret, just as a matter of consistency. We often leave out params untouched on failure.

>+        for (ObjectValueMap::Base::Range r = map->all(); !r.empty(); r.popFront())
>+            js_NewbornArrayPush(cx, arr, ObjectValue(*r.front().key));

Check the return value.

In jsweakmap.h:
> template <class Key, class Value,
>           class HashPolicy = DefaultHasher<Key>,
>           class MarkPolicy = DefaultMarkPolicy<Key, Value> >
> class WeakMap : public HashMap<Key, Value, HashPolicy, RuntimeAllocPolicy>, public WeakMapBase {
>+  public:
>+    // This typedef is public so friend classes have a nice abbreviation for the base.
>+    typedef HashMap<Key, Value, HashPolicy, RuntimeAllocPolicy> Base;
>   private:
>-    typedef HashMap<Key, Value, HashPolicy, RuntimeAllocPolicy> Base;
>     typedef typename Base::Range Range;
>     typedef typename Base::Enum Enum;
> 
>+    // For writing invasive WeakMap tests.
>+    friend JSObject *JS_NondeterministicGetWeakMapKeys(JSContext *cx, JSObject *obj);

Are both changes necessary? I think if JS_N28s is a friend, then Base
doesn't need to be public.

There is another place where we walk WeakMap entries; that's in
vm/Debugger.cpp, Debugger::markKeysInCompartment. I would be OK with
giving WeakMap this instead; not sure if it would actually work:
    using Base::Range;
    Range nondeterministicAll() { return all(); }

Then JS_N28s wouldn't even need to be a friend. But whatever seems best
to you is fine with me.

r=me with those changes.

Comment 17

[Andrew McCreight [:mccr8]]

Thanks for the comments!

(In reply to Jason Orendorff [:jorendorff] from comment #16)
> >+    // For writing invasive WeakMap tests.
> >+    friend JSObject *JS_NondeterministicGetWeakMapKeys(JSContext *cx, JSObject *obj);
> 
> Are both changes necessary? I think if JS_N28s is a friend, then Base
> doesn't need to be public.

If I make Base private then I get this error when I try to use Base:

error: ‘typedef class js::HashMap<JSObject*, JS::Value, js::DefaultHasher<JSObject*>, js::RuntimeAllocPolicy> js::WeakMap<JSObject*, JS::Value, js::DefaultHasher<JSObject*>, js::DefaultMarkPolicy<JSObject*, JS::Value> >::Base’ is private

I can replace Base with that whole gigantic template type and it works, but it is pretty awful looking.

> There is another place where we walk WeakMap entries; that's in
> vm/Debugger.cpp, Debugger::markKeysInCompartment. I would be OK with
> giving WeakMap this instead; not sure if it would actually work:
>     using Base::Range;
>     Range nondeterministicAll() { return all(); }
> 
> Then JS_N28s wouldn't even need to be a friend. But whatever seems best
> to you is fine with me.

Yeah, that might make more sense.  There's much more control on the C++ side about when GCs happen, so the behavior is less likely to be weird.  (and people can step around it anyways as you demonstrated in Debugger)  I'll look into that.

Comment 18

[Andrew McCreight [:mccr8]]

Attachment: part 1: add JS friend API for getting weak map keys, in a JS array

Carrying forward jorendorff's r+.  Addressed his review comments.  I went with just adding a nondeterministicAll() function.  I added a public typedef for Range, instead of Base.  I'm going to do a full try server run before I actually push the patch.

Comment 19

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f4ab23e499
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fca2f2f79a3

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f5f4ab23e499
https://hg.mozilla.org/mozilla-central/rev/8fca2f2f79a3