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

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: anba, Assigned: arai)

Tracking

(Blocks: 1 bug)

Trunk
mozilla44
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

6 years ago
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".
We should either fix our implementation or fix the spec, for sure.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

6 years ago
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

6 years ago
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.
Assignee: general → nobody
(Reporter)

Updated

4 years ago
Blocks: 694100
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.
I was wrong.  we should do SETRVAL with UNDEFINED before executing catch/finally block in eval.
I'll post the patch after test.
Assignee: nobody → arai.unmht
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.
> So, we should set frame's return value to undefined before executing catch block.
*finally* block, for 2nd one :P
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
Attachment #8679733 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/d3b3ccf98cbc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.