593556 - JM: variable from outer scope is not seen in "with" block

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

(function () {
    var c = 3;
    (function () {
        with({}) {
            print(c);
        }
    })();
})();

Interpreter: 3
Method JIT:  undefined

The first bad revision is:
  changeset:   52276:9852618873d9
  user:        David Anderson <[e-mail]>
  date:        Mon Jul 12 15:00:58 2010 -0700
  summary:     [JAEGER] Remove stores from OOL transitions when
               not needed (bug 573083).

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

This may be folded into bug 585524, which is closely related.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix attempt

Most of the diff is unindenting. This patch makes primaryExpr() build use-def information for names inside |with| statements, however, it makes sure to mark the names as deoptimized.

NewBindingNode no longer steals an existing pn if it's inside a |with|.

BindNameToSlot now makes sure not to emit GETGNAME for deoptimized definitions. (GETGNAME says that the name is either on the global object or is a ReferenceError.)

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 473807 [details] [diff] [review]
fix attempt

>+            bool inWith = stmt && stmt->type == STMT_WITH;

No need for this bool, just test (stmt && stmt->type == STMT_WITH) later where you make single use of it:

>+            if (inWith)
>+                pn->pn_dflags = PND_DEOPTIMIZED;

But clobbering dflags is not a good idea if something precomputed other flags. Why not wait a bit more for where dflags is |='ed:

>+
>+            JS_ASSERT(dn->pn_defn);
>+            LinkUseToDef(pn, dn, tc);
>+
>+            /* Here we handle the backward function reference case. */
>+            if (tokenStream.peekToken() != TOK_LP)
>+                dn->pn_dflags |= PND_FUNARG;
>+
>+            pn->pn_dflags |= (dn->pn_dflags & PND_FUNARG);

and do the PND_DEOPTIMIZED |= here to keep all such together?

Looks good otherwise -- does it fix the testcases with 'with' and 'function' sub-statements?

/be

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

That clobbering was a typo. Thanks for catching.

No, it does not fix the other bugs on file. Those need more changes to BindVarOrConst, I think.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/eba54c4edd6f

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/eba54c4edd6f

Comment 7

[Mozilla RelEng Bot]

A changeset from this bug was associated with a XP Ts, Cold MED Dirty Profile regression on Firefox. boo-urns :(

  Previous: avg 420.356 stddev 3.679 of 30 runs up to a9d1ad0bc386
  New     : avg 430.832 stddev 1.905 of 5 runs since a60414d076b5
  Change  : +10.475 (2.49% / z=2.847)
  Graph   : http://mzl.la/bZFaB3

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-ts_cold_generated_med] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 8

[Mozilla RelEng Bot]

A changeset from this bug was associated with a Win7 Dromaeo (CSS) regression on Firefox. boo-urns :(

  Previous: avg 2014.419 stddev 40.480 of 30 runs up to a9d1ad0bc386
  New     : avg 1901.610 stddev 12.432 of 5 runs since a60414d076b5
  Change  : -112.809 (-5.6% / z=2.787)
  Graph   : http://mzl.la/9gFu4n

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_css] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 9

[Mozilla RelEng Bot]

A changeset from this bug was associated with a Win7 Dromaeo (jslib) regression on Firefox. boo-urns :(

  Previous: avg 127.610 stddev 4.222 of 30 runs up to a9d1ad0bc386
  New     : avg 116.384 stddev 0.751 of 5 runs since a60414d076b5
  Change  : -11.226 (-8.8% / z=2.659)
  Graph   : http://mzl.la/bZu5UP

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_jslib] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 10

[Mozilla RelEng Bot]

A changeset from this bug was associated with a XP Dromaeo (CSS) regression on Firefox. boo-urns :(

  Previous: avg 2045.275 stddev 49.676 of 30 runs up to a9d1ad0bc386
  New     : avg 1936.120 stddev 13.937 of 5 runs since a60414d076b5
  Change  : -109.155 (-5.34% / z=2.197)
  Graph   : http://mzl.la/b0dlou

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_css] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 11

[Mozilla RelEng Bot]

A changeset from this bug was associated with a XP Dromaeo (jslib) regression on Firefox. boo-urns :(

  Previous: avg 129.703 stddev 4.099 of 30 runs up to a9d1ad0bc386
  New     : avg 117.954 stddev 0.660 of 5 runs since a60414d076b5
  Change  : -11.749 (-9.06% / z=2.866)
  Graph   : http://mzl.la/avZij4

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_jslib] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.