819125 - try-finally should return try's completion value if finally returned normally

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20121119183901

Steps to reproduce:

Test case:
---
try{ "try-value" }finally{ "finally-value" }
---



Expected results:

Per [ES5.1, 12.14 The try Statement], the code should return "try-value", but V8, JSC, IE and Spidermonkey return "finally-value". Opera does return "try-value".

Comment 1

[Boris Zbarsky [:bzbarsky]]

We should either fix our implementation or fix the spec, for sure.

Comment 2

[Gareth Smith]

Attachment: In-browser tests for the return value of "finally" blocks

Notice in particular test 2 - I believe the return value is coming from the finally block, but the completion type of that value is coming from the try block.

Comment 3

[Gareth Smith]

Attachment: Better in-browser tests

A better version of the tests in the previous attachment. This time the tests run properly on chrome, and I've included the results of running the tests on various browsers, and a simple specification-style description of what I think the browsers are doing.

Comment 4

[Gareth Smith]

Attachment: unit-tests for the in-browser tests in the previous attachment.

Comment 5

[Tooru Fujisawa [:arai]]

bug 1202134 fixed some cases, now following cases don't work
  * |break| in finally block without any other expression statement before it
  * |continue| in finally block without any other expression statement before it

I think we should call SETRVAL with NULL (or UNDEFINED?) if leaving finally block, for eval's case.

Comment 6

[Tooru Fujisawa [:arai]]

I was wrong.  we should do SETRVAL with UNDEFINED before executing catch/finally block in eval.
I'll post the patch after test.

Comment 7

[Tooru Fujisawa [:arai]]

Here's current issues and solutions.

1. catch block's value may become try block's one

If catch block has no expression statement (or no expression statement is executed), catch block's value becomes try block's last executed expression statement result.

e.g. |eval(" try { 1; throw 2; } catch (e) {} ")| should be undefined, but it becomes 1 with current implementation.

So, we should set frame's return value to undefined before executing catch block.


2. finally block's value may become try/catch block's one

finally block's value is visible only if it exits with break/continue (am I correct?), if there's no expression statement executed before break/continue, finally block's value becomes try/catch block's last executed expression statement result.

e.g. |eval("do { try { 1; } finally { break; } } while (false);")| should be undefined, but it becomes 1 with current implementation.

So, we should set frame's return value to undefined before executing catch block.

Comment 8

[Tooru Fujisawa [:arai]]

> So, we should set frame's return value to undefined before executing catch block.
*finally* block, for 2nd one :P

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Reset return value before executing catch/finally block if script needs value.

Almost green on try runs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8efd648373c5 (not sure the reason tho, all jobs are queued thrice, cancelled all of them :P)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c9e95a9be29 (added test)

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8679733 [details] [diff] [review]
Reset return value before executing catch/finally block if script needs value.

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

Thanks! Also great tests :)

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4984,5 @@
>          for (ParseNode* pn3 = catchList->pn_head; pn3; pn3 = pn3->pn_next) {
>              MOZ_ASSERT(this->stackDepth == depth);
>  
> +            if (!script->noScriptRval()) {
> +                // If script needs value, clear the frame's return value that

Nit: I think it's fine to always emit a SETRVAL at the start of catch/finally blocks, so we can simplify this a bit by removing the if statement and the first part of the comment.

Also "return value" may confuse future readers (if they think about functions), so an example may help:

// Clear the frame's return value that might have been set by the
// try block:
//
//   eval("try { 1; throw 2 } catch(e) {}"); // undefined, not 1

@@ +5050,5 @@
> +
> +        if (!script->noScriptRval()) {
> +            // If script needs value, clear the frame's return value to
> +            // make break/continue return correct value even if there is no
> +            // other statement before them.

Same here.

Example could be like this:

// ...if there's no other statement before them:
// 
//   eval("x: try { 1 } finally { break x; }");  // undefined, not 1

Comment 11

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3b3ccf98cbcdbc4f8c76a03ee4d54fb420c4930
Bug 819125 - Reset return value before executing catch/finally block. r=jandem

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d3b3ccf98cbc