Closed Bug 755099 Opened 8 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed
firefox48 --- unaffected
firefox49 --- unaffected
firefox-esr45 --- affected
firefox50 --- unaffected

People

(Reporter: brendan, Assigned: jorendorff)

References

Details

(Keywords: regression, Whiteboard: [js:p1][js:bumped:5][js:ni])

Attachments

(1 file)

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
Whiteboard: [js:p1:fx16][js:ni]
Blocks: 649576
Summary: Extricate JSHashTable from JSAtomList death grip → Wrong output due to 'Extricate JSHashTable from JSAtomList death grip'
No longer blocks: 647367
Summary: Wrong output due to 'Extricate JSHashTable from JSAtomList death grip' → var e inside catch (e) not hoisted after patch for bug 649576
(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
Assignee: general → 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.
Whiteboard: [js:p1:fx16][js:ni] → [js:p1:fx17][js:bumped:1][js:ni]
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.
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))"...
Whiteboard: [js:p1:fx17][js:bumped:1][js:ni] → [js:p1:fx18][js:bumped:2][js:ni]
> 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
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.
Attachment #669200 - Flags: review?
Attachment #669200 - Flags: review? → review?(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.
Attachment #669200 - Flags: review?(luke) → review+
Whiteboard: [js:p1:fx18][js:bumped:2][js:ni] → [js:p1:fx21][js:bumped:5][js:ni]
Whiteboard: [js:p1:fx21][js:bumped:5][js:ni] → [js:p1][js:bumped:5][js:ni]
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
Flags: needinfo?(cpeterson)
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.
Depends on: 1263355
Fixed in Firefox 47 by bug 1225041.
Status: NEW → RESOLVED
Closed: 3 years ago
Depends on: 1225041
Flags: needinfo?(cpeterson)
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.