Closed Bug 801114 Opened 7 years ago Closed 7 years ago

JSDebugErrorHook could trigger GC out of OOM handler

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox19 --- fixed
firefox-esr10 --- wontfix
firefox-esr17 --- wontfix
b2g18 --- wontfix

People

(Reporter: terrence, Assigned: terrence)

Details

(Keywords: sec-audit, Whiteboard: [adv-main19+])

Attachments

(1 file)

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?
(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
Attached patch v0Splinter Review
(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.
Attachment #672830 - Flags: review?(wmccloskey)
Attachment #672830 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/d1984b76a0b2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Whiteboard: [adv-main19+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.