990787 - Fix a bunch of OOM bugs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: bug-990787-part-1-shrinkElements-v1.patch

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: 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.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: bug-990787-part-3-js_InitArrayClass-v1.patch

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: bug-990787-part-4-Shape-search-v1.patch

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: 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.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: bug-990787-part-6-Compression-v1.patch

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: bug-990787-part-7-sps-v1.patch

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: bug-990787-part-8-WeakMap-set-v1.patch

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: bug-990787-part-9-TokenStream-v1.patch

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: bug-990787-part-10-addPredecessor-v1.patch

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: bug-990787-part-11-IonAnalysis-v1.patch

Comment 13

[Andrew McCreight [:mccr8]]

I'm going to set this to sec-other, because it looks like there's no known sec issues here.  Please adjust as needed.

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: bug-990787-part-12-MakeMIRTypeSet-v1.patch

Comment 15

[Shu-yu Guo [:shu]]

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.

Comment 16

[Hannes Verschore [:h4writer]]

Comment on attachment 8400595 [details] [diff] [review]
bug-990787-part-10-addPredecessor-v1.patch

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

Good work!

Comment 17

[Jason Orendorff [:jorendorff]]

(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.

Comment 18

[Jan de Mooij [:jandem]]

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 {}

Comment 19

[Jason Orendorff [:jorendorff]]

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.

Comment 20

[Jeff Walden [:Waldo]]

Comment on attachment 8400591 [details] [diff] [review]
bug-990787-part-8-WeakMap-set-v1.patch

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

Obvs.

Comment 21

[Nicholas Nethercote [inactive]]

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.

Comment 22

[Jason Orendorff [:jorendorff]]

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.

Comment 23

[Jason Orendorff [:jorendorff]]

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

Comment 25

[Carsten Book [:Tomcat]]

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