Closed Bug 296512 Opened 15 years ago Closed 15 years ago

Change DOMGCCallback to *always* chain to other callbacks


(Core :: DOM: Core & HTML, defect)

Not set





(Reporter: timeless, Assigned: timeless)



(1 obsolete file)

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.
Attachment #185262 - Flags: superreview?(jst)
Attachment #185262 - Flags: review?(jst)
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

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 :).
Comment on attachment 185262 [details] [diff] [review]
reorder chaining call before early return

Attachment #185262 - Flags: superreview?(jst)
Attachment #185262 - Flags: superreview?(brendan)
Attachment #185262 - Flags: review?(jst)
Attachment #185262 - Flags: review+
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:
<brendan> your patch just reorders, but it doesn't cancel the DOMGCCallback if
<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
<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

Attachment #185262 - Flags: superreview?(brendan)
Attachment #185262 - Flags: superreview+
Attachment #185262 - Flags: approval1.8b3+
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
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.