755099 - var e inside catch (e) not hoisted after patch for bug 649576

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

This testcase, from es-discuss today (), regressed due to the patch for bug 649576 (according to hg bisect on mozilla-central):

if (typeof console == 'undefined') var console = {log:print};

(function(){

try {
 throw "hi";
} catch (e) {
 var e = "ho";
 var o = "hu";
 var u;
 console.log(e);
}
console.log(e,u,o);

}()); 

Expected results:

ho
undefined undefined hu

Actual results (for above test in /tmp/x.js):

ho
/tmp/x.js:13: ReferenceError: e is not defined

I thought we had test coverage of var in catch but somehow we don't -- haven't looked deeper.

Someone with frontend skills, please take this bug, and if you are extra brave fix it by making all var e in catch (e) an early error (easiest fix) per proposed ES6.

/be

Comment 1

[Brendan Eich [:brendan]]

(In reply to Brendan Eich [:brendan] from comment #0)
> This testcase, from es-discuss today (),

Missing link:

https://mail.mozilla.org/pipermail/es-discuss/2012-May/022749.html

/be

Comment 2

[Jason Orendorff [:jorendorff]]

Found the bug, I think, but the obvious fix doesn't work.

The regressing patch does this:

>+        /* Find the first non-let binding of this atom. */
>         while (dn->kind() == JSDefinition::LET) {
>-            do {
>-                ale = ALE_NEXT(ale);
>-            } while (ale && ALE_ATOM(ale) != atom);
>-            if (!ale)
>+            mdl.popFront();
>+            if (mdl.empty())
>                 break;
>-            dn = ALE_DEFN(ale);
>+            dn = mdl.front();
>         }
> 
>-        if (ale) {
>+        if (dn) {
>             JS_ASSERT_IF(data->op == JSOP_DEFCONST,
>                          dn->kind() == JSDefinition::CONST);
>             return JS_TRUE;
>         }

It used to be, if the break-statement executed, ale was NULL, so the subsequent if-block would be skipped.

After that patch, dn is always non-NULL on exit from the loop, so we always return JS_TRUE and all the code after that is dead. At least, that's how it looks to me.

However just setting dn = NULL before breaking triggers assertions. More on Monday.

Comment 3

[Jason Orendorff [:jorendorff]]

The code has been rearranged a lot since then. Now the problem, I think, is in BindVarOrConst.  LexicalLookup does its job properly, and so does pc->decls().lookupMulti(). We just don't do the right thing with the results. We never call define() unless LexicalLookup comes up empty.

However it's not as simple as just calling define(). That trips an assertion in ParseContext::define.

Comment 4

[Luke Wagner [:luke]]

Ah, I think this explains the "dead" code that was removed in bug 775734: I suspect the code was live until the bug reported in comment 0 killed it.

> and if you are extra
> brave fix it by making all var e in catch (e) an early error (easiest fix)
> per proposed ES6.

Thank goodness, let's do this!  I wonder if the fix is as simple as removing the final "&& (stmt->type != STMT_CATCH || OuterLet(pc, stmt, name))"...

Comment 5

[Dave Herman [:dherman]]

> Thank goodness, let's do this!  I wonder if the fix is as simple as removing
> the final "&& (stmt->type != STMT_CATCH || OuterLet(pc, stmt, name))"...

+1

We don't know for sure whether making it an early error will break the web, since the current behavior is a softer failure mode. But I say let's try it.

Dave

Comment 6

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=b6de6f442c3e

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v1 - Ban `var e` in `catch (e)` block.

Green on try server. Since it's a breaking change, I want to land this in after the FF18 train departs. This means the bug will ship in FF18.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 669200 [details] [diff] [review]
v1 - Ban `var e` in `catch (e)` block.

Sweet.  I think you can nix OuterLet as well.

Comment 9

[Mark S. Miller]

Status?

See https://code.google.com/p/google-caja/issues/detail?id=1470

Comment 10

[Michelle Funches - QA]

Chris, can you provide guidance - Should this be closed now per Comment9?
Also:  Fx 47.0.1 I only see "undefined" when testing per original problem as stated in the Description.
Version 	47.0.1
Build ID 	20160623154057
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0

Comment 11

[Jeff Walden [:Waldo]]

Nobody's going to be trying to touch any aspect of this (if any touching is necessary) until the frontend rewrite is done and landed, I think.

Comment 12

[Chris Peterson [:cpeterson]]

Fixed in Firefox 47 by bug 1225041.