The GC can still happen when reporting OOM

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(firefox5- wontfix, firefox6- wontfix, firefox7+ fixed, status1.9.2 wontfix)

Details

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

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

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

Comment 2

7 years ago
Created attachment 534304 [details] [diff] [review]
v1

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?]

Updated

7 years ago
status-firefox5: --- → wontfix
status-firefox6: --- → affected
status-firefox7: --- → affected
tracking-firefox5: --- → -
tracking-firefox6: --- → +
tracking-firefox7: --- → +
Igor: any progress on this one?
(Assignee)

Comment 5

7 years ago
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
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?] → [sg:critical?] fixed-in-tracemonkey
status-firefox7: affected → fixed
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.