208496 - Incorrect scope when |with (f)| is used inside the definition of |function f()|

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Phil Schwartau]

Reported by Itaj Sherman in the Mozilla JS Engine newsgroup:

----
... there is another incompatibility there (with ECMA and IE):

function MyFunc(par)
{
  var a = par;

  with(MyFunc)
  {
    var b = par;
  }
}

In IE both |a| and |b| get the value the actual passed argument.

With SpiderMonkey of Mozilla 1.31, |b| will be undefined, because the
lookup finds 'par' on |MyFunc| which is the first on the scope chain,
and then getProperty is activated on |MyFunc|, using js_GetArgument().
----

Comment 1

[Phil Schwartau]

Testcase added to JS testsuite:

      mozilla/js/tests/js1_5/Scope/regress-208496.js

Currently passing in Rhino; failing in SpiderMonkey.

Comment 2

[Brendan Eich [:brendan]]

Patch in a second.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: ad-hoc fix, ugly but small

I'm not going to rip out the reflection of args and local vars as properties of
their function object -- that's too big an incompatibility, and too big a
change.  It could easily increase code footprint, even though we could do
without hacks such as this one.  And it would increase data footprint for sure,
because it would require adding a separate, secondary scope (or equivalent) per
function, used only by the compiler.

/be

Comment 4

[rogerl (gone)]

Comment on attachment 125112 [details] [diff] [review]
ad-hoc fix, ugly but small

Just for my edification; the change in jsinterp.c is an optimization because
argv is NULL so the error handling in JS_InstanceOf isn't going to be used?
(But why not the same change on line 484?)

Comment 5

[Brendan Eich [:brendan]]

Rogerl: oops, missed that one -- I was not vigilant enough in hunting down
JS_InstanceOf(..., NULL) uses inside the engine.  Now that I've expanded and
reduced the one you pointed out, only the jsfile.c one is left -- and jsfile.c
is orphaned code.  Thanks, checking in.

/be

Comment 6

[Phil Schwartau]

We now have two regression tests for this bug:

   mozilla/js/tests/js1_5/Scope/regress-208496-001.js
   mozilla/js/tests/js1_5/Scope/regress-208496-002.js

I renamed mozilla/js/tests/js1_5/Scope/regress-208496.js
as regress-208496-001.js, and added regress-208496-002.

The new test, again from Itaj, checks access of static
properties of |f| from inside the |with(f)| block.

Both tests are passing in the current JS shell, and I see
no test regressions when running the JS testsuite on Linux.

Therefore marking Verified FIXED -