99663 - top disappears when you run a for loop on it.

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Steven Toth]

When you run a for loop over the top variable, it causes it to become undefined.

See attached html file for an example.

Comment 1

[Steven Toth]

Attachment: See comment for usage.

Comment 2

[Steven Toth]

To use the test html page.

1. Click on ALERT TOP, notice that a message with the top object and the url of 
the top object are displayed.

2. Click on SHOW STATUS, in the status bar a simple 'OK!' message is displayed.

3. Click on cause error, shows the results of a for loop on the window object.

4. Click on SHOW STATUS, this tries, top.status='OK!' and fails, generating an 
exception.  The exception is caught, and an alert is displayed saying as such.

5. Click on ALERT TOP.   This generated an exception, see it in the JavaScript 
console window.  Since the exception was uncaught, the page then reloads.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I think I've seen other bugs along these lines...

Seeing this on Linux as well.

Comment 4

[Johnny Stenback (:jst)]

Hey Brendan,

The JS:

  for (xx in window) {
    // ...
  }

ends up calling the resolve hook on the window object with the flag
JSRESOLVE_ASSIGNING set, that's the cause of this problem since the resolve hook
sees that our "replaceable" (i.e. pseudo-readonly) properties are being set at
which point it defines a property with the name of the replaceable property on
the global object so that the property value can be set. The DOM code sets the
value of this property to undefined since the real value is thought to be "on
its way".

Is this as designed, or is this a JS engine bug? 

Comment 5

[Brendan Eich [:brendan]]

JS engine bug, and an old one at that.  Taking...

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: proposed fix, please test and review

Comment 7

[Brendan Eich [:brendan]]

Am I gilding the lily?  I need to keep the JSRESOLVE_ASSIGNING commitment.  The
cost is small, although the cost of yet another byte-store (among many stores)
during frame initialization bugs me quite a lot.  Probably the thing to do is
address frame set-up overhead in a sequel bug.

/be

Comment 8

[Mike Shaver (:shaver emeritus)]

Comment on attachment 51088 [details] [diff] [review]
proposed fix, please test and review

If you're worried about assignment costs, maybe we should group the initialized-as-0 members together and memset(0) them.  I recall that being faster than explicit assignment for sets of a certain length, but disassembly and A-B testing will tell the tale.

Regardless, sr=shaver in the interim.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 51088 [details] [diff] [review]
proposed fix, please test and review

r=jst

Comment 10

[Brendan Eich [:brendan]]

Attachment: alterna-patch, bigger but better (fp->flags, uint32; bonus fixes)

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 51262 [details] [diff] [review]
alterna-patch, bigger but better (fp->flags, uint32; bonus fixes)

Fix for bug 99663 (for loop resolves properties of the object being enumerated
with JSRESOLVE_ASSIGNING, wrongly), plus a few miscellaneous bugfixes.

- Combine the JSStackFrame members constructing, special, overrides, and
  reserved into a uint32 flags member.

- Separate JOF_ASSIGNING from the JOF_SET bytecode format flag, and impute
  JSRESOLVE_ASSIGNING from the presence of JOF_ASSIGNING among the current
  opcode's format flags.  To handle the for-in loop opcodes, which do more
  than simply assign -- in particular, they do property lookups whose resolve
  hook outcalls should not be flagged with JSRESOLVE_ASSIGNING -- a new frame
  flag, JSFRAME_ASSIGNING, has been added.

- Fix interpreter version selection to respect JS_SetVersion, whose effect on
  cx->version is "sticky".

- Fix js_DecompileValueGenerator to deal with JSOP_ENUMELEM -- it never had,
  as this testcase shows (it crashes without this patch):

    version(120);
    eval("function fe(s) { for (it[s] in this); }");
    try { fe('rdonly'); } catch (e) { print(e); }

Comment 12

[Brendan Eich [:brendan]]

The JS_SetVersion stickyness is necessary for tests such as this one:


to work properly.  I think the existing js/tests have been lucky in dodging the
buggy way that JS_SetVersion's effect can be undone by function return.

Second patch is ready for r= and sr= -- if for some reason you want me to hold
it for 0.9.6, I'll consider checking in the first patch.  But the second patch
is just as good (testsuite passes).  Thanks,

/be

Comment 13

[Brendan Eich [:brendan]]

Oops!  Paste error on that inline testcase.  Here it is:

version(120)

eval("function fn() { with (it) for (rdonly in this); }")

try { fn() } catch (e) { print(e) }

eval("function fp() { for (it.rdonly in this); }")

try { fp() } catch (e) { print(e) }

eval("function fe(s) { for (it[s] in this); }")

try { fe('rdonly') } catch (e) { print(e) }

Each catch should fire and print "Error: rdonly is read-only" (e.g. -- the 2nd
and 3rd should print it.rdonly and it[s], respectively).

/be

Comment 14

[Brendan Eich [:brendan]]

Please don't make me check in on Tuesday near midnight!

/be

Comment 15

[Mike Shaver (:shaver emeritus)]

