Closed Bug 718531 Opened 8 years ago Closed 5 years ago

can't override return with throw in a finally block

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: dherman, Assigned: jandem)

Details

Attachments

(1 file)

This function:

    function test() {
        try {
            try {
                return 12
            } finally {
                throw "oh no you don't"
            }
        } catch (e) { }
        print("ha!");
    }

Should print "ha!" and then return undefined, if I'm reading 12.14 correctly (the try-finally block should disregard the return because the finally block produces a non-normal result), but in SpiderMonkey it returns 12. FWIW, Safari and Chrome both return undefined.

Dave
CC'ing Jesse, who may want to feed this morsel to his fuzzer.

Dave
This is in the bytecode emitter, since returns inside try-finally blocks are emitted as |setrval; gosub ...; retrval|. So if we have nonlocal control flow in the finally block and we never get to |retsub|, rval is not cleared.

I guess nonlocal control flow inside finally need to clear rval? Maybe there's a cleaner solution I'm not seeing.
(In reply to Shu-yu Guo [:shu] from comment #2)
> I guess nonlocal control flow inside finally need to clear rval? Maybe
> there's a cleaner solution I'm not seeing.

Would it not be sufficient to clear the rval on implicit return, i.e., with an extra opcode tacked onto the end of the whole bytecode script? That might be expensive, so in cases where you can prove it's unnecessary you could leave it out.

Dave
(In reply to Dave Herman [:dherman] from comment #3)
> (In reply to Shu-yu Guo [:shu] from comment #2)
> > I guess nonlocal control flow inside finally need to clear rval? Maybe
> > there's a cleaner solution I'm not seeing.
> 
> Would it not be sufficient to clear the rval on implicit return, i.e., with
> an extra opcode tacked onto the end of the whole bytecode script? That might
> be expensive, so in cases where you can prove it's unnecessary you could
> leave it out.
> 
> Dave

You're right, what I suggested isn't sound. Your suggested fix would be the simplest to implement. I suppose entry/exit from finally blocks could be finneagled to save/restore rval, but it seems like a PITA.
Another test case:

    function test() {
        L: try {
            return 12
        } finally {
            break L
        }
    }
    test()

Dave
Status?
Assignee: general → nobody
Both cases are still buggy, although the symptoms of the first have changed. It now, bizarrely, both prints "ha!" (as it should) and returns 12 (as it should not).
This is silly, will fix.
Flags: needinfo?(jdemooij)
Attached patch PatchSplinter Review
Emit an explicit "return undefined" if the script has a finally block.

I considered always doing this, but it will require changes to JSScript::isEmpty() and try-finally is very uncommon so I think only doing it in that case makes sense.

Patch also updates an ancient comment.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8564949 - Flags: review?(shu)
And wow, this bug was filed exactly 3 years ago, 2012-01-16 :)
(In reply to Jan de Mooij [:jandem] from comment #11)
> And wow, this bug was filed exactly 3 years ago, 2012-01-16 :)

Oops, I'm stupid.

*hides*
Comment on attachment 8564949 [details] [diff] [review]
Patch

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

Thanks! I wonder how this bug completely fell off my radar for 3 years.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3160,5 @@
> +            if (bce->hasTryFinally) {
> +                if (Emit1(cx, bce, JSOP_UNDEFINED) < 0)
> +                    return false;
> +                if (Emit1(cx, bce, JSOP_RETURN) < 0)
> +                    return false;

Any reason for having |undefined; return| over |undefined; setrval|?

In particular, is this going to result in a small bit of extraneous code generated by baseline for functions with try-finally, since all scripts already end with retrval.
Attachment #8564949 - Flags: review?(shu) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e87ae428ae

(In reply to Shu-yu Guo [:shu] from comment #13)
> Any reason for having |undefined; return| over |undefined; setrval|?

It doesn't really matter I think. |undefined; return| seemed more efficient than |undefined; setrval; retrval|.

> In particular, is this going to result in a small bit of extraneous code
> generated by baseline for functions with try-finally, since all scripts
> already end with retrval.

No, the bytecode analysis knows the JSOP_RETRVAL is unreachable so baseline shouldn't emit any code for it.
https://hg.mozilla.org/mozilla-central/rev/f6e87ae428ae
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.