Bug 195131 – Fix incorrect handling of try-finally completion values.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

<rdar://problem/46222079>

[Mark Lam]

Created attachment 363158 [details]
proposed patch.

[Mark Lam]

Comment on attachment 363158 [details]
proposed patch.

Talked to Saam.  This fix is incorrect.  New patch coming soon.

[Mark Lam]

Created attachment 363188 [details]
work in progress for EWS testing only.

[Mark Lam]

The real bug I was investigating turns out to be due to incorrect handling of completion values of try-finally blocks.

The relevant specs:
Section 13.15.8: https://www.ecma-international.org/ecma-262/9.0/index.html#sec-try-statement-runtime-semantics-evaluation
Section 6.3.2.4: https://www.ecma-international.org/ecma-262/9.0/index.html#sec-updateempty

[Mark Lam]

Created attachment 363763 [details]
proposed patch.

[Saam Barati]

Comment on attachment 363763 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=363763&action=review

> Source/JavaScriptCore/ChangeLog:39
> +        One might be deceived into thinking that the above example should complete with
> +        the exception thrown at line 7.  However, according to Section 13.15.8 of the
> +        ECMAScript spec, the 'continue' in the finally at line 9 counts as an abrupt
> +        completion.  As a result, it overrides the throw from line 7.  After the continue,
> +        execution resumes at the top of the top of the loop at line 5, followed by a
> +        normal completion at line 11.

Crazy example.

> Source/JavaScriptCore/ChangeLog:60
> +        4. Allocate the FinallyContext on the stack instead of as a member of the
> +           heap allocated ControlFlowScope.  This simplifies its life-cycle management
> +           and reduces the amount of needed copying.

I'm not sure it's worth reverting behavior on this. I feel like it'd be cleaner just to make it an inline field again and use a move constructor to avoid copying the object.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:201
> +FinallyContext::~FinallyContext()
> +{
> +    m_completion.typeRegister = nullptr;
> +    m_completion.valueRegister = nullptr;
> +}

Why? The default destructor already does this.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:273
> +            // finally need to rethrow. catch does not.

make this: Finally needs to rethrow. Catch does not.
Alternatively, just remove the comment.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4814
> +            if (hasBreaksOrContinuesNotCoveredByJumps) {

name nit: Let's call this something like hasBreaksOrContinuesThatEscapeFinally (or similar). As we discussed in person, renaming may be more apt for a follow-up

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4816
> +                emitJumpIf<OpBeloweq>(context.completionTypeRegister(), CompletionType::Throw, isThrowOrNormalLabel.get());

Can you also add static_assert(return > throw && return > normal)?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:160
> +        struct {
> +            RefPtr<RegisterID> typeRegister;
> +            RefPtr<RegisterID> valueRegister;
> +        } m_completion;

Why in a struct?

[Yusuke Suzuki]

Comment on attachment 363763 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=363763&action=review

r=me too, with some suggestions.

> Source/JavaScriptCore/ChangeLog:17
> +                            throw '';               // line 7

OK. This produces [[Type] == throw, but

> Source/JavaScriptCore/ChangeLog:19
> +                            continue;               // line 9

This makes Completion of try-finally { continue, undefined }, and execution goes on.

> Source/JavaScriptCore/ChangeLog:21
> +                    }

Here, the completion becomes { normal, undefined }.

> Source/JavaScriptCore/ChangeLog:22
> +                }                                   // line 11

Then, try-finally's completion becomes { return, 42 } because finally block produces normal completion.

> Source/JavaScriptCore/ChangeLog:38
> +        execution resumes at the top of the top of the loop at line 5, followed by a

