Last Comment Bug 948023 - Refactor ReconstructStackDepth to allow handling of OOM condition
: Refactor ReconstructStackDepth to allow handling of OOM condition
Status: RESOLVED FIXED
[qa-][adv-main28-]
: assertion, sec-want
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:
Depends on:
Blocks: langfuzz 912928
  Show dependency treegraph
 
Reported: 2013-12-09 10:31 PST by Christian Holler (:decoder)
Modified: 2014-03-04 16:53 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
js-ReconstructStackDepth-oom.patch (4.19 KB, patch)
2013-12-09 10:31 PST, Christian Holler (:decoder)
jdemooij: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2013-12-09 10:31:15 PST
Created attachment 8344767 [details] [diff] [review]
js-ReconstructStackDepth-oom.patch

Right now, [@ ReconstructStackDepth] returns false both when an OOM occurs and when the *pc is unreachable. The attached patch refactors the function to return false on OOM and pass the reachable/unreachable part back in a second variable. With this patch, some OOM assertions are fixed that LangFuzz keeps hitting.
Comment 1 User image Jan de Mooij [:jandem] 2013-12-10 02:46:19 PST
Comment on attachment 8344767 [details] [diff] [review]
js-ReconstructStackDepth-oom.patch

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

Thanks.
Comment 2 User image Christian Holler (:decoder) 2013-12-10 03:32:17 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/029c51a1a898
Comment 3 User image Wes Kocher (:KWierso) 2013-12-10 16:49:37 PST
https://hg.mozilla.org/mozilla-central/rev/029c51a1a898
Comment 4 User image Chris Peterson [:cpeterson] 2013-12-12 14:04:28 PST
Christian: should this OOM fix be uplifted to Aurora 28?
Comment 5 User image Christian Holler (:decoder) 2013-12-12 16:18:21 PST
This isn't a huge refactoring, but it's still more than just adding a null check, so I suggest we just let it ride the trains. No immediate need for this on the branches.
Comment 6 User image Christian Holler (:decoder) 2013-12-12 16:24:45 PST
gkw just pointed out that Aurora 28 is going to be used as a foundation for the next B2G version. Since OOM crashes are more of an issue to them than regularly, it might be worth uplifting. I'll fill out the form.
Comment 7 User image Christian Holler (:decoder) 2013-12-12 16:25:55 PST
Comment on attachment 8344767 [details] [diff] [review]
js-ReconstructStackDepth-oom.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Crashes in OOM conditions
Testing completed (on m-c, etc.): A few days on mozilla-central
Risk to taking this patch (and alternatives if risky): Very low, this is just a simple refactoring and adding some return value checks that were previously missing/wrong.
String or IDL/UUID changes made by this patch: None
Comment 8 User image Christian Holler (:decoder) 2013-12-15 05:02:03 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/c6eb37cebcd9

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