Status
()
People
(Reporter: gal, Assigned: gal)
Tracking
Firefox Tracking Flags
(Not tracked)
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 attachment)
2.67 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
Comment hidden (empty) |
(Assignee) | ||
Comment 2•8 years ago
|
||
Created attachment 515386 [details] [diff] [review] patch
Assignee: general → gal
(Assignee) | ||
Updated•8 years ago
|
Attachment #515386 -
Flags: review?(brendan)
(Assignee) | ||
Comment 3•8 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•8 years ago
|
Attachment #515386 -
Flags: approval2.0?
(Assignee) | ||
Updated•8 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 4•8 years ago
|
||
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•8 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•8 years ago
|
Attachment #515386 -
Flags: review- → review?
Comment 6•8 years ago
|
||
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•8 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 8•8 years ago
|
||
Comment on attachment 515386 [details] [diff] [review] patch a=beltzner
Attachment #515386 -
Flags: approval2.0? → approval2.0+
(Assignee) | ||
Updated•8 years ago
|
Attachment #515386 -
Flags: approval2.0+
(Assignee) | ||
Comment 9•8 years ago
|
||
I just land this on TM.
(Assignee) | ||
Comment 10•8 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/1c8e91b2e3a4
Whiteboard: fixed-in-tracemonkey
Comment 11•8 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/1c8e91b2e3a4
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.