Closed Bug 658864 Opened 12 years ago Closed 12 years ago

The GC can still happen when reporting OOM

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox5 - wontfix
firefox6 - wontfix
firefox7 + fixed
status1.9.2 --- wontfix

People

(Reporter: igor, Assigned: igor)

References

Details

(Whiteboard: [sg:critical?] fixed-in-tracemonkey [qa-])

Attachments

(1 file)

v1
5.55 KB, patch
gwagner
: review+
Details | Diff | Splinter Review 
The bug 640265 tries to prevent the GC from happening when reporting OOM. Hover, the implemented fix has a race that still allows to run the GC under OOM. The problem comes from using the global boolean JSRuntime::inOOMReport flag that is updated and checked outside a lock. As such the check in the js_GC for the flag can happen after an OOM also happens on another thread which can clear the flag before js_GC accesses it.
There is yet another problem. The error reporter for DOM workers suspends the current request even for OOM error, see http://hg.mozilla.org/tracemonkey/file/57412df720cf/dom/src/threads/nsDOMThreadService.cpp#l630. So what can happen is that the main thread waits in js_GC for a worker request to complete, then the worker gets OOM and suspend the request in the error reporter. Then the GC on the main thread starts resulting in GC during memory allocation.

To fix both this and the problem from the comment 0 I plan to use atomic increment/decrement to set the OOM reporting flag and move the flag check after the GC has waited for other threads to suspend the requests.
Assignee: general → igor
Attached patch v1 — — Splinter Review 
The patch implements the plan from the comment 1.
Attachment #534304 - Flags: review?(anygregor)
Attachment #534304 - Flags: review?(anygregor) → review+
severity rating based on bug 640265
Whiteboard: [sg:critical?]
Igor: any progress on this one?
http://hg.mozilla.org/mozilla-central/rev/87cb1807aab9

- I forgot to mark the bug fixed-in-tracemonkey after landing it for some time. Sorry about that.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?] → [sg:critical?] fixed-in-tracemonkey
Merged to mozilla-central on June 6 so it made mozilla7 but not mozilla6. Luke asserts backporting isn't really necessary here, it's an extremely low probability condition to exploit.
status-firefox6: affected → wontfix
tracking-firefox6: + → -
I don't know if this applies to the 1.9.2 branch, but we're unlikely to fix it even if it does.
status1.9.2: --- → wontfix
qa-: does not seem to be anything QA can do to verify this fix
Whiteboard: [sg:critical?] fixed-in-tracemonkey → [sg:critical?] fixed-in-tracemonkey [qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.