327996 - Consider triggering GC from class hooks when WAY_TOO_MUCH_GC defined

Status: RESOLVED INCOMPLETE

(Core :: XPConnect, defect)

Description

[Boris Zbarsky [:bzbarsky]]

From IRC today:

<dbaron> So it seems like one of the problem areas that WAY_TOO_MUCH_GC doesn't
  catch is when various things within the JS engine do things that call out to a 
  JS API user (e.g., something in XPConnect) who is permitted to allocate JS
  objects, but usually doesn't.  Should there be any effort to document which
  callbacks are allowed to allocate and add js_GC calls #ifdef WAY_TOO_MUCH_GC?

Brendan and I talked about this, and doing it for the JS engine in general is not something brendan really wants to do.  What he's willing to do is to expose an API that will do exactly what we want here (not poke (to attempt to not hit perf any more than we already do), but clear newborns and then gc).  Then we could call this API at least from the XPCWrappedNative jsops (from the macros those use).  Preferably we should also look at other JSClass definitions we have and decide whether those ops should call this API.  Places that should be checked if we do that:

A few places in XPConnect.
A few places in nsDOMClassInfo (global scope polluter, constructor, etc)
XBL

David, what do you think?

Comment 1

[Brendan Eich [:brendan]]

(In reply to comment #0)
> <dbaron> So it seems like one of the problem areas that WAY_TOO_MUCH_GC doesn't
>   catch is when various things within the JS engine do things that call out to
> a 
>   JS API user (e.g., something in XPConnect) who is permitted to allocate JS
>   objects, but usually doesn't.  Should there be any effort to document which
>   callbacks are allowed to allocate and add js_GC calls #ifdef WAY_TOO_MUCH_GC?

Just to be crystal clear: every callback is allowed to call any JS API entry point, in general.  Counterexamples welcome.  I'm probably forgetting something, but the full API is what we expose, and Murphy's Law says every hook will call the least convenient API at the worst time.  IOW we haven't designed any implicit restrictions into the system that are in need of documentation.

/be

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It is nice to be able to define/undefine WAY_TOO_MUCH_GC and rebuild only in js/src/, though.

Comment 3

[Boris Zbarsky [:bzbarsky]]

It's already used in nsJSEnvironment.cpp...

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

We could catch many of them just by making the JS_PropertyStub and friends force a GC #if WAY_TOO_MUCH_GC, I suspect.  There are a handful of other cases (debugger callouts, etc.) that aren't stubbed, but I suspect we could audit our way to victory there without undue pain.

(Counterexample (?): finalizers can't allocate, and I suspect that GC-notification callbacks can't either, looking quickly at the test in js_NewGCThing.)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Should we be setting rt->gcPoke in JS_ClearNewbortRoots and when setting newborns in js_NewGCThing?

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #4)
> We could catch many of them just by making the JS_PropertyStub and friends
> force a GC #if WAY_TOO_MUCH_GC, I suspect.

JS_PropertyStub is not called at all for stubbed getters, setters, and addProperty calls..

I continue to think we want to avoid diminishing returns here.  Fixing bug 313437 would be a better use of time at some point :-P.

/be

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #5)
> Should we be setting rt->gcPoke in JS_ClearNewbortRoots and when setting
> newborns in js_NewGCThing?

#ifdef WAY_TOO_MUCH_GC, maybe in the first case.  In the second, I'm not sure.  That might cross the line into unbearable slowness (WAY_WAY_TOO_MUCH_GC?).

/be

Comment 8

[Igor Bukanov]

*** Bug 333119 has been marked as a duplicate of this bug. ***

Comment 9

[Igor Bukanov]

(In reply to comment #6)
> JS_PropertyStub is not called at all for stubbed getters, setters, and
> addProperty calls..
> 
> I continue to think we want to avoid diminishing returns here.  Fixing bug
> 313437 would be a better use of time at some point :-P.

IMO it worths to spend some time for an option to force GC whenever JS-defined getter, setter or object convereter could be called. It helps to prevent GC bugs that scripts can exploit to subvert jsval type in few lines of code.

Resolving bug 313437 is nice but it would not prevent bugs, say, in jsval->JSString conversion code as they would be bugs in the engine, not in application callbacks.

Comment 10

[Brendan Eich [:brendan]]

We could macroize the JSClass hook calls, for sure.  That would be a JS engine patch, not an XPConnect patch.  Or were people thinking only of patching XPConnect somehow?

/be

Comment 11

[Andrew McCreight [:mccr8]]

WAY_TOO_MUCH_GC doesn't exist any more.