Comment on attachment 51262 [details] [diff] [review]
alterna-patch, bigger but better (fp->flags, uint32; bonus fixes)

>-    ok = js_Execute(cx, scopeobj, script, caller, fp->special & JSFRAME_EVAL,
>-                    rval);
>+    ok = js_Execute(cx, scopeobj, script, caller, JSFRAME_EVAL, rval);
>     JS_DestroyScript(cx, script);

Do we really want to unconditionally call with JSFRAME_EVAL, rather than
(fp->flags & JSFRAME_EVAL)?

(I do so love patches with "bonus fixes".)

Comment 16

[Brendan Eich [:brendan]]

shaver: we know fp->flags contains JSFRAME_EVAL, it was set there at line 1007.
 This was true for fp->special as well.  IIRC, the & expression for the
js_Execute argument used to be necessary, because JSFRAME_EVAL was |='d into
fp->special only if (!indirectCall) -- see `cvs diff -u -r3.{79,80} jsobj.c`. 
Anyway, we now know that the bit is set (nothing clears it from a given frame's
flags), so there is no need to &.

/be

Comment 17

[Mike Shaver (:shaver emeritus)]

Comment on attachment 51262 [details] [diff] [review]
alterna-patch, bigger but better (fp->flags, uint32; bonus fixes)

sr=shaver (flags are hard)

Comment 18

[Chris Waterson]

Comment on attachment 51262 [details] [diff] [review]
alterna-patch, bigger but better (fp->flags, uint32; bonus fixes)

r=waterson. dee-lish.

Comment 19

[Brendan Eich [:brendan]]

Yay, fixed.

/be

Comment 20

[Steven Toth]

I just downloaded the newest talkback nightly build (20001100403) and I found 
that the bug is gone, but has changed.  If you run my original test page, you 
will find that there are now JS exceptions being generated during the for loop 
when I try to get the properties of the window.   Do I have a build with the 
newest code, or is this a new bug?

Comment 21

[Brendan Eich [:brendan]]

Steve, this bug is indeed fixed.  I see the same problem you see, however:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'JavaScript component does not have a method named:
"getHelperForLanguage"' when calling method:
[nsIClassInfo::getHelperForLanguage]"  nsresult: "0x80570030
(NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame ::
file:///tmp/foo.html :: rewriteCallback :: line 38"  data: no]
************************************************************
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'JavaScript component does not have a method named:
"getInterfaces"' when calling method: [nsIClassInfo::getInterfaces]"  nsresult:
"0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame
:: file:///tmp/foo.html :: rewriteCallback :: line 38"  data: no]
************************************************************
JavaScript error: 
 line 0: uncaught exception: Permission denied to create wrapper for object 

Please file a new bug, probably should go to jst, with jband and me cc'd.  Thanks,

/be

Comment 22

[Phil Schwartau]

Testcase added to JS testsuite: 

           mozilla/js/tests/js1_2/version120/regress-99663.js

Comment 23

[Phil Schwartau]

Note: the testcase is the one written by Brendan above.
Here is how to load it in the interactive JS shell:

[//d/JS_TRUNK/mozilla/js/src/WINNT4.0_DBG.OBJ] ./js

js> load('../../tests/js1_2/shell.js')
js> load('../../tests/js1_2/version120/regress-99663.js')

Bug Number 99663
STATUS: Regression test for Bugzilla bug 99663
Section 1 of test - got a "read-only" error PASSED!
Section 2 of test - got a "read-only" error PASSED!
Section 3 of test - got a "read-only" error PASSED!


NOTE: version(120) is automatically called by the js1_2/shell.js file.
Here's what happens if we try the test with a later version of JS:

js> version(150);
120
js> version(150);
150

js> load('../../tests/js1_2/version120/regress-99663.js')
Bug Number 99663
STATUS: Regression test for Bugzilla bug 99663
Section 1 of test - NO ERROR WAS GENERATED! FAILED! expected: a "read-only" error
Section 2 of test - NO ERROR WAS GENERATED! FAILED! expected: a "read-only" error
Section 3 of test - NO ERROR WAS GENERATED! FAILED! expected: a "read-only" error

This shows the version sensitivity Brendan mentioned above.

Also, if we try the test on an earlier codebase, for example Release
Candidate 3 of JS1.5, the testcase crashes as  Brendan states above -

Comment 24

[Phil Schwartau]

Using Mozilla debug trunk build 2001-10-08 on WinNT. I am trying Steven's HTML 
testcase above. Steven, is this behaving as expected now? In step 3, I get 
redirected to http://bugzilla.mozilla.org/showattachment.cgi? and cannot
complete the rest of the test. 

Hitting the "Back" button and trying steps 4 and 5 gives the same result.
Am I missing something? 

Comment 25

[Phil Schwartau]

OK, I guess this is due to the error reported above:

JavaScript error: 
 line 0: uncaught exception: Permission denied to create wrapper for object

Is it OK to mark this bug verified, then? I will do so. Please reopen
if anyone disagrees.

Comment 26

[Phil Schwartau]

Changing QA to self -