JSDebugErrorHook could trigger GC out of OOM handler

RESOLVED FIXED in Firefox 19

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

({sec-audit})

Trunk
mozilla19
sec-audit
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox19 fixed, firefox-esr10 wontfix, firefox-esr17 wontfix, b2g18 wontfix)

Details

(Whiteboard: [adv-main19+])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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
(Assignee)

Comment 2

5 years ago
Created attachment 672830 [details] [diff] [review]
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.
Attachment #672830 - Flags: review?(wmccloskey)
Attachment #672830 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 3

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1984b76a0b2
https://hg.mozilla.org/mozilla-central/rev/d1984b76a0b2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox19: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
status-firefox-esr10: --- → wontfix
Keywords: sec-audit

Updated

5 years ago
status-firefox-esr17: --- → wontfix
status-b2g18: --- → wontfix
Whiteboard: [adv-main19+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.