/the top of the top of the/the top of the/

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:271
> +    for (auto& tuple : m_exceptionHandlersToEmit) {
>          Ref<Label> realCatchTarget = newLabel();
> +        TryData* tryData = std::get<0>(tuple);
> +
>          OpCatch::emit(this, std::get<1>(tuple), std::get<2>(tuple));
>          realCatchTarget->setLocation(*this, m_lastInstruction.offset());
> +        if (std::get<3>(tuple).isValid()) {

tuple hurts readability... can we replace it with a struct?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4788
> +            // completion type to Normal. We can also  set the completion value to undefined,

unnecessary space after "also".

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4823
> +                // Not Throw means we have a Break or Continue that should be handled by the outer context.
> +                // These are for Break or Continue completions that have not reached their jump targets
> +                // yet. The outer context needs to run its finally, and resume the jump outwards (unless
> +                // interrupted by another abrupt completion). So, we need to pass the completion type to
> +                // the outer finally. Again, we can skip the completion value because it's not used for
> +                // Break nor Continue.

This means the following code, is it right?
I think it would be nice if we can have the example for each emitFinallyCompletion's case.

label: {
    try {
        try {
            break label;
        } finally {
            // this completion is here.
        }
    } finally {
        // we need to jump here next.
    }
}

[Mark Lam]

Comment on attachment 363763 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=363763&action=review

Thanks for the reviews.

>> Source/JavaScriptCore/ChangeLog:60
>> +           and reduces the amount of needed copying.
> 
> I'm not sure it's worth reverting behavior on this. I feel like it'd be cleaner just to make it an inline field again and use a move constructor to avoid copying the object.

I still think the current form is easier to reason about.  So, I'm going to leave it as is.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:201
>> +}
> 
> Why? The default destructor already does this.

I agree.  Not needed.  Will remove.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:271
>> +        if (std::get<3>(tuple).isValid()) {
> 
> tuple hurts readability... can we replace it with a struct?

I'll do this in a follow up patch.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4814
>> +            if (hasBreaksOrContinuesNotCoveredByJumps) {
> 
> name nit: Let's call this something like hasBreaksOrContinuesThatEscapeFinally (or similar). As we discussed in person, renaming may be more apt for a follow-up

Will do in a follow up.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4816
>> +                emitJumpIf<OpBeloweq>(context.completionTypeRegister(), CompletionType::Throw, isThrowOrNormalLabel.get());
> 
> Can you also add static_assert(return > throw && return > normal)?

I thought about adding this, but the assert we really want is that (all jumpIDs > throw && all jumpIDs > normal).  I couldn't think of how to express that in a meaningful way, and ended up leaving out the assert.  That said, I think I'll at least add the following to serve as documentation even if it isn't a strong guarantee about jumpIDs.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4823
>> +                // Break nor Continue.
> 
> This means the following code, is it right?
> I think it would be nice if we can have the example for each emitFinallyCompletion's case.
> 
> label: {
>     try {
>         try {
>             break label;
>         } finally {
>             // this completion is here.
>         }
>     } finally {
>         // we need to jump here next.
>     }
> }

Yes.  That example is correct.  I agree that example JS code does help make this code easier to understand.  I'll add these in a follow up patch.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:160
>> +        } m_completion;
> 
> Why in a struct?

The spec speaks of a Completion record.  This helps communicate that concept and also documents the fact that these 2 registers work together as a pair.

[Mark Lam]

Created attachment 363826 [details]
patch for landing.

[Saam Barati]

Comment on attachment 363763 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=363763&action=review

>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:160
>>> +        } m_completion;
>> 
>> Why in a struct?
> 
> The spec speaks of a Completion record.  This helps communicate that concept and also documents the fact that these 2 registers work together as a pair.

can you call it m_completionRecord then?

[Mark Lam]

(In reply to Saam Barati from comment #10)
> Comment on attachment 363763 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363763&action=review
> 
> >>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:160
> >>> +        } m_completion;
> >> 
> >> Why in a struct?
> > 
> > The spec speaks of a Completion record.  This helps communicate that concept and also documents the fact that these 2 registers work together as a pair.
> 
> can you call it m_completionRecord then?

Will do.

[Mark Lam]

Landed in r242591: <http://trac.webkit.org/r242591>.