Last Comment Bug 655297 - XPCWrappedNative keys can disappear from WeakMaps
: XPCWrappedNative keys can disappear from WeakMaps
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal with 1 vote (vote)
: mozilla8
Assigned To: Andrew McCreight [:mccr8]
:
Mentors:
Depends on: 819131
Blocks: WeakMap 659710 673468 680937
  Show dependency treegraph
 
Reported: 2011-05-06 10:23 PDT by Jason Orendorff [:jorendorff]
Modified: 2013-01-23 00:35 PST (History)
16 users (show)
continuation: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Have JavaScript WeakMaps look through wrappers when deciding which entries to retain, to cope with wrapper re-synthesis (2.15 KB, patch)
2011-06-30 13:44 PDT, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
test wrappers + weak maps (731 bytes, text/html)
2011-07-18 18:08 PDT, Andrew McCreight [:mccr8]
no flags Details
add a callback hook for XPCOM for weak map keys (4.48 KB, patch)
2011-07-18 19:30 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
mark native wrappers that are weak map keys (5.81 KB, patch)
2011-07-19 18:45 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
mark native wrappers that are weak map keys (5.83 KB, patch)
2011-07-20 11:13 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
mark native wrappers that are weak map keys (6.67 KB, patch)
2011-07-21 12:33 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
jorendorff's test, in Mochitest form (make sure a GC doesn't throw out a wrapped native weak map key) (2.35 KB, patch)
2011-07-21 15:37 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
mark native wrappers that are weak map keys (6.96 KB, patch)
2011-07-21 17:34 PDT, Andrew McCreight [:mccr8]
wmccloskey: review+
Details | Diff | Splinter Review
mark native wrappers that are weak map keys (6.99 KB, patch)
2011-07-22 10:01 PDT, Andrew McCreight [:mccr8]
continuation: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2011-05-06 10:23:19 PDT
Unconfirmed but I'm pretty sure. The test case would be like,

xray = otherWindow.document;
var map = new WeakMap;
map.set(xray, 1);
xray = null;
gc();                           // wipe out the xray wrapper, remove map entry
xray = otherWindow.document;    // silently generate a new xray wrapper
assertEq(map.get(xray), 1);     // nope, map entry is gone

jimb pointed out that this fits right in with the mental model of WeakMaps as extra properties on the key objects. We have a hack to make this work for expando properties, but not for WeakMap entries.

jorendorff: gal: does making an expando on an xraywrapper keep it live as long as the xpcwn is alive?
gal: yes/no jorendorff
gal: we preserve the expandos
gal: you might get a new wrapper
gal: but you can't tell
jorendorff: gal: ...well, you can tell with WeakMap
gal: mhm
gal: lame
gal: jorendorff, can you please file that, its actually important I think
Comment 1 Kyle Simpson 2011-06-22 10:35:53 PDT
I am working on the html-editor devtool, and was trying to use weakmap's to map various DOM elements (both in the target page and in my devtool frame) to a metadata structure. 

I ran into this issue where I am storing a reference to a newly created, but not yet appended, DOM element (from `document.createElement()`), as the key in a WeakMap. Then, later, after that DOM element has been appended to the DOM, grabbing a reference to that DOM element, and trying to look it up in the WeakMap, and not finding it.

Comparing the console.log() output for the DOM element's reference, I find...

before append:
[object XrayWrapper [object HTMLDivElement @ 0xc20c4f0 (native @ 0x96aece0)]]

later, after append:
[object XrayWrapper [object HTMLDivElement @ 0xc1feb98 (native @ 0x96aece0)]]

So it appears the wrapper itself changed, while the underlying element did not.

It seems my problem is either:
1. This bug, where the XRayWrapper has been GC'd, and the entry no longer exists in the WeakMap.

2. The WeakMap is storing the XRayWrapper (and address), and since the XRayWrapper seems to be different, the lookup is failing.

In either case, it would seem that a user who wants to store a DOM element as a key in a WeakMap (pretty much its intended use-case) shouldn't have this case where two separate references to the same underlying DOM element (before and after append) can't be used to set() and get() in a WeakMap, respectively.

The workaround that was suggested is that I call `XPCNativeWrapper.unwrap(x)` for any DOM element reference, before setting it into the WeakMap, or looking up a previously stored entry. Since I'm working on a devtool in the chrome-js space, this may very well work, but content-js users won't have that luxury.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-22 16:31:13 PDT
Just to be sure, you're not setting innerHTML anywhere in that code, are you?  innerHTML last I recall will blow away sibling elements when you use += with it.
Comment 3 Kyle Simpson 2011-06-22 16:42:41 PDT
I do use .innerHTML, but *not* in this section of the code, nor affecting that part of the DOM. I use .innerHTML only against an in-memory created element, which I never attach to the DOM, and which I'm doing only to invoke the parser to parse an inputted list of attributes.

So, in short, no I am not using .innerHTML on the elements in question.
Comment 4 Jason Orendorff [:jorendorff] 2011-06-23 10:11:16 PDT
Kyle's last paragraph says:
> The workaround that was suggested is that I call `XPCNativeWrapper.unwrap(x)`
> for any DOM element reference, before setting it into the WeakMap, or looking
> up a previously stored entry. Since I'm working on a devtool in the chrome-js
> space, this may very well work, but content-js users won't have that luxury.

Content-js users won't have the problem to begin with, if it's this bug.

However -- I don't think this workaround will really work; I asked mrbkap about it on IRC and he says he doesn't know if it'll work or not.

I vote we just rip out the bogus optimization here. The cost is that once you create a wrapper for a DOM node, (a) the wrapper stays around for the entire life of the DOM node; (b) the DOM node can't be collected by per-comparment GC. How bad would that be? I vote we try living with it. The alternative is to hack WeakMap further to support this optimization.
Comment 5 Kyle Simpson 2011-06-23 10:34:04 PDT
I was experiencing my bug from inside of content-js (where my prototype code was being developed). I was running the page inside of my dev-build of the browser, and was getting those XrayWrapper outputs when I console.log()'d a DOM element. So I'm not sure if XrayWrapper is only for debug builds or something, but I was definitely seeing my problem (perhaps it's not actually related to this bug after all?) in content-js land, not just chrome-js land.

Moreover, I've tried the workaround suggested, and it didn't fix my problem.

Could the problem be that my JS (including my creation and retrieval of WeakMap entries) is running in a different window/frame than the DOM elements that I am storing and looking up references to?

Here's how I am storing and (trying to) retrieve the reference in the WeakMap:


myweakmap.set(XPCNativeWrapper.unwrap(el), ....);
dump(XPCNativeWrapper.unwrap(el));

// ...

var foo = evt.target.parentNode.parentNode;
var blah = myweakmap.get(XPCNativeWrapper.unwrap(foo));
blah; // blah is `undefined`
dump(XPCNativeWrapper.unwrap(foo));


The outputs from `dump()` were, respectively:

[object HTMLDivElement @ 0x790a238 (native @ 0x5ea3ee0)]

[object HTMLDivElement @ 0x7937f90 (native @ 0x5ea3ee0)]

Any thoughts on what's happening? I'm not sure how to track down if my reference is getting GC'd (as this bug reports), or if it's just simply not finding the element in the .get() lookup because the address of the reference that I'm passing doesn't match what was stored.
Comment 6 Jason Orendorff [:jorendorff] 2011-06-23 10:48:00 PDT
(In reply to comment #5)
> I was experiencing my bug from inside of content-js

Oh, yeah, sorry. This can happen because of this:

> Could the problem be that my JS (including my creation and retrieval of
> WeakMap entries) is running in a different window/frame than the DOM
> elements that I am storing and looking up references to?

I think it must be related to this, because within a single web page, single window, you shouldn't see XrayWrappers.

I asked Kyle for a test case and he's working on one.
Comment 7 Steve Fink [:sfink] [:s:] 2011-06-23 11:27:42 PDT
[naïve bystander]

Is there some notion of object identity we could invent that would work for this? So when you use an object as a key in a weakmap, it would internally say weakmap[obj.birthDateAndSocialSecurityNumber] = value?

Doesn't remote debugging want something like this?

cf bug 325636
Comment 8 Jason Orendorff [:jorendorff] 2011-06-23 15:38:35 PDT
This reproduces it for me. To get a chrome scratchpad, you have to set devtools.chrome.enabled, open a scratchpad, and then find the Environment menu.

<!DOCTYPE html>

<script>
var map = WeakMap();
function f() {
    var paras = document.getElementsByTagName("P");
    for (var i = 0; i < paras.length; i++)
        map.set(paras[i], "ok");
}
function g() {
    var paras = document.getElementsByTagName("P");
    for (var i = 0; i < paras.length; i++) {
        if (map.get(paras[i]) != "ok") {
            alert("fail");
            return;
	}
    }
    alert("pass");
}
</script>

<body>
    <ul>
        <li><a href="javascript:f()">click here</a></li>
        <li>In a chrome scratchpad: <code>Components.utils.forceGC()</code></li>
        <li><a href="javascript:g()">click here</a></li>
    </ul>
    <p>0</p> <p>1</p> <p>2</p> <p>3</p> <p>4</p>
    <p>5</p> <p>6</p> <p>7</p> <p>8</p> <p>9</p>
</body>
Comment 9 Jim Blandy :jimb 2011-06-27 10:45:03 PDT
If we can get bug 667126 landed, I think we can fix this as follows:

Have the WeakMap instantiation used to implement the JavaScript WeakMap type use a MarkPolicy where:

- the entryLive member, when the entry's key is a wrapper, looks through the wrapper and tests the mark bit of the wrappers' referent, not the wrapper itself, and

- the markLiveEntry member marks both the key (the wrapper, in the test case) and the value.

The underlying cause of the bug is that, if a wrapper is unreferenced and hasn't been modified, then the compartment machinery will just drop it, assuming that it can re-synthesize a perfectly equivalent new wrapper on demand. However, when a wrapper appears as the key in a WeakMap, the WeakMap does not cause the wrapper to appear to be referenced, but can distinguish it from its rematerialization nonetheless --- the optimization can be detected.

With the change above, the map entry lives as long as the referent lives, and keeps the wrapper alive, so that it is never dropped and re-synthesized, so the bug does not occur.

I'll try to put together a patch that implements this.
Comment 10 Peter Van der Beken [:peterv] - away till Aug 1st 2011-06-29 07:31:24 PDT
For some XPCWrappedNatives we can "preserve" the wrapper, that's how we keep it alive if an expando is set on it. There's no easy API to do that from just a JSObject*, but we could add one if you need it. Though it seems like it might be hard to use it without leaking XPConnect into jsweakmap.* :-(.
Comment 11 Jim Blandy :jimb 2011-06-30 11:34:17 PDT
(In reply to comment #10)
> For some XPCWrappedNatives we can "preserve" the wrapper, that's how we keep
> it alive if an expando is set on it. There's no easy API to do that from
> just a JSObject*, but we could add one if you need it. Though it seems like
> it might be hard to use it without leaking XPConnect into jsweakmap.* :-(.

The idea proposed in comment 9 would have the WeakMap hold a strong reference to the wrapper as long as the referent was alive. Wouldn't that be just as good?
Comment 12 Jim Blandy :jimb 2011-06-30 13:44:13 PDT
Created attachment 543235 [details] [diff] [review]
Have JavaScript WeakMaps look through wrappers when deciding which entries to retain, to cope with wrapper re-synthesis

Kyle, could you give this patch a try and see if it helps?
Comment 13 Jim Blandy :jimb 2011-06-30 15:11:46 PDT
(In reply to comment #12)
> Created attachment 543235 [details] [diff] [review] [review]
> Have JavaScript WeakMaps look through wrappers when deciding which entries
> to retain, to cope with wrapper re-synthesis
> 
> Kyle, could you give this patch a try and see if it helps?

Actually, don't bother; I had misunderstood the underlying issue. The patch doesn't address the real problem.
Comment 14 Mark S. Miller 2011-06-30 17:43:00 PDT
(In reply to comment #13)
> Actually, don't bother; I had misunderstood the underlying issue. The patch
> doesn't address the real problem.

Now that you understand the underlying issue, could you explain it to those of us interested in WeakMaps but unfamiliar with the internal implementation of Firefox?
Comment 15 Peter Van der Beken [:peterv] - away till Aug 1st 2011-07-01 02:29:00 PDT
(In reply to comment #11)
> (In reply to comment #10)
> The idea proposed in comment 9 would have the WeakMap hold a strong
> reference to the wrapper as long as the referent was alive. Wouldn't that be
> just as good?

I don't see how that solves anything for XPCWrappedNatives that aren't wrapped in a security wrapper?
Comment 16 Jim Blandy :jimb 2011-07-05 17:35:24 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > Actually, don't bother; I had misunderstood the underlying issue. The patch
> > doesn't address the real problem.
> 
> Now that you understand the underlying issue, could you explain it to those
> of us interested in WeakMaps but unfamiliar with the internal implementation
> of Firefox?

Certainly!

Fundamentally, we've got here are two optimizations that conflict:

- WeakMaps retain only those entries whose keys are reachable. If a key is unreachable by any means other than the WeakMap, which cannot (by design) be persuaded to cough it up, then certainly the entry will never be used, so it should have no effect on the program to drop the entry altogether.

- Things that create wrapper-like objects --- objects that exist to provide an interface to some underlying object --- often hold their wrappers weakly, on the rationale that, if nobody else is holding a reference to the wrapper, then nobody will notice if it is freed and a new one created on demand ("re-materialized"). (If the wrapper has had properties stored directly on it, then obviously one can't re-materialize an equivalent wrapper, and this optimization is disabled in this case.)

The problem is that one can use WeakMaps to distinguish a wrapper from its "equivalent" re-materialization. The second optimization's technique for deciding whether JavaScript would notice the wrapper's recreation is unsound.

When presented with a WeakMap entry whose key is a wrapper whose semantics is known to the JavaScript implementation, like cross-compartment wrappers, it's reasonable for the implementation to simply check the *referent's* liveness, and hold the key strongly when its referent is live.

However --- and this is what I'd just realized when I wrote comment 13 --- Firefox's JavaScript DOM elements are fronts for non-JavaScript objects that represent the actual DOM node. These underlying objects have their lifetimes managed using reference counts and a very unusual cycle collection algorithm. But critically, they are completely outside the JavaScript implementation. There's no clean way for a WeakMap to ask a JavaScript DOM element object if its referent is alive.

I can see two ways to fix this:

- Add a new bit to JSObjects, MIGHT_DEMATERIALIZE, indicating that someone is planning to apply the de-/re-materialization optimization to this object. An embedding would set this bit when the referent is live *other than* because of the wrapper itself. WeakMap would strongly reference keys with this bit set.

- Add a new JSClass or JSObjectOps member function that, on objects that may be rematerialized, returns true if the underlying referent is otherwise live. On ordinary objects, this would simply return the object's own mark bit. On XPCWrappedNatives, this would (I believe) return true if the native's reference count were greater than one.

The "preserve" operation isn't enough: WeakMaps inherently need to know whether the *underlying* object is alive. Given that information, they can preserve the wrapper just by marking it.
Comment 17 Jim Blandy :jimb 2011-07-05 18:02:38 PDT
I don't think the "preserve" functionality is what we need to expose here. We must distinguish between these two cases, in both of which the wrapper is initially unmarked:

1) the element is live only because of the wrapper JSObject; versus

2) nobody is strongly referencing the wrapper JSObject, but the element is live for other reasons.

In 1), we should drop the WeakMap table entry, and leave the wrapper unmarked, to be freed, which will (I believe) delete the element.

In 2), we should mark the wrapper, which will keep the element alive.

In either case, we can preserve or drop the wrapper just by marking it or not.
Comment 18 Jim Blandy :jimb 2011-07-06 10:35:25 PDT
(In reply to comment #16)
> If a key is
> unreachable by any means other than the WeakMap, which cannot (by design) be
> persuaded to cough it up,

Poor use of English: it's the WeakMap that can't be persuaded to cough up the key, not the "means other than the WeakMap". I guess that's unambiguous if you think about it, but it's not very good.
Comment 19 Jim Blandy :jimb 2011-07-06 14:59:03 PDT
Here are possible docs for the JSObjectOps hook I'm considering adding; does this sound right to people?

----

Some kinds of objects are just disposable wrappers for other objects: the wrapper can be freed when there are no references to it, and then a new wrapper can be re-materialized when and if it is ever needed. For example, JSCrossCompartmentWrappers refer to objects in other compartments; and Wrapped Native wrappers refer to C++ XPCWrappedNative instances, like those representing DOM elements. (Here we're using the term 'wrapper' to refer to the general design pattern, of which JSWrapper is one application.)

However, if such a disposable wrapper object appears as a key in a WeakMap, things are tricky. The user's intention is usually to associate metadata with the wrapper's *referent*, not the wrapper itself. But WeakMaps don't hold their keys strongly: if there are no other references to the wrapper, the collector will free it, and the WeakMap will drop the entry, losing the user's metadata. If the referent itself lives on, then a re-materialized wrapper won't find the original metadata the user stored in the WeakMap. In other words, a re-materialized wrapper may have the same properties and referent as the original, but they are not equivalent when used as keys to WeakMaps.

Note that the problem arises when the wrapper is not strongly referenced, but the referent is: if there's no chance that we'll need to re-materialize a wrapper for this referent (because the referent itself is dead), then we *must* delete the WeakMap entry.

Our solution is to have WeakMap strongly hold (i.e. mark) keys that are wrappers whose referents are otherwise live. Thus, such wrappers are never collected while they appear as WeakMap keys.

A class's weakMapMustHold hook should return true if instances of the class are wrappers for some other object, and the instance passed to the hook has a referent that is live other than because of the wrapper itself. Classes that do not represent wrappers can leave this hook NULL, which is equivalent to a function that always returns false.

When a WeakMap finds a key object that is not marked, and whose weakMapMustHold hook is non-zero, it applies the hook to the key object. If the hook returns true, then the WeakMap marks the key.

For JSWrappers, this hook checks the referent's mark bit. For XPCWrappedNatives, it checks the referent's reference count.
Comment 20 Peter Van der Beken [:peterv] - away till Aug 1st 2011-07-07 08:01:00 PDT
I might be missing something, but I don't see how this will work with cycle collection given that we won't unlink cycles that are held by things marked by the JS GC itself. If a weakmap holds an XPCWrappedNative whose "referent" is in a collectable cycle and only held by the XPCWrappedNative and things in that cycle, then we'll do a GC and mark the key. This will make it impossible for the CC to collect the cycle that the referent is in, and so we'll never not mark the key. (If the referent preserves its XPCWrappedNative the CC would unlink the edge from referent to XPCWrappedNative in this case).
Comment 21 Andrew McCreight [:mccr8] 2011-07-07 16:36:00 PDT
Just to record the discussion Peter, Jim and I had in IRC, it seems like marking the wrappers grey instead of black should be okay.  Existing solutions involving preserving wrappers aren't quite right because there's a general problem involving cross-compartment pointers, and maybe some weird wrappers that don't work quite the same.  Or something like that.
Comment 22 Jim Blandy :jimb 2011-07-11 11:22:23 PDT
Bug 668855 is working on the algorithmic changes needed to handle weakmaps.

Bug 653248 correctly integrates WeakMap traversal with the black and gray marking phases, but can't land until 668855 is addressed.

It seems like the solution here is to have a JSObjectOps (or similar) hook that WeakMaps can call that, in the XPCWrappedNative wrapper case, tells the wrapped native to reference its wrapper strongly. This will make the native and its wrapper behave like a cycle, and then the WeakMap integration will just work.
Comment 23 Jim Blandy :jimb 2011-07-11 16:15:48 PDT
Andrew, I'm going to leave this to you, since it seems like a solution to bug 668855 will sort this out as well, or in any case, the right solution here depends on the solution chosen there.

If you do need a 'preserve' hook or something like that, any JS hacker will be fine for that. I need to concentrate on the Debugger stuff.
Comment 24 Andrew McCreight [:mccr8] 2011-07-14 18:49:02 PDT
dmandelin wanted me to take a look at this.

I don't think Bug 668855 and Bug 653248 are really blocking this bug.

I need some more information on the problem here, or at least pointers to where I can look.  Who is it who is throwing away these wrappers?  It sounds like you are saying that if the JS GC is sweeping, and it sees a wrapper that is unmarked, then it throws it away?  I'm going to assume for the rest of this post that it is the GC that throws away wrappers.

Do we need to deal with wrapped JS objects in addition to wrapped DOM objects?

Jim's general approach sounds reasonable to me: we need a way to see if an object is a wrapper for something that is alive.  If the wrappee is alive, keep the wrapper alive by marking it.

The DOM case is actually fairly direct: from the JS GC's perspective, DOM nodes are always alive.  So any XPCWrappedNative would return "true".  Likewise with JS nodes from other compartments, if we're doing a per-compartment GC.  Of course, if we're doing a global GC, then it would have to return the mark bit of the underlying object.

One problem I see with this is that the color we need to mark the wrapper can vary.  If the underlying object is a DOM object, or a JS object that has been marked grey, then the wrapper should be marked grey.  If the underlying object is a JS object from another compartment, or a JS object that has been marked black, then the wrapper should be marked black.  We need to ask about "Is wrapping a black marked object?" during the black marking phase, and "Is wrapping a gray marked object?" during the grey marking phase.

I don't know if this should be two functions, or somehow we can have a function that returns a color, or the function can look at the current tracer to get the current color, and return something based on that or what.  Is there some documentation about implementing a JSObjectOps hook?  I don't know anything about them.

Of course, the CC-WeakMap story is messed up now anyways, so we might as well just mark everything black for the moment, if it is safe.  The value will get marked black even if they key is grey.  Though I guess if there is stuff hanging off of the wrapper somehow, then it is better to mark the wrapper grey rather than black?  I don't know if that is possible or not.
Comment 25 Andrew McCreight [:mccr8] 2011-07-15 16:57:09 PDT
So, I came across this macro IS_WRAPPER_CLASS.  Is that going to return true if and only if for a class that is a native wrapper?  The comments in xpcpublic.h kind of make it sound like that.  Or can that include other kinds of wrappers, such as cross-compartment wrappers?

If it is only true on things that hold native wrappers, then we can just mark the key grey if it is unmarked, and IS_WRAPPER_CLASS(obj->getClass()).
Comment 26 Andrew McCreight [:mccr8] 2011-07-15 17:18:05 PDT
(In reply to comment #8)
> This reproduces it for me. To get a chrome scratchpad, you have to set
> devtools.chrome.enabled, open a scratchpad, and then find the Environment
> menu.

This didn't work for me in a browser scratchpad.  I got "Error: SyntaxError: missing = in XML attribute", a blank error, and an error because I used the command line flag "-no-attach" which it didn't recognize.  I'll try to make a Mochitest or something and maybe that will work better for me.
Comment 27 Andrew McCreight [:mccr8] 2011-07-18 14:32:35 PDT
I spent a few days looking into this, but I think I'm going to have to unassign myself.  I just don't know enough about wrappers to fix this, I think.  The more I look into them, the less I think I know.  The actual WeakMap part of this is probably comparatively simple, so if anybody else wants to look into this, I can help them from that side of it.
Comment 28 Andrew McCreight [:mccr8] 2011-07-18 18:08:28 PDT
Created attachment 546688 [details]
test wrappers + weak maps

This is just jorendorff's in an easy-to-run form.  I also changed the directions for invoking the GC to just go to about:memory and click on GC, instead of messing around with a chrome scratch pad.

If you click the first button, then immediately click the second, it passes.  If you do a GC in between, it fails.

Bill and I were looking at this and it doesn't actually seem too bad, at least for the WrappedNative's case.
Comment 29 Andrew McCreight [:mccr8] 2011-07-18 19:30:18 PDT
Created attachment 546697 [details] [diff] [review]
add a callback hook for XPCOM for weak map keys

Work in progress.

Whenever you come across an unmarked key, push it through the new weak key marker callback.  If that returns true, the key is now marked.

The XPCOM callback will be something lovely like this:

bool gcWeakKeyMarkerOp(JSTracer *t, JSObject *k) {
  if (!IS_WRAPPER_CLASS(k->getClass()))
    return false;
  c = t->getColor();
  t->setColor(grey);
  markObject(k);
  t->setColor(c);
  return true;
}

We have to mark the wrapper of a C++ object grey, but our marker is probably already black, so we have to change the color.  But in the future we could be calling this while the color is already grey, so we have to save and restore the color!  The color swapping logic could probably also check if the color is currently grey and not do anything in that case or something like that.
Comment 30 Andrew McCreight [:mccr8] 2011-07-19 12:10:07 PDT
Comment on attachment 546697 [details] [diff] [review]
add a callback hook for XPCOM for weak map keys

billm said a JSOPS thing would be less hideous than JS API, so I'm going to do that instead.
Comment 31 Andrew McCreight [:mccr8] 2011-07-19 18:45:36 PDT
Created attachment 546966 [details] [diff] [review]
mark native wrappers that are weak map keys

Here's a rough first pass at a patch based on discussion with billm.  I add a new ClassExtension "wrappeeColor" (suggestions welcomed for naming on many of these things) that, if defined, returns the color of the object contained in the wrapper.  If undefined, the given object is not a wrapper.  For wrapped natives, this is just a constant.  Right now, it returns black, but once we have a proper grey marking phase for weak maps, it should return grey.

When we are marking WeakMaps, if a key is unmarked, we see if the key is a wrapper.  If the color of the wrappee matches our current marking color, we mark the key, then we finish out markEntryIfLive as usual.

Because we mark the key black and not grey, this means that we will usually hold on to WrappedNatives for the lifetime of the map*.  EG it is not actually a WeakMap, it is a StrongMap.  This is not good, but should go away when Bug 653248 is fixed.  I also don't see any way to fix this without adding a new WeakMap marking phase that does nothing except mark wrapper keys grey.  Initially, I figured we could switch to grey in the middle of marking black, but that's not right, because this could result in an object that should be marked black being marked grey instead.

Anyways, with this patch applied, jorendorff's patch passes for me, so that's something.  I also confirmed that it still fails when the line " mJSClass.base.ext.wrappeeColor = XPC_WN_Wrappee_GC_Color" is commented out.  I'll make sure that it also fails if XPC_WN_Wrappee_GC_Color returns Gray instead of black.

*If the wrapper is otherwise reachable from DOM, then it will be marked grey by the normal grey marking phase.  If you make the wrapper as part of an extra spurious DOM cycle, then I think it will be kept grey, and thus be collected properly when other references to the key go away...
Comment 32 Andrew McCreight [:mccr8] 2011-07-20 08:34:59 PDT
I should also note that this only deals with XPC Wrapped Natives.  It doesn't handle "proxy objects" or whatever, where a JS object is wrapped in another JS object.  billm is working on a patch for that.  It has to deal with various scenarios about per-compartment-GC vs global-GC, and whether the underlying object is marked black or grey or not at all.
Comment 33 Andrew McCreight [:mccr8] 2011-07-20 11:13:13 PDT
Created attachment 547155 [details] [diff] [review]
mark native wrappers that are weak map keys

Small cleanup of my previous patch.
Comment 34 Andrew McCreight [:mccr8] 2011-07-20 15:07:18 PDT
Pass a Linux Debug try run.  http://tbpl.mozilla.org/?tree=Try&rev=1d010287eb8b

I still don't quite like how this is set up.  We may be able to just use unwrap() for proxies, which would leave the wrappeeColor callback as no more than an "IsWrappedNative" predicate that the JS engine can call, without having to see XPConnect code, which seems kind of gross.
Comment 35 Andrew McCreight [:mccr8] 2011-07-21 12:33:02 PDT
Created attachment 547470 [details] [diff] [review]
mark native wrappers that are weak map keys

Renamed a few things, added some comments, refactored markEntryIfLive a bit so it should be easier to understand.
Comment 36 Andrew McCreight [:mccr8] 2011-07-21 15:37:56 PDT
Created attachment 547542 [details] [diff] [review]
jorendorff's test, in Mochitest form (make sure a GC doesn't throw out a wrapped native weak map key)

I confirmed this fails without my patch (well, without the line that assigns the special hook, which should be equivalent more or less to current behavior), and passes with my patch.  I'll fold this into my final patch when that's ready.
Comment 37 Andrew McCreight [:mccr8] 2011-07-21 17:34:26 PDT
Created attachment 547574 [details] [diff] [review]
mark native wrappers that are weak map keys

I'm going to try-server this, too, but I confirmed that the included test passes with the patch applied, and fails when the line that sets the native wrapper flag to true is commented out.
Comment 38 Bill McCloskey (:billm) 2011-07-21 17:46:23 PDT
Comment on attachment 547574 [details] [diff] [review]
mark native wrappers that are weak map keys

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

Looks great, thanks!

::: js/src/jsvalue.h
@@ +1016,5 @@
>      JSObjectOp          innerObject;
>      JSIteratorOp        iteratorObject;
>      void               *unused;
> +    /* isWrappedNative is true only if the class is an XPCWrappedNative.
> +       WeakMaps use this to override the wrapper disposal optimization. */

I think it's typical to have an empty line before the comment. Also, the comment should look like this:
  /*
   * isWrappedNative ...
   * more words...
   */
(This syntax is specific to JS; I think it's different in the rest of Mozilla.)

::: js/src/jsweakmap.h
@@ +238,5 @@
> +//
> +// We always mark wrapped natives.  This will cause leaks, but WeakMap+CC
> +// integration is currently busted anyways.  When WeakMap+CC integration is
> +// fixed, XPC wrapped natives should only be marked during non-BLACK marking
> +// (ie grey marking).

Please reference the bug number for fixing WeakMap+CC integration.

@@ +259,5 @@
> +        js::gc::MarkValue(tracer, v, "WeakMap entry value");
> +        return true;
> +    }
> +    // Return true if we should override the GC's default marking
> +    // behavior for this key.

Empty line before the comment again.
Comment 39 Mozilla RelEng Bot 2011-07-22 03:20:17 PDT
Try run for 37fa78d23c33 is complete.
Detailed breakdown of the results available here:
    http://tbpl.mozilla.org/?tree=Try&rev=37fa78d23c33
Results:
    success: 144
    warnings: 2
Total buildrequests: 146
Comment 40 Andrew McCreight [:mccr8] 2011-07-22 10:01:11 PDT
Created attachment 547720 [details] [diff] [review]
mark native wrappers that are weak map keys

Addressed review comments.  Carrying forward the review.
Comment 41 Andrew McCreight [:mccr8] 2011-07-22 10:06:46 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/c2c5b41d0a3c
Comment 42 Bill McCloskey (:billm) 2011-07-22 11:16:52 PDT
Changing the title to reflect what was fixed here. Jason, to construct a test case for XrayWrappers, do we just need to use a DOM element from a different window rather than the same window?
Comment 43 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-07-25 05:49:41 PDT
http://hg.mozilla.org/mozilla-central/rev/c2c5b41d0a3c

Note You need to log in before you can comment on or make changes to this bug.