As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 990787 - Fix a bunch of OOM bugs
: Fix a bunch of OOM bugs
Status: RESOLVED FIXED
: sec-other
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla31
Assigned To: Jason Orendorff [:jorendorff]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 750278 (view as bug list)
Depends on:
Blocks: 912928 988953
  Show dependency treegraph
 
Reported: 2014-04-01 16:06 PDT by Jason Orendorff [:jorendorff]
Modified: 2014-04-09 15:37 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
bug-990787-part-1-shrinkElements-v1.patch (4.62 KB, patch)
2014-04-01 16:16 PDT, Jason Orendorff [:jorendorff]
shu: review+
Details | Diff | Splinter Review
bug-990787-part-2-EnsureTrackPropertyTypes-v1.patch (780 bytes, patch)
2014-04-01 16:35 PDT, Jason Orendorff [:jorendorff]
jdemooij: review+
Details | Diff | Splinter Review
bug-990787-part-3-js_InitArrayClass-v1.patch (956 bytes, patch)
2014-04-01 16:36 PDT, Jason Orendorff [:jorendorff]
n.nethercote: review+
Details | Diff | Splinter Review
bug-990787-part-4-Shape-search-v1.patch (812 bytes, patch)
2014-04-01 19:13 PDT, Jason Orendorff [:jorendorff]
shu: review+
Details | Diff | Splinter Review
bug-990787-part-5-allocateInfallible-v1.patch (3.13 KB, patch)
2014-04-01 19:16 PDT, Jason Orendorff [:jorendorff]
n.nethercote: review+
Details | Diff | Splinter Review
bug-990787-part-6-Compression-v1.patch (2.21 KB, patch)
2014-04-02 03:13 PDT, Jason Orendorff [:jorendorff]
luke: review+
Details | Diff | Splinter Review
bug-990787-part-7-sps-v1.patch (4.25 KB, patch)
2014-04-02 05:10 PDT, Jason Orendorff [:jorendorff]
jdemooij: review+
Details | Diff | Splinter Review
bug-990787-part-8-WeakMap-set-v1.patch (709 bytes, patch)
2014-04-02 05:10 PDT, Jason Orendorff [:jorendorff]
jwalden+bmo: review+
Details | Diff | Splinter Review
bug-990787-part-9-TokenStream-v1.patch (20.77 KB, patch)
2014-04-02 05:13 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
bug-990787-part-10-addPredecessor-v1.patch (5.62 KB, patch)
2014-04-02 05:14 PDT, Jason Orendorff [:jorendorff]
hv1989: review+
Details | Diff | Splinter Review
bug-990787-part-11-IonAnalysis-v1.patch (1001 bytes, patch)
2014-04-02 05:16 PDT, Jason Orendorff [:jorendorff]
jdemooij: review+
Details | Diff | Splinter Review
bug-990787-part-12-MakeMIRTypeSet-v1.patch (37.46 KB, patch)
2014-04-02 11:49 PDT, Jason Orendorff [:jorendorff]
jdemooij: review+
Details | Diff | Splinter Review

Description User image Jason Orendorff [:jorendorff] 2014-04-01 16:06:42 PDT

    
Comment 1 User image Jason Orendorff [:jorendorff] 2014-04-01 16:16:10 PDT
Created attachment 8400254 [details] [diff] [review]
bug-990787-part-1-shrinkElements-v1.patch
Comment 2 User image Jason Orendorff [:jorendorff] 2014-04-01 16:35:17 PDT
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.
Comment 3 User image Jason Orendorff [:jorendorff] 2014-04-01 16:36:11 PDT
Created attachment 8400270 [details] [diff] [review]
bug-990787-part-3-js_InitArrayClass-v1.patch
Comment 5 User image Jason Orendorff [:jorendorff] 2014-04-01 19:13:28 PDT
Created attachment 8400341 [details] [diff] [review]
bug-990787-part-4-Shape-search-v1.patch
Comment 6 User image Jason Orendorff [:jorendorff] 2014-04-01 19:16:42 PDT
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.
Comment 7 User image Jason Orendorff [:jorendorff] 2014-04-02 03:13:42 PDT
Created attachment 8400536 [details] [diff] [review]
bug-990787-part-6-Compression-v1.patch
Comment 8 User image Jason Orendorff [:jorendorff] 2014-04-02 05:10:09 PDT
Created attachment 8400589 [details] [diff] [review]
bug-990787-part-7-sps-v1.patch
Comment 9 User image Jason Orendorff [:jorendorff] 2014-04-02 05:10:43 PDT
Created attachment 8400591 [details] [diff] [review]
bug-990787-part-8-WeakMap-set-v1.patch
Comment 10 User image Jason Orendorff [:jorendorff] 2014-04-02 05:13:35 PDT
Created attachment 8400594 [details] [diff] [review]
bug-990787-part-9-TokenStream-v1.patch
Comment 11 User image Jason Orendorff [:jorendorff] 2014-04-02 05:14:56 PDT
Created attachment 8400595 [details] [diff] [review]
bug-990787-part-10-addPredecessor-v1.patch
Comment 12 User image Jason Orendorff [:jorendorff] 2014-04-02 05:16:22 PDT
Created attachment 8400596 [details] [diff] [review]
bug-990787-part-11-IonAnalysis-v1.patch
Comment 13 User image Andrew McCreight [:mccr8] 2014-04-02 10:12:26 PDT
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 User image Jason Orendorff [:jorendorff] 2014-04-02 11:49:20 PDT
Created attachment 8400813 [details] [diff] [review]
bug-990787-part-12-MakeMIRTypeSet-v1.patch
Comment 15 User image Shu-yu Guo [:shu] 2014-04-07 11:05:03 PDT
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 User image Hannes Verschore [:h4writer] 2014-04-07 11:12:56 PDT
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 User image Jason Orendorff [:jorendorff] 2014-04-07 11:26:09 PDT
(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 User image Jan de Mooij [:jandem] 2014-04-07 12:05:54 PDT
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 User image Jason Orendorff [:jorendorff] 2014-04-07 12:34:37 PDT
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 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-04-07 13:29:32 PDT
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 User image Nicholas Nethercote [:njn] 2014-04-07 18:20:26 PDT
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 User image Jason Orendorff [:jorendorff] 2014-04-08 11:44:01 PDT
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 User image Jason Orendorff [:jorendorff] 2014-04-08 13:44:52 PDT
This introduced some GC hazards, fixed here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/475160609573
Comment 24 User image Sean Stangl [:sstangl] 2014-04-08 15:01:20 PDT
*** Bug 750278 has been marked as a duplicate of this bug. ***

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