Closed Bug 990787 Opened 6 years ago Closed 6 years ago

Fix a bunch of OOM bugs

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: jorendorff, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

(Keywords: sec-other)

Attachments

(12 files)

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
No description provided.
Assignee: general → jorendorff
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.
Group: javascript-core-security
Asserting that allocation succeeded is not enough. Actually call js::CrashAtUnhandlableOOM() so the testing machinery knows what happened.
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
Blocks: 912928
Attachment #8400254 - Flags: review?(shu)
Attachment #8400268 - Flags: review?(jdemooij)
Attachment #8400270 - Flags: review?(n.nethercote)
Attachment #8400341 - Flags: review?(shu)
Attachment #8400342 - Flags: review?(n.nethercote)
Attachment #8400536 - Flags: review?(luke)
Attachment #8400589 - Flags: review?(jdemooij)
Attachment #8400536 - Flags: review?(luke) → review+
Attachment #8400591 - Flags: review?(jwalden+bmo)
Attachment #8400594 - Flags: review?(jwalden+bmo)
Attachment #8400595 - Flags: review?(hv1989)
Attachment #8400596 - Flags: review?(jdemooij)
Attachment #8400813 - Flags: review?(jdemooij)
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+
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+
(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.
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+
Attachment #8400596 - Flags: review?(jdemooij) → review+
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+
Attachment #8400813 - Flags: review?(jdemooij) → review+
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
This introduced some GC hazards, fixed here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/475160609573
Duplicate of this bug: 750278
You need to log in before you can comment on or make changes to this bug.