Closed Bug 296512 Opened 15 years ago Closed 15 years ago
Change DOMGCCallback to *always* chain to other callbacks
the other gccallbacks (xpc, jsj) all nicely always chain to the previous callback. i have code which really needs a chance to fire on other threads and unfortunately the DOM callback is really mean.
summary of conversation from irc: jst: So I'm looking at your DOMGCCallback change. The whole point of that callback is to prevent GC from ever happening on any thread other than the main thread. I know, but that's not very nice. The garbage collector is evil; xpconnect always releases all wrapped objects on the gc thread. This is absolutely broken since many objects are not threadsafe and expect to be destroyed on the thread that created them. I'm not changing the code to actually perform a GC off the main thread. I'm changing the code so that other threads can be notified that a gc would like to have considered happening on that other thread. This gives the other threads the ability to properly dispose of objects that were created on their threads. Since the callback will still ignore the result in these cases, GC will still only ever happen on the main thread. This means that when xpconnect shuts down as some other thread is destroyed, that thread will have a chance to properly release objects it owns. jst: but just because you get a GC callback doesn't mean any garbage will be collected... are you talking about non-JS objects here? yes! The wrappednative garbage will be collected on the main thread, and a later change will make the main thread not destroy garbage it doesn't legally own. that garbage will then accumulate in per thread bins and will be released by the individual threads with this signal jst: doesn't this mean that xpconnect's GC callbacks may now also run on other threads than the main thread? Only the first one. The one where xpconnect currently does its work won't because the callback I'm changing will still veto the js_gc - which means gc won't happen, so the other phases still won't happen on other threads. jst: ok, makes sense As it happens... the xpconnect gc callback also vetos if it's the wrong thread, jband stuck that there ages ago :). So, to some extent, this stuff expected to happen :).
Status: NEW → ASSIGNED
Comment on attachment 185262 [details] [diff] [review] reorder chaining call before early return r=jst
I wonder what the intent of this test originally was. The CVS comment talks about not allowing this to occur on anything but the DOM thread which makes sense. But the test includes the JSGC_BEGIN. So apparently this chains for the rest of the GC states even if called on a diferent thread. I guess I'd be curious to know why that test was added in the first place specifically why the JSGC_BEGIN test was there.
A false return from a GC callback for the JSGC_BEGIN stage will stop the GC, so no further calls will be made by the stopped GC.
Comment on attachment 185262 [details] [diff] [review] reorder chaining call before early return sr+a=me, with more IRC logging for posterity: <brendan> ping <timeless> pong <brendan> re: https://bugzilla.mozilla.org/show_bug.cgi?id=296512 <brendan> your patch just reorders, but it doesn't cancel the DOMGCCallback if !result <timeless> it doesn't matter <brendan> yeah <timeless> the code always cancels <brendan> just wondering if it might <timeless> i don't think it can <timeless> since it either always cancels <brendan> if you had an "inner" gc callback that wanted to cancel on the main thread <timeless> or honors in the case when it isn't planning to cancel <timeless> oh <brendan> i mean cancel even the dom's gccallback <timeless> well, then you're screwed <brendan> y <timeless> but i don't think that's really a bug here <brendan> i'll sr and a for now <brendan> right /be
Comment on attachment 185262 [details] [diff] [review] reorder chaining call before early return mozilla/dom/src/base/nsJSEnvironment.cpp 1.249 i incorrectly specified a=, oops.
Attachment #185262 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Component: DOM: Core → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.