The default bug view has changed. See this FAQ.

Cleanup/Improve OOM testing code in the JS shell

RESOLVED FIXED in Firefox 31

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: decoder, Assigned: decoder)

Tracking

(Blocks: 1 bug, {sec-want})

Trunk
mozilla31
x86_64
Linux
sec-want
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox31 fixed)

Details

(Whiteboard: [qa-][adv-main31-])

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8366658 [details] [diff] [review]
js-oom-cleanup.patch

Currently, the JS shell can be tested for OOM behavior using the oomAfterAllocations function. This function works together with macros in js/Public.h. There are several issues:

1. The backtrace code stuff in there isn't used anymore. I initially added that code, but later found out that using a scripted gdb is easier, so we should just rip out that stuff.

2. We have two macros, JS_OOM_POSSIBLY_FAIL() and JS_OOM_POSSIBLY_FAIL_REPORT(cx). The second macro was added because some places where OOM could happen did not call js_ReportOutOfMemory, which made it impossible to break on that function to get an OOM backtrace. However, the number of places that would need JS_OOM_POSSIBLY_FAIL_REPORT has increased and I don't see a big advantage in keeping that macro (also because it will report OOM where no OOM should be reported). Instead, we should just use JS_OOM_POSSIBLY_FAIL() and add an empty function that we can break on in gdb. This function must not be inlined etc, and should only be activated with a configure flag (we can recycle the flag from 1. for that purpose and just rename it).

Patch is attached.
Attachment #8366658 - Flags: review?(jdemooij)
Comment on attachment 8366658 [details] [diff] [review]
js-oom-cleanup.patch

Review of attachment 8366658 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good; nice cleanup!

::: js/public/Utility.h
@@ +81,5 @@
>  extern JS_PUBLIC_DATA(uint32_t) OOM_maxAllocations; /* set in builtins/TestingFunctions.cpp */
>  extern JS_PUBLIC_DATA(uint32_t) OOM_counter; /* data race, who cares. */
>  
> +#ifdef JS_OOM_BREAKPOINT
> +static JS_NEVER_INLINE void js_failedAllocBreakpoint() { asm(""); }

Nit: s/JS_NEVER_INLINE/MOZ_NEVER_INLINE (bug 964016 got rid of JS_NEVER_INLINE)

@@ +92,4 @@
>      do \
>      { \
>          if (++OOM_counter > OOM_maxAllocations) { \
> +            JS_OOM_CALLBPFUNC();\

Nit: I think either JS_OOM_CALL_BREAKPOINT_FUNC() or JS_OOM_CALL_BP_FUNC() is a bit clearer.
Attachment #8366658 - Flags: review?(jdemooij) → review+

Updated

3 years ago
Depends on: 872823
(Assignee)

Comment 2

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/338a45ec7af9
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Blocks: 988097
https://hg.mozilla.org/mozilla-central/rev/338a45ec7af9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
status-firefox31: --- → fixed
Whiteboard: [qa-]
Whiteboard: [qa-] → [qa-][adv-main31-]
You need to log in before you can comment on or make changes to this bug.