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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

7 years ago
Embarrassing.
blocking2.0: --- → ?
(Assignee)

Comment 2

7 years ago
Created attachment 515386 [details] [diff] [review]
patch
Assignee: general → gal
(Assignee)

Updated

7 years ago
Attachment #515386 - Flags: review?(brendan)
(Assignee)

Comment 3

7 years ago
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).
blocking2.0: ? → ---
(Assignee)

Updated

7 years ago
Attachment #515386 - Flags: approval2.0?
(Assignee)

Updated

7 years ago
Summary: proxies leak memory, finalize on clear, don't do anything on finalize → proxies finalize on clear, don't do anything on finalize
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
Attachment #515386 - Flags: review?(brendan) → review-
(Assignee)

Comment 5

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

Updated

7 years ago
Attachment #515386 - Flags: review- → review?
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
Attachment #515386 - Flags: review? → review+
(Assignee)

Comment 7

7 years ago
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 on attachment 515386 [details] [diff] [review]
patch

a=beltzner
Attachment #515386 - Flags: approval2.0? → approval2.0+
(Assignee)

Updated

7 years ago
Attachment #515386 - Flags: approval2.0+
(Assignee)

Comment 9

7 years ago
I just land this on TM.
(Assignee)

Comment 10

7 years ago
http://hg.mozilla.org/tracemonkey/rev/1c8e91b2e3a4
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/1c8e91b2e3a4
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.