380016 - Catchguard decompilation includes "/*EXCEPTION*/"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> function() { try { } catch(e if x) { } } 
function () {
    try {
    } catch (e if x) {
        /*EXCEPTION*/;
    }
}

Comment 1

[Bob Clary [:bc] (inactive)]

bug 379860 is at fault here.

Comment 2

[Bob Clary [:bc] (inactive)]

test js1_5/Regress/regress-346494.js covers this.

Comment 3

[Bob Clary [:bc] (inactive)]

ditto 
js1_5/extensions/regress-374589.js
js1_7/decompilation/regress-355105.js
js1_7/decompilation/regress-375794.js

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

The problem here is that an unannotated JSOP_POP simply decompiles whatever's on the stack. In this case, we want to really pop the exception cookie. It appears that we need to annotate the JSOP_POP emitted for the catchguard (for the DUP, I think) in some way so the decompiler simply pops and throws away the result.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Something like this

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 264380 [details] [diff] [review]
Something like this

Even easier: check for exception_cookie in the default JSOP_POP code and discard it. There's no way to generate such a comment that the decompiler could pop.

/be

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Easier

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 264393 [details] [diff] [review]
Easier

Sorry for driving by again: one thought is to exclude all non-empty strings that start with "/*".

/be

Comment 9

[Igor Bukanov]

Comment on attachment 264393 [details] [diff] [review]
Easier

I would prefer comments explaining why empty string and the exception cookie are special.

Comment 10

[Igor Bukanov]

(In reply to comment #8)
> (From update of attachment 264393 [details] [diff] [review])
> Sorry for driving by again: one thought is to exclude all non-empty strings
> that start with "/*".

I would prefer to keep the check as in Blake's patch but add an assert after it to ensure that the string does not start from "/*" or any other invalid chars at that point.

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> (In reply to comment #8)
> > (From update of attachment 264393 [details] [diff] [review] [details])
> > Sorry for driving by again: one thought is to exclude all non-empty strings
> > that start with "/*".
> 
> I would prefer to keep the check as in Blake's patch but add an assert after it
> to ensure that the string does not start from "/*" or any other invalid chars
> at that point.

Yeah, I considered that but tilted back toward silently popping any cookie. On reflection your preferred plan seems best to me, though.

/be

Comment 12

[Igor Bukanov]

(In reply to comment #11)
> Yeah, I considered that but tilted back toward silently popping any cookie. On
> reflection your preferred plan seems best to me, though.

An alternative is to check for /* and then assert that is the exception cookie.

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> (In reply to comment #11)
> > Yeah, I considered that but tilted back toward silently popping any cookie. On
> > reflection your preferred plan seems best to me, though.
> 
> An alternative is to check for /* and then assert that is the exception cookie.

Best yet ;-).

/be

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With the assertion

I think this is the smallest patch that has the desired behavior.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 265168 [details] [diff] [review]
With the assertion

>+                    if (*rval != '\0' && (rval[0] != '/' || rval[1] != '*')) {
>+

Nit: lose this blank line.

r=me with that.

/be

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: What I'm checking in

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

This was checked in on trunk.

Comment 18

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows.