801114 - JSDebugErrorHook could trigger GC out of OOM handler

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

SpiderMonkey's GC is not prepared to handle this case.

The JSDebugErrorHook is only used by JSD, which is only used by Firebug.  We need to ensure that Firebug does nothing that could trigger GC in this path (including running any JS code).  We should remove this callback from the OOM path for future versions.  Since there isn't much of anything useful that Firebug (or anyone) could do with an OOM callback, I doubt this will be problematic.

Honza, do you see any problems from Firebug's perspective if we just remove this callback from js_ReportOutOfMemory?

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Terrence Cole [:terrence] from comment #0)
> SpiderMonkey's GC is not prepared to handle this case.
> 
> The JSDebugErrorHook is only used by JSD, which is only used by Firebug.  We
> need to ensure that Firebug does nothing that could trigger GC in this path
> (including running any JS code).
Firebug's onError hook is here
https://github.com/firebug/firebug/blob/master/extension/modules/firebug-service.js#L2173

I don't know how to test that GC is triggered within the hook.

The hook doesn't do anything special except of iterating possible error breakpoints (it's the enumerateErrorBreakpoints call) and setting a flag if Firebug should break in consequent onDebug hook.

> We should remove this callback from the OOM path for future versions.
> Since there isn't much of anything useful
> that Firebug (or anyone) could do with an OOM callback, I doubt this will be
> problematic.
> 
> Honza, do you see any problems from Firebug's perspective if we just remove
> this callback from js_ReportOutOfMemory?
Firebug actually uses a condition, see:
https://github.com/firebug/firebug/blob/master/extension/modules/firebug-service.js#L2221

...to ignore the hook and bail out (not drop into onDebug) if message passed into onError is equal to "out of memory" so, I think it can be safely removed for this case.

I suppose that the hook still works for the other JS errors.

Honza

Comment 2

[Terrence Cole [:terrence]]

Attachment: v0

(In reply to Jan Honza Odvarko from comment #1)
> (In reply to Terrence Cole [:terrence] from comment #0)
> > SpiderMonkey's GC is not prepared to handle this case.
> > 
> > The JSDebugErrorHook is only used by JSD, which is only used by Firebug.  We
> > need to ensure that Firebug does nothing that could trigger GC in this path
> > (including running any JS code).
> Firebug's onError hook is here
> https://github.com/firebug/firebug/blob/master/extension/modules/firebug-
> service.js#L2173
> 
> I don't know how to test that GC is triggered within the hook.

There is JS code running, so it is already too late. :-)
 
> The hook doesn't do anything special except of iterating possible error
> breakpoints (it's the enumerateErrorBreakpoints call) and setting a flag if
> Firebug should break in consequent onDebug hook.
> 
> > We should remove this callback from the OOM path for future versions.
> > Since there isn't much of anything useful
> > that Firebug (or anyone) could do with an OOM callback, I doubt this will be
> > problematic.
> > 
> > Honza, do you see any problems from Firebug's perspective if we just remove
> > this callback from js_ReportOutOfMemory?
> Firebug actually uses a condition, see:
> https://github.com/firebug/firebug/blob/master/extension/modules/firebug-
> service.js#L2221
> 
> ...to ignore the hook and bail out (not drop into onDebug) if message passed
> into onError is equal to "out of memory" so, I think it can be safely
> removed for this case.
> 
> I suppose that the hook still works for the other JS errors.

You are correct.  Thank you for the links!

****

Bill, this only removes the JSDebugErrorHook call from OOM: it keeps the clearPendingException and errorReporter callback.  I /think/ this is the desired behavior, even if it's not entirely sane.

Comment 3

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1984b76a0b2

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d1984b76a0b2