319391 - "eval('...') = ..." gives "invalid assignment lefthand side" as a compile-time error instead of as a runtime error

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[gwatmuff]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

This test for DOM compliance is supported by javascript 1.5. 
Shouldn't 'document.getElementById' return true for Firefox 1.5 as well? Firefox 1.5  will even return true for a test on 'document.all'

Is this a change in js/DOM/DHTML standard or a very serious bug? Hopefully the latter or a lot of recoding will have to be done! Known to affect Firefox 1.5 on Mac OS X, Windows, Linux (Red Hat 9).

Reproducible: Always

Comment 1

[Georgi D. Sotirov]

I've try this in the console:

if( document.getElementById ) alert("True"); else alert("False")

and it popups the alert box with the text True in it.

Comment 2

[Georgi D. Sotirov]

(In reply to comment #1)
> I've try this in the console:
> 
> if( document.getElementById ) alert("True"); else alert("False")
> 
> and it popups the alert box with the text True in it.
> 

Sorry, my user agent identifier is: Mozilla/5.0 (X11; U; Linux i686; bg; rv:1.8) Gecko/20051107 Firefox/1.5

Comment 3

[gwatmuff]

You are correct. My apologies for jumping to the wrong conclusion! Here is the full story:

if(document.getElementById){
	        document.getElementById(id).style.clip = 'rect(' + top + ' ' + right + ' ' + bottom + ' ' + left +')';

	}else if(document.all){

		eval('document.all.' + id + '.style.clip') = 'rect(' + top + ' ' + right + ' ' + bottom + ' ' + left + ')';

	}

The eval statement is in error. However when this code is run in firefox 1.0.1 and other mainstream browsers, the code between the first set of braces is executed and the eval line in the second set of braces is ignored (as might be expected). In firefox 1.5, the erroneous eval line in the second set of braces is detected and javascript console shows the error:

invalid assignment lefthand side
eval('document.all.' + id + '.style.clip') = 'rect(' + top + ' ' + right + ' ' + bottom + ' ' + left + ')';
------------------------------------------^

Why does Firefox 1.5 even detect this line code when the first condition: document.getElementById is true? 

When the second condition + eval line is removed altogether, leaving the first condition only intact:

if(document.getElementById){
	        document.getElementById(id).style.clip = 'rect(' + top + ' ' + right + ' ' + bottom + ' ' + left +')';

	}

then the line of code : document.getElementById(id).style.clip = 'rect(' + top + ' ' + right + ' ' + bottom + ' ' + left +')';
 is executed (as expected).

Comment 4

[Jesse Ruderman]

Attachment: testcase without document.all

This testcase demonstrates that the relevant difference between Firefox 1.0.7 and Firefox 1.5 is that in Firefox 1.0.7, "invalid assignment lefthand side" is a runtime error, while in Firefox 1.5, it is a parse- or compile-time error.

Comment 5

[Jesse Ruderman]

It's a runtime error in IE7B1 too.  The code in comment 3 would seem to imply that it was allowed in some version of IE (probably IE4).

Comment 6

[gwatmuff]

The code in comment #3 ran happily in:

Windows 98/NT/2000/XP

Mozilla 1.0 and above
Firefox 1.0 and above
Internet Explorer 6.0 and above

Macintosh OS X

Safari 1.0 and above
Firefox 1.0 and above
Internet Explorer 5.0 and above

Macintosh OS 9

Mozilla 1.0.2
Netscape 7.0
Internet Explorer 5 for OS 9 is not supported

Linux

Mozilla 1.0 and above
Firefox 1.0 and above

Don't know about IE4 or IE5 on windows.

I got around the problem encountered with Firefox 1.5 by splitting the single eval(... line of code into two steps (Not yet runtime tested on IE4 and unlikely to be!):

var docall = eval('document.all.' + id + '.style.clip');
docall = 'rect(' + top + ' ' + right + ' ' + bottom + ' ' + left + ')';

Firefox 1.5 no longer reports an error in the document.all eval statement and now executes the code in the document.getElementById statement (as might be expected).

Comment 7

[Brendan Eich [:brendan]]

Unintended breakage from development of E4X (cvs annotate blames bug 246441).  Sorry, this should be fixed quickly.  It's a violation of ECMA-262 Edition 3 Section 16 to report a compile-time error unconditionally, where the error might or might not be reported at runtime depending on control flow.

gwatmuff, thanks for finding this.  You're right, it could be a bad compatibility break, but your report is the first we've had, and the regression was checked in on 2005/10/05.

/be

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #6)

> I got around the problem encountered with Firefox 1.5 by splitting the single
> eval(... line of code into two steps (Not yet runtime tested on IE4 and
> unlikely to be!):
> 
> var docall = eval('document.all.' + id + '.style.clip');
> docall = 'rect(' + top + ' ' + right + ' ' + bottom + ' ' + left + ')';

You could also do everything in the eval:

  eval('document.all.' + id + '.style.clip = rect(' + top + ' ' + right + ' ' + bottom + ' ' + left + ')');

(I hope I got the quoting right.)

/be

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #7)
> gwatmuff, thanks for finding this.  You're right, it could be a bad
> compatibility break, but your report is the first we've had, and the regression
> was checked in on 2005/10/05.

I mistyped 2004/10/05 -- i.e., the regression was committed more than a year ago.

/be

Comment 10

[gwatmuff]

You are correct. I tried that approach earlier and got some error. Maybe I was blind to quotes! Anyway your single eval line works for me in my context too - thanks. 

Comment 11

[Brendan Eich [:brendan]]

Attachment: revert regressing change

The effect is to turn a JSOP_EVAL in a left-hand side of assignment into a JSOP_SETCALL.  The obj_eval native would still be called if the assignment was reached at runtime, and JSOP_SETCALL would throw JSMSG_BAD_LEFTSIDE_OF_ASS after obj_eval failed to set cx->rval2set.  So we'd be ECMA-compliant and backward compatible with this fix.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 205374 [details] [diff] [review]
revert regressing change

Looks good.

Comment 13

[Brendan Eich [:brendan]]

Fixed on trunk.  Still looking for shaver's review and then I'll nominate the patch for branch approval.

/be

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 205374 [details] [diff] [review]
revert regressing change

sr=shaver

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 205374 [details] [diff] [review]
revert regressing change

This is going in along with the fix for bug 318922.

/be

Comment 16

[Brendan Eich [:brendan]]

Fixed on the 1.8 and 1.8.0 branches.  Do we need a fixed1.8.1 keyword?

/be

Comment 17

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-319391.js,v  <--  regress-319391.js
initial revision: 1.1

Comment 18

[Bob Clary [:bc] (inactive)]

v 2006-01-11 1.8.0.1, 1.8.1, trunk windows/linux/mac