Refactor ReconstructStackDepth to allow handling of OOM condition

RESOLVED FIXED in Firefox 28

Status

()

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

People

(Reporter: decoder, Assigned: decoder)

Tracking

(Blocks: 2 bugs, {assertion, sec-want})

Trunk
mozilla29
x86_64
Linux
assertion, sec-want
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28 fixed, firefox29 fixed)

Details

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

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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.
Attachment #8344767 - Flags: review?(jdemooij)
(Assignee)

Updated

4 years ago
Blocks: 912928
Comment on attachment 8344767 [details] [diff] [review]
js-ReconstructStackDepth-oom.patch

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

Thanks.
Attachment #8344767 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 2

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/029c51a1a898
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/029c51a1a898
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Christian: should this OOM fix be uplifted to Aurora 28?
Flags: needinfo?(choller)
(Assignee)

Comment 5

4 years ago
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.
Flags: needinfo?(choller)
(Assignee)

Comment 6

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

Comment 7

4 years ago
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
Attachment #8344767 - Flags: approval-mozilla-aurora?
Attachment #8344767 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 8

4 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/c6eb37cebcd9
status-firefox28: --- → fixed
status-firefox29: --- → fixed

Updated

3 years ago
Whiteboard: [qa-]
Whiteboard: [qa-] → [qa-][adv-main28-]
You need to log in before you can comment on or make changes to this bug.