The default bug view has changed. See this FAQ.

add way to examine WeakMap keys in browser tests

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 561806 [details] [diff] [review]
add a function to return the keys of a weak map.  WIP.
Assignee: general → continuation
(Assignee)

Updated

6 years ago
Duplicate of this bug: 686114
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
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();
(Assignee)

Comment 5

6 years ago
Created attachment 562156 [details] [diff] [review]
part 1: add JS friend API for getting weak map keys, in a JS array
Attachment #561806 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
Created attachment 562158 [details] [diff] [review]
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.
(Assignee)

Comment 7

6 years ago
Created attachment 562480 [details] [diff] [review]
part 2: expose the keys function to JS, plus a test
Attachment #562158 - Attachment is obsolete: true
(Assignee)

Comment 8

6 years ago
Created attachment 562516 [details] [diff] [review]
part 1: add JS friend API for getting weak map keys, in a JS array
Attachment #562156 - Attachment is obsolete: true
(Assignee)

Comment 9

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #562516 - Flags: review?(jorendorff)
(Assignee)

Updated

6 years ago
Attachment #562480 - Flags: review?(mrbkap)
(Assignee)

Comment 10

6 years ago
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.
(Assignee)

Comment 11

6 years ago
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.
Attachment #562516 - Flags: review?(jorendorff)
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.
Attachment #562480 - Flags: review?(mrbkap)
(Assignee)

Comment 13

6 years ago
Created attachment 562815 [details] [diff] [review]
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.
Attachment #562480 - Attachment is obsolete: true
Attachment #562815 - Flags: review?(mrbkap)
(Assignee)

Comment 14

6 years ago
Created attachment 562818 [details] [diff] [review]
part 1: add JS friend API for getting weak map keys, in a JS array
Attachment #562516 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #562818 - Flags: review?(jorendorff)

Updated

6 years ago
Attachment #562815 - Flags: review?(mrbkap) → review+
(Assignee)

Updated

6 years ago
Summary: add way to examine WeakMap bindings in the browser → add way to examine WeakMap keys in browser tests
(Assignee)

Comment 15

6 years ago
jorendorff: review ping.  Let me know if you don't have time this week and I can find somebody else.  Thanks.
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.
Attachment #562818 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 17

6 years ago
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.
(Assignee)

Comment 18

6 years ago
Created attachment 566697 [details] [diff] [review]
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.
Attachment #562818 - Attachment is obsolete: true
Attachment #566697 - Flags: review+
(Assignee)

Comment 19

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f4ab23e499
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fca2f2f79a3
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/f5f4ab23e499
https://hg.mozilla.org/mozilla-central/rev/8fca2f2f79a3
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.