Last Comment Bug 948188 - Handle OOM in NewPropertyIteratorObject
: Handle OOM in NewPropertyIteratorObject
Status: RESOLVED FIXED
[qa-]
: crash
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
-- critical (vote)
: mozilla29
Assigned To: Christian Holler (:decoder)
: general
: Jason Orendorff [:jorendorff]
Mentors:
: 948187 (view as bug list)
Depends on:
Blocks: langfuzz 912928
  Show dependency treegraph
 
Reported: 2013-12-09 15:47 PST by Christian Holler (:decoder)
Modified: 2013-12-17 14:51 PST (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
js-NewPropertyIteratorObject-oom.patch (1.12 KB, patch)
2013-12-09 15:49 PST, Christian Holler (:decoder)
n.nethercote: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2013-12-09 15:47:06 PST
In NewPropertyIteratorObject we seem to be calling NewBuiltinClassInstance without checking its return value, although it's fallible:

>     return &NewBuiltinClassInstance(cx, &PropertyIteratorObject::class_)->as<PropertyIteratorObject>();


The attached patch checks the return value first and returns NULL on failure. This fixes an OOM crash bug for me that the fuzzer keeps hitting.
Comment 1 User image Christian Holler (:decoder) 2013-12-09 15:49:15 PST
Created attachment 8344973 [details] [diff] [review]
js-NewPropertyIteratorObject-oom.patch

Yay, Bugzilla ate my attachment \o/

njn, can you review this? Blame tells me you added the line in question :) Thanks!
Comment 2 User image Christian Holler (:decoder) 2013-12-09 15:50:18 PST
*** Bug 948187 has been marked as a duplicate of this bug. ***
Comment 3 User image Nicholas Nethercote [:njn] 2013-12-09 16:24:10 PST
Comment on attachment 8344973 [details] [diff] [review]
js-NewPropertyIteratorObject-oom.patch

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

I suspect I refactored that line but didn't add it originally.  But the patch is simple and looks fine.  Thanks!
Comment 4 User image Christian Holler (:decoder) 2013-12-09 18:18:20 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad9945733719
Comment 5 User image Carsten Book [:Tomcat] 2013-12-10 04:06:42 PST
https://hg.mozilla.org/mozilla-central/rev/ad9945733719
Comment 6 User image Chris Peterson [:cpeterson] 2013-12-12 14:03:22 PST
Christian: should this OOM fix be uplifted to Aurora 28 and Beta 27?
Comment 7 User image Christian Holler (:decoder) 2013-12-12 16:17:36 PST
It's not necessary to uplift OOM fixes unless we see them in some crash stats. Uplifting to Aurora would surely be possible (easy fix, and less crashes in fuzzing then).
Comment 8 User image Christian Holler (:decoder) 2013-12-12 16:20:42 PST
Comment on attachment 8344973 [details] [diff] [review]
js-NewPropertyIteratorObject-oom.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Crashes with OOM conditions
Testing completed (on m-c, etc.): A few days on mozilla-central
Risk to taking this patch (and alternatives if risky): Not risky, patch is very simple (just adding a null check).
String or IDL/UUID changes made by this patch: None
Comment 9 User image Ryan VanderMeulen [:RyanVM] 2013-12-12 16:42:01 PST
(In reply to Christian Holler (:decoder) from comment #7)
> It's not necessary to uplift OOM fixes unless we see them in some crash
> stats.

During the Aurora timeframe, I would think the bar would be pretty low with respect to taking potential stability fixes. If it's simple and low-risk, I would argue to just take it whether it's in crash stats. On beta, sure the bar is higher :)
Comment 10 User image Christian Holler (:decoder) 2013-12-15 05:00:34 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/75a2bbd8c4b6
Comment 11 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-12-17 14:51:02 PST
I don't think this needs QA verification. If anyone thinks that's a mistake please remove the [qa-] whiteboard tag and add the verifyme keyword.

Note You need to log in before you can comment on or make changes to this bug.