author | Bill McCloskey <wmccloskey@mozilla.com> |
Mon, 08 Oct 2012 18:22:47 -0700 | |
changeset 110618 | 4ca5990b24d5654294d83ae25a428e58282cef5d |
parent 110617 | 2a96aeb11ad44ab6ac55c32a2bf177ad89dabb32 |
child 110619 | 1e5bf398fc887b518462e789526ee4159fdaf632 |
push id | 16640 |
push user | wmccloskey@mozilla.com |
push date | Thu, 18 Oct 2012 01:23:27 +0000 |
treeherder | mozilla-inbound@2f193f44cb99 [default view] [failures only] |
perfherder | [talos] [build metrics] [platform microbench] (compared to previous push) |
reviewers | mccr8 |
bugs | 798678 |
milestone | 19.0a1 |
first release with | nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
|
last release without | nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
|
new file mode 100644 --- /dev/null +++ b/js/src/jit-test/tests/basic/bug673468.js @@ -0,0 +1,8 @@ +var g = newGlobal(); +var k = g.eval('var u = new Object(); u'); +var m = new WeakMap(); +m.set(k, {}); +k = null; +gc(); +k = g.eval('u'); +assertEq(m.has(k), true);
--- a/js/src/jsapi.h +++ b/js/src/jsapi.h @@ -1878,16 +1878,19 @@ typedef void * a string describing the reference traced with JS_CallTracer. */ typedef void (* JSTraceNamePrinter)(JSTracer *trc, char *buf, size_t bufsize); typedef JSBool (* JSEqualityOp)(JSContext *cx, JSHandleObject obj, JSHandleValue v, JSBool *bp); +typedef JSRawObject +(* JSWeakmapKeyDelegateOp)(JSRawObject obj); + /* * Typedef for native functions called by the JS VM. * * See jsapi.h, the JS_CALLEE, JS_THIS, etc. macros. */ typedef JSBool (* JSNative)(JSContext *cx, unsigned argc, jsval *vp);
--- a/js/src/jsclass.h +++ b/js/src/jsclass.h @@ -250,19 +250,32 @@ struct ClassExtension JSIteratorOp iteratorObject; void *unused; /* * isWrappedNative is true only if the class is an XPCWrappedNative. * WeakMaps use this to override the wrapper disposal optimization. */ bool isWrappedNative; + + /* + * If an object is used as a key in a weakmap, it may be desirable for the + * garbage collector to keep that object around longer than it otherwise + * would. A common case is when the key is a wrapper around an object in + * another compartment, and we want to avoid collecting the wrapper (and + * removing the weakmap entry) as long as the wrapped object is alive. In + * that case, the wrapped object is returned by the wrapper's + * weakmapKeyDelegateOp hook. As long as the wrapper is used as a weakmap + * key, it will not be collected (and remain in the weakmap) until the + * wrapped object is collected. + */ + JSWeakmapKeyDelegateOp weakmapKeyDelegateOp; }; -#define JS_NULL_CLASS_EXT {NULL,NULL,NULL,NULL,NULL,false} +#define JS_NULL_CLASS_EXT {NULL,NULL,NULL,NULL,NULL,false,NULL} struct ObjectOps { LookupGenericOp lookupGeneric; LookupPropOp lookupProperty; LookupElementOp lookupElement; LookupSpecialOp lookupSpecial; DefineGenericOp defineGeneric;
--- a/js/src/jsproxy.cpp +++ b/js/src/jsproxy.cpp @@ -348,16 +348,22 @@ BaseProxyHandler::objectClassIs(JSObject return false; } void BaseProxyHandler::finalize(JSFreeOp *fop, JSObject *proxy) { } +JSObject * +BaseProxyHandler::weakmapKeyDelegate(JSObject *proxy) +{ + return NULL; +} + bool BaseProxyHandler::getPrototypeOf(JSContext *cx, JSObject *proxy, JSObject **proto) { // The default implementation here just uses proto of the proxy object. *proto = proxy->getTaggedProto().toObjectOrNull(); return true; } @@ -544,16 +550,22 @@ IndirectProxyHandler::iteratorNext(JSCon *vp = cx->iterValue; cx->iterValue = UndefinedValue(); } else { *vp = MagicValue(JS_NO_ITER_VALUE); } return true; } +JSObject * +IndirectProxyHandler::weakmapKeyDelegate(JSObject *proxy) +{ + return UnwrapObject(proxy); +} + DirectProxyHandler::DirectProxyHandler(void *family) : IndirectProxyHandler(family) { } bool DirectProxyHandler::has(JSContext *cx, JSObject *proxy, jsid id, bool *bp) { @@ -2852,29 +2864,35 @@ proxy_TraceFunction(JSTracer *trc, RawOb { // NB: If you add new slots here, make sure to change // js::NukeChromeCrossCompartmentWrappers to cope. MarkCrossCompartmentSlot(trc, &GetCall(obj), "call"); MarkSlot(trc, &GetFunctionProxyConstruct(obj), "construct"); proxy_TraceObject(trc, obj); } +static JSObject * +proxy_WeakmapKeyDelegate(RawObject obj) +{ + JS_ASSERT(obj->isProxy()); + return GetProxyHandler(obj)->weakmapKeyDelegate(obj); +} + static JSBool proxy_Convert(JSContext *cx, HandleObject proxy, JSType hint, MutableHandleValue vp) { JS_ASSERT(proxy->isProxy()); return Proxy::defaultValue(cx, proxy, hint, vp.address()); } static void proxy_Finalize(FreeOp *fop, RawObject obj) { JS_ASSERT(obj->isProxy()); - if (!obj->getSlot(JSSLOT_PROXY_HANDLER).isUndefined()) - GetProxyHandler(obj)->finalize(fop, obj); + GetProxyHandler(obj)->finalize(fop, obj); } static JSBool proxy_HasInstance(JSContext *cx, HandleObject proxy, MutableHandleValue v, JSBool *bp) { bool b; if (!Proxy::hasInstance(cx, proxy, v, &b)) return false; @@ -2884,33 +2902,44 @@ proxy_HasInstance(JSContext *cx, HandleO static JSType proxy_TypeOf(JSContext *cx, HandleObject proxy) { JS_ASSERT(proxy->isProxy()); return Proxy::typeOf(cx, proxy); } +#define PROXY_CLASS_EXT \ + { \ + NULL, /* equality */ \ + NULL, /* outerObject */ \ + NULL, /* innerObject */ \ + NULL, /* iteratorObject */ \ + NULL, /* unused */ \ + false, /* isWrappedNative */ \ + proxy_WeakmapKeyDelegate \ + } + JS_FRIEND_DATA(Class) js::ObjectProxyClass = { "Proxy", Class::NON_NATIVE | JSCLASS_IMPLEMENTS_BARRIERS | JSCLASS_HAS_RESERVED_SLOTS(4), JS_PropertyStub, /* addProperty */ JS_PropertyStub, /* delProperty */ JS_PropertyStub, /* getProperty */ JS_StrictPropertyStub, /* setProperty */ JS_EnumerateStub, JS_ResolveStub, proxy_Convert, proxy_Finalize, /* finalize */ NULL, /* checkAccess */ NULL, /* call */ proxy_HasInstance, /* hasInstance */ NULL, /* construct */ proxy_TraceObject, /* trace */ - JS_NULL_CLASS_EXT, + PROXY_CLASS_EXT, { proxy_LookupGeneric, proxy_LookupProperty, proxy_LookupElement, proxy_LookupSpecial, proxy_DefineGeneric, proxy_DefineProperty, proxy_DefineElement, @@ -2956,17 +2985,20 @@ JS_FRIEND_DATA(Class) js::OuterWindowPro NULL, /* call */ NULL, /* hasInstance */ NULL, /* construct */ proxy_TraceObject, /* trace */ { NULL, /* equality */ NULL, /* outerObject */ proxy_innerObject, - NULL /* unused */ + NULL, /* iteratorObject */ + NULL, /* unused */ + false, /* isWrappedNative */ + proxy_WeakmapKeyDelegate }, { proxy_LookupGeneric, proxy_LookupProperty, proxy_LookupElement, proxy_LookupSpecial, proxy_DefineGeneric, proxy_DefineProperty, @@ -3026,17 +3058,17 @@ JS_FRIEND_DATA(Class) js::FunctionProxyC JS_ResolveStub, JS_ConvertStub, proxy_Finalize, /* finalize */ NULL, /* checkAccess */ proxy_Call, FunctionClass.hasInstance, proxy_Construct, proxy_TraceFunction, /* trace */ - JS_NULL_CLASS_EXT, + PROXY_CLASS_EXT, { proxy_LookupGeneric, proxy_LookupProperty, proxy_LookupElement, proxy_LookupSpecial, proxy_DefineGeneric, proxy_DefineProperty, proxy_DefineElement,
--- a/js/src/jsproxy.h +++ b/js/src/jsproxy.h @@ -119,16 +119,19 @@ class JS_FRIEND_API(BaseProxyHandler) { virtual JSString *fun_toString(JSContext *cx, JSObject *proxy, unsigned indent); virtual bool regexp_toShared(JSContext *cx, JSObject *proxy, RegExpGuard *g); virtual bool defaultValue(JSContext *cx, JSObject *obj, JSType hint, Value *vp); virtual bool iteratorNext(JSContext *cx, JSObject *proxy, Value *vp); virtual void finalize(JSFreeOp *fop, JSObject *proxy); virtual bool getElementIfPresent(JSContext *cx, JSObject *obj, JSObject *receiver, uint32_t index, Value *vp, bool *present); virtual bool getPrototypeOf(JSContext *cx, JSObject *proxy, JSObject **protop); + + /* See comment for weakmapKeyDelegateOp in jsclass.h. */ + virtual JSObject *weakmapKeyDelegate(JSObject *proxy); }; /* * IndirectProxyHandler assumes that a target exists. Moreover, it assumes the * target is a JSObject. Consequently, it provides default implementations for * the fundamental traps that forward their behavior to the target. The derived * traps, however, are inherited from BaseProxyHandler, and therefore still * implemented in terms of the fundamental ones. This allows consumers of this @@ -171,16 +174,17 @@ class JS_PUBLIC_API(IndirectProxyHandler virtual JSString *fun_toString(JSContext *cx, JSObject *proxy, unsigned indent) MOZ_OVERRIDE; virtual bool regexp_toShared(JSContext *cx, JSObject *proxy, RegExpGuard *g) MOZ_OVERRIDE; virtual bool defaultValue(JSContext *cx, JSObject *obj, JSType hint, Value *vp) MOZ_OVERRIDE; virtual bool iteratorNext(JSContext *cx, JSObject *proxy, Value *vp) MOZ_OVERRIDE; + virtual JSObject *weakmapKeyDelegate(JSObject *proxy); }; /* * DirectProxyHandler has the same assumptions about the target as its base, * IndirectProxyHandler. Its fundamental traps are inherited from this class, * and therefore forward their behavior to the target. The derived traps, * however, are overrided so that, they too, forward their behavior to the * target. This allows consumers of this class to forward to another object as
--- a/js/src/jsweakmap.cpp +++ b/js/src/jsweakmap.cpp @@ -103,24 +103,17 @@ GetObjectMap(JSObject *obj) static JSObject * GetKeyArg(JSContext *cx, CallArgs &args) { Value *vp = &args[0]; if (vp->isPrimitive()) { JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_NONNULL_OBJECT); return NULL; } - JSObject &key = vp->toObject(); - - // If the key is from another compartment, and we store the wrapper as the key - // the wrapper might be GC-ed since it is not strong referenced (Bug 673468). - // To avoid this we always use the unwrapped object as the key instead of its - // security wrapper. This also means that if the keys are ever exposed they must - // be re-wrapped (see: JS_NondeterministicGetWeakMapKeys). - return JS_UnwrapObject(&key); + return &vp->toObject(); } JS_ALWAYS_INLINE bool IsWeakMap(const Value &v) { return v.isObject() && v.toObject().hasClass(&WeakMapClass); } @@ -246,17 +239,17 @@ WeakMap_set_impl(JSContext *cx, CallArgs JS_ReportOutOfMemory(cx); return false; } thisObj->setPrivate(map); } // Preserve wrapped native keys to prevent wrapper optimization. if (key->getClass()->ext.isWrappedNative) { - MOZ_ASSERT(cx->runtime->preserveWrapperCallback, "wrapped native weak map key needs preserveWrapperCallback"); + JS_ASSERT(cx->runtime->preserveWrapperCallback); if (!cx->runtime->preserveWrapperCallback(cx, key)) { JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_BAD_WEAKMAP_KEY); return false; } } if (!map->put(key, value)) { JS_ReportOutOfMemory(cx); @@ -283,22 +276,17 @@ JS_NondeterministicGetWeakMapKeys(JSCont return true; } RootedObject arr(cx, NewDenseEmptyArray(cx)); if (!arr) return false; ObjectValueMap *map = GetObjectMap(obj); if (map) { for (ObjectValueMap::Base::Range r = map->all(); !r.empty(); r.popFront()) { - RootedObject key(cx, r.front().key); - // Re-wrapping the key (see comment of GetKeyArg) - if (!JS_WrapObject(cx, key.address())) - return false; - - if (!js_NewbornArrayPush(cx, arr, ObjectValue(*key))) + if (!js_NewbornArrayPush(cx, arr, ObjectValue(*r.front().key))) return false; } } *ret = arr; return true; } static void
--- a/js/src/jsweakmap.h +++ b/js/src/jsweakmap.h @@ -22,52 +22,20 @@ namespace js { // a key is collected, the table entry disappears, dropping its reference to the value. // // More precisely: // // A WeakMap entry is collected if and only if either the WeakMap or the entry's key // is collected. If an entry is not collected, it remains in the WeakMap and it has a // strong reference to the value. // -// You must call this table's 'mark' method when the object of which it is a part is +// You must call this table's 'trace' method when the object of which it is a part is // reached by the garbage collection tracer. Once a table is known to be live, the // implementation takes care of the iterative marking needed for weak tables and removing // table entries when collection is complete. -// -// You may provide your own MarkPolicy class to specify how keys and values are marked; a -// policy template provides default definitions for some common key/value type -// combinations. -// -// Details: -// -// The interface is as for a js::HashMap, with the following additions: -// -// - You must call the WeakMap's 'trace' member function when you discover that the map is -// part of a live object. (You'll typically call this from the containing type's 'trace' -// function.) -// -// - There is no AllocPolicy parameter; these are used with our garbage collector, so -// RuntimeAllocPolicy is hard-wired. -// -// - Optional fourth and fifth parameters are the MarkPolicies for the key and value type. -// A MarkPolicy has the constructor: -// -// MarkPolicy(JSTracer *) -// -// and the following member functions: -// -// bool isMarked(const Type &x) -// Return true if x has been marked as live by the garbage collector. -// -// bool mark(Type &x) -// Return false if x is already marked. Otherwise, mark x and return true. -// -// If omitted, the MarkPolicy parameter defaults to js::DefaultMarkPolicy<Type>, -// a policy template with the obvious definitions for some typical -// SpiderMonkey type combinations. // The value for the next pointer for maps not in the map list. static WeakMapBase * const WeakMapNotInList = reinterpret_cast<WeakMapBase *>(1); typedef Vector<WeakMapBase *, 0, SystemAllocPolicy> WeakMapVector; // Common base class for all WeakMap specializations. The collector uses this to call // their markIteratively and sweep methods. @@ -165,26 +133,49 @@ class WeakMap : public HashMap<Key, Valu return true; } void nonMarkingTrace(JSTracer *trc) { for (Range r = Base::all(); !r.empty(); r.popFront()) markValue(trc, &r.front().value); } + bool keyNeedsMark(JSObject *key) { + if (JSWeakmapKeyDelegateOp op = key->getClass()->ext.weakmapKeyDelegateOp) { + JSObject *delegate = op(key); + /* + * Check if the delegate is marked with any color to properly handle + * gray marking when the key's delegate is black and the map is + * gray. + */ + return delegate && gc::IsObjectMarked(&delegate); + } + return false; + } + + bool keyNeedsMark(gc::Cell *cell) { + return false; + } + bool markIteratively(JSTracer *trc) { bool markedAny = false; for (Enum e(*this); !e.empty(); e.popFront()) { /* If the entry is live, ensure its key and value are marked. */ Key prior(e.front().key); if (gc::IsMarked(const_cast<Key *>(&e.front().key))) { if (markValue(trc, &e.front().value)) markedAny = true; if (prior != e.front().key) e.rekeyFront(e.front().key); + } else if (keyNeedsMark(e.front().key)) { + gc::Mark(trc, const_cast<Key *>(&e.front().key), "proxy-preserved WeakMap key"); + if (prior != e.front().key) + e.rekeyFront(e.front().key); + gc::Mark(trc, &e.front().value, "WeakMap entry"); + markedAny = true; } } return markedAny; } void sweep(JSTracer *trc) { /* Remove all entries whose keys remain unmarked. */ for (Enum e(*this); !e.empty(); e.popFront()) {
--- a/js/xpconnect/tests/chrome/Makefile.in +++ b/js/xpconnect/tests/chrome/Makefile.in @@ -57,16 +57,17 @@ MOCHITEST_CHROME_FILES = \ test_expandosharing.xul \ file_expandosharing.jsm \ test_getweakmapkeys.xul \ test_mozMatchesSelector.xul \ test_nodelists.xul \ test_precisegc.xul \ test_sandboxImport.xul \ test_weakmaps.xul \ + test_weakmap_keys_preserved.xul \ test_weakref.xul \ test_wrappers.xul \ $(NULL) # Disabled until this test gets updated to test the new proxy based # wrappers. # test_wrappers-2.xul \
new file mode 100644 --- /dev/null +++ b/js/xpconnect/tests/chrome/test_weakmap_keys_preserved.xul @@ -0,0 +1,37 @@ +<?xml version="1.0"?> +<?xml-stylesheet type="text/css" href="chrome://global/skin"?> +<?xml-stylesheet type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css"?> +<!-- +https://bugzilla.mozilla.org/show_bug.cgi?id=673468 +--> +<window title="Mozilla Bug " + xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> + <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"/> + + <!-- test results are displayed in the html:body --> + <body xmlns="http://www.w3.org/1999/xhtml"> + <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=" + target="_blank">Mozilla Bug 673468</a> + </body> + + <!-- test code goes here --> + <script type="application/javascript"> + <![CDATA[ + /** Test for Bug 673468 **/ + + let Cc = Components.classes; + let Cu = Components.utils; + let Ci = Components.interfaces; + + let system = Cc["@mozilla.org/systemprincipal;1"].createInstance(); + let sandbox = Cu.Sandbox(system); + let map = sandbox.WeakMap(); + let obj = {}; + map.set(obj, {}); + + Cu.forceGC(); + + ok(map.has(obj), "Weakmap still contains our wrapper!"); + ]]> + </script> +</window>