Fix a bunch of OOM bugs

RESOLVED FIXED in mozilla31

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

(Blocks: 2 bugs, {sec-other})

unspecified
mozilla31
sec-other
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(12 attachments)

4.62 KB, patch
shu
: review+
Details | Diff | Splinter Review
780 bytes, patch
jandem
: review+
Details | Diff | Splinter Review
956 bytes, patch
njn
: review+
Details | Diff | Splinter Review
812 bytes, patch
shu
: review+
Details | Diff | Splinter Review
3.13 KB, patch
njn
: review+
Details | Diff | Splinter Review
2.21 KB, patch
luke
: review+
Details | Diff | Splinter Review
4.25 KB, patch
jandem
: review+
Details | Diff | Splinter Review
709 bytes, patch
Waldo
: review+
Details | Diff | Splinter Review
20.77 KB, patch
Details | Diff | Splinter Review
5.62 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
1001 bytes, patch
jandem
: review+
Details | Diff | Splinter Review
37.46 KB, patch
jandem
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8400254 [details] [diff] [review]
bug-990787-part-1-shrinkElements-v1.patch
Assignee: general → jorendorff
(Assignee)

Comment 2

3 years ago
Created attachment 8400268 [details] [diff] [review]
bug-990787-part-2-EnsureTrackPropertyTypes-v1.patch

GetProperty will already have called markUnknown() on error. Calling it again trips an assertion. We didn't notice before because it only happens in this OOM path.
(Assignee)

Comment 3

3 years ago
Created attachment 8400270 [details] [diff] [review]
bug-990787-part-3-js_InitArrayClass-v1.patch
(Assignee)

Updated

3 years ago
Group: javascript-core-security
(Assignee)

Comment 5

3 years ago
Created attachment 8400341 [details] [diff] [review]
bug-990787-part-4-Shape-search-v1.patch
(Assignee)

Comment 6

3 years ago
Created attachment 8400342 [details] [diff] [review]
bug-990787-part-5-allocateInfallible-v1.patch

Asserting that allocation succeeded is not enough. Actually call js::CrashAtUnhandlableOOM() so the testing machinery knows what happened.
(Assignee)

Comment 7

3 years ago
Created attachment 8400536 [details] [diff] [review]
bug-990787-part-6-Compression-v1.patch
(Assignee)

Comment 8

3 years ago
Created attachment 8400589 [details] [diff] [review]
bug-990787-part-7-sps-v1.patch
(Assignee)

Comment 9

3 years ago
Created attachment 8400591 [details] [diff] [review]
bug-990787-part-8-WeakMap-set-v1.patch
(Assignee)

Comment 10

3 years ago
Created attachment 8400594 [details] [diff] [review]
bug-990787-part-9-TokenStream-v1.patch
(Assignee)

Comment 11

3 years ago
Created attachment 8400595 [details] [diff] [review]
bug-990787-part-10-addPredecessor-v1.patch
(Assignee)

Comment 12

3 years ago
Created attachment 8400596 [details] [diff] [review]
bug-990787-part-11-IonAnalysis-v1.patch
I'm going to set this to sec-other, because it looks like there's no known sec issues here.  Please adjust as needed.
Keywords: sec-other
(Assignee)

Comment 14

3 years ago
Created attachment 8400813 [details] [diff] [review]
bug-990787-part-12-MakeMIRTypeSet-v1.patch
(Assignee)

Updated

3 years ago
Blocks: 912928, 988953
(Assignee)

Updated

3 years ago
Attachment #8400254 - Flags: review?(shu)
(Assignee)

Updated

3 years ago
Attachment #8400268 - Flags: review?(jdemooij)
(Assignee)

Updated

3 years ago
Attachment #8400270 - Flags: review?(n.nethercote)
(Assignee)

Updated

3 years ago
Attachment #8400341 - Flags: review?(shu)
(Assignee)

Updated

3 years ago
Attachment #8400342 - Flags: review?(n.nethercote)
(Assignee)

Updated

3 years ago
Attachment #8400536 - Flags: review?(luke)
(Assignee)

Updated

3 years ago
Attachment #8400589 - Flags: review?(jdemooij)

Updated

3 years ago
Attachment #8400536 - Flags: review?(luke) → review+
(Assignee)

Updated

3 years ago
Attachment #8400591 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

3 years ago
Attachment #8400594 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

3 years ago
Attachment #8400595 - Flags: review?(hv1989)
(Assignee)

Updated

3 years ago
Attachment #8400596 - Flags: review?(jdemooij)
(Assignee)

Updated

3 years ago
Attachment #8400813 - Flags: review?(jdemooij)

Comment 15

3 years ago
Comment on attachment 8400254 [details] [diff] [review]
bug-990787-part-1-shrinkElements-v1.patch

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

::: js/src/jscntxt.cpp
@@ +1075,5 @@
>  {
>      JS_ASSERT(isForkJoinContext());
>      return reinterpret_cast<ForkJoinContext *>(this);
>  }
> + 

Nit: trailing whitespace

@@ +1082,5 @@
> +{
> +    // If this is not a JSContext, there's nothing to do.
> +    if (JSContext *maybecx = maybeJSContext()) {
> +        if (maybecx->isExceptionPending()) {
> +#ifdef MOZ_DEBUG

Is this the new thing to use now over DEBUG? Just wondering.
Attachment #8400254 - Flags: review?(shu) → review+

Updated

3 years ago
Attachment #8400341 - Flags: review?(shu) → review+
Comment on attachment 8400595 [details] [diff] [review]
bug-990787-part-10-addPredecessor-v1.patch

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

Good work!
Attachment #8400595 - Flags: review?(hv1989) → review+
(Assignee)

Comment 17

3 years ago
(In reply to Shu-yu Guo [:shu] from comment #15)
> Is this the new thing to use now over DEBUG? Just wondering.

No, that's a typo. Good catch.

Updated

3 years ago
Attachment #8400268 - Flags: review?(jdemooij) → review+
Comment on attachment 8400589 [details] [diff] [review]
bug-990787-part-7-sps-v1.patch

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

r=me with nits below addressed.

::: js/src/vm/Probes-inl.h
@@ +75,5 @@
>  
>      if (popSPSFrame)
>          cx->runtime()->spsProfiler.exit(script, maybeFun);
>  
> +    return true;

Change the return type of probes::ExitScript to |void| (it always returns |true|), or fix the callers to propagate OOM.

::: js/src/vm/Stack.cpp
@@ +247,1 @@
>      }

Nit: no {}
Attachment #8400589 - Flags: review?(jdemooij) → review+

Updated

3 years ago
Attachment #8400596 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 19

3 years ago
Comment on attachment 8400594 [details] [diff] [review]
bug-990787-part-9-TokenStream-v1.patch

Clearing review on part 9 because njn independently fixed it in bug 992274.
Attachment #8400594 - Flags: review?(jwalden+bmo)
Comment on attachment 8400591 [details] [diff] [review]
bug-990787-part-8-WeakMap-set-v1.patch

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

Obvs.
Attachment #8400591 - Flags: review?(jwalden+bmo) → review+
Attachment #8400270 - Flags: review?(n.nethercote) → review+
Comment on attachment 8400342 [details] [diff] [review]
bug-990787-part-5-allocateInfallible-v1.patch

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

Righteous.

::: js/src/ds/LifoAlloc.h
@@ -144,5 @@
> -    void *allocInfallible(size_t n) {
> -        void *result = tryAlloc(n);
> -        JS_ASSERT(result);
> -        return result;
> -    }

Yikes. I nominate that for "Most Misleading Function Name" award.
Attachment #8400342 - Flags: review?(n.nethercote) → review+

Updated

3 years ago
Attachment #8400813 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 22

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=6d4aa2555326

Opening as I reviewed all these bugs and I'm fairly sure there's nothing security-sensitive here.
Group: javascript-core-security
(Assignee)

Comment 23

3 years ago
This introduced some GC hazards, fixed here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/475160609573

Updated

3 years ago
Duplicate of this bug: 750278
https://hg.mozilla.org/mozilla-central/rev/5708e5bff2bc
https://hg.mozilla.org/mozilla-central/rev/9574d9188bc9
https://hg.mozilla.org/mozilla-central/rev/8305d8ac2d84
https://hg.mozilla.org/mozilla-central/rev/d00e63e84c85
https://hg.mozilla.org/mozilla-central/rev/2409ea181738
https://hg.mozilla.org/mozilla-central/rev/7228d78eb20b
https://hg.mozilla.org/mozilla-central/rev/dfa124c782ed
https://hg.mozilla.org/mozilla-central/rev/f253ea1b13f4
https://hg.mozilla.org/mozilla-central/rev/c90e6a2348a4
https://hg.mozilla.org/mozilla-central/rev/10efb7f9811b
https://hg.mozilla.org/mozilla-central/rev/cde67feab42c
https://hg.mozilla.org/mozilla-central/rev/475160609573
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.