637054 - proxies finalize on clear, don't do anything on finalize

Status: RESOLVED FIXED

Description

[Andreas Gal :gal]

Comment 1

[Andreas Gal :gal]

Embarrassing.

Comment 2

[Andreas Gal :gal]

Created attachment 515386 [details] [diff] [review]
patch

Comment 3

[Andreas Gal :gal]

Removing b?. It seems this has currently no ill effects (we don't rely on the finalize hook anywhere as far as I can tell).

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 515386 [details] [diff] [review]
patch

Did I really review this?

static void
proxy_Finalize(JSContext *cx, JSObject *obj)
{
    JS_ASSERT(obj->isProxy());
    if (!obj->getSlot(JSSLOT_PROXY_HANDLER).isUndefined())
        obj->getProxyHandler()->finalize(cx, obj);
}

It's up to the GC whether the handler is garbage and should be finalized. This code cannot assume the proxy's JSSLOT_PROXY_HANDLER slot holds the last ref.

/be

Comment 5

[Andreas Gal :gal]

The proxy handler is a singleton C++ class without any internal state. We dispatch over the handler's VMT to finalize any handler-specific state in _obj_. obj is garbage, since its finalize hook was just invoked by the GC.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 515386 [details] [diff] [review]
patch

Duh, sorry for not reading harder -- and finalize (the JSProxyHandler method, not a JSClass hook) is empty.

The finalize name is confusing. But it is empty so I can't suggest a better one at the moment. Bleah.

This bug, although zero risk, is not exactly blocking material.

/be

Comment 7

[Andreas Gal :gal]

It would be blocking if via clear you can premature cause finalization to happen, but all finalizer handlers are empty, so its a noop in essence, which is why I didn't ask for blocking.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 515386 [details] [diff] [review]
patch

a=beltzner

Comment 9

[Andreas Gal :gal]

I just land this on TM.

Comment 10

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/1c8e91b2e3a4

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/1c8e91b2e3a4