296512 - Change DOMGCCallback to *always* chain to other callbacks

Status: RESOLVED FIXED

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

Description

[timeless]

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.

Comment 1

[timeless]

Attachment: reorder chaining call before early return

Comment 2

[timeless]

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 :).

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 185262 [details] [diff] [review]
reorder chaining call before early return

r=jst

Comment 4

[David Bradley]

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.

Comment 5

[Johnny Stenback (:jst)]

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 6

[Brendan Eich [:brendan]]

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 7

[timeless]

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.