811111 - IonMonkey: Bailout and InvalidationBailout shoud use AssertNoGC.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

We cannot GC inside a bailout because we have no simple way to mark the bailing frame.  Being able to GC might GC a pointer only accessible in the bailed frames, which would then cause a crash if used by the interpreter after the bailout.

Mark this bug as s-s as it might be a source of crash.  If changes have to be made to the allocation mechanism done in these paths, we should consider back-porting these to fx18 and older.

Comment 1

[Nicolas B. Pierron [:nbp]]

Currently flagged as sec-critical as it might be a potential source of failure.

Comment 2

[Naveed Ihsanullah [:naveed]]

This bug is investigative. It is not s-s.

Comment 3

[Nicolas B. Pierron [:nbp]]

At first sight it seems that Stack functions that are used by the Bailouts are always asserting that they can GC, but this is not the case when report is set to DONT_REPORT_ERROR.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Ensure that no GC happen during bailouts.

This patch replace AssertCanGC() by AutoAssertNoGC in bailouts functions and ensure that Stack frame manipulation function are only asserting that a GC can happen if the report flag is not set to DONT_REPORT_ERROR.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 688988 [details] [diff] [review]
Ensure that no GC happen during bailouts.

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

R+ with the nit addressed in all places.

::: js/src/vm/Stack-inl.h
@@ +386,5 @@
>  {
> +    if (report)
> +        AssertCanGC();
> +    else
> +        AutoAssertNoGC nogc;

These guards are only going to live as long as their scope (the else block), so this won't actually have the intended effect.  As Steve pointed out in IRC, our Maybe<T> class should be able to allow you to rewrite these like so:

Maybe<AutoAssertNoGC> maybeNoGC;
if (report)
    AssertCanGC();
else
    maybeNoGC.construct();

Comment 6

[Nicolas B. Pierron [:nbp]]

Remove s-s flag as we can safely use AutoAssertNoGC in bailouts.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f3975a90f40

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1f3975a90f40