Closed Bug 49233 Opened 24 years ago Closed 24 years ago

eval causes an error in <SCRIPT language="JavaScript1.2">

Categories

(Core :: JavaScript Engine, defect, P3)

Product:
Core
Component:
JavaScript Engine
Platform:
x86
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: kazhik, Assigned: brendan)

References

Details

(Keywords: js1.5)

Attachments

(10 files)

testcase
512 bytes, text/html
Details
proposed fix
686 bytes, patch
Details | Diff | Splinter Review
better fix (CRLFs again, uploaded from Windoze)
1.10 KB, patch
Details | Diff | Splinter Review
best fix (no CRLFs, I'm on Linux now)
1.32 KB, patch
Details | Diff | Splinter Review
latest patch revises the warning message (and sneaks in an unrelated fix to the strict warning about falling off the end of a function without return -- throw was not considered an intentional "return")
2.87 KB, patch
Details | Diff | Splinter Review
better jsparse.c patch to apprise strict warning code of exception handling statements
1.49 KB, patch
Details | Diff | Splinter Review
even better jsparse.c patch, thanks to jag for "catching" a bug in the last one
1.53 KB, patch
Details | Diff | Splinter Review
best patch (flailing hard, same logic bug jag caught afflicted try case)
1.56 KB, patch
Details | Diff | Splinter Review
i got some zzzzzzz's, couldn't stop dreaming about this
2.23 KB, patch
Details | Diff | Splinter Review
ok, i still need sleep; thanks again to jag
2.26 KB, patch
Details | Diff | Splinter Review 
Javascript does not work if a script is defined in
<SCRIPT language="JavaScript1.2">.

<SCRIPT language="JavaScript"> isn't ignored.

I'll attach a testcase.

(1) Push "javascript1.2" button. Nothing happens.
(2) Push "javascript" button. Works as it should.

Win98 2000081508
Attached file testcase — 

    
    

    
  
Previous description was wrong.

eval statement causes an error when it is in <SCRIPT language="JavaScript1.2">.

JavaScript Console says "missing ; before statement".
Summary: <SCRIPT language="JavaScript1.2"> is ignored → eval causes an error in <SCRIPT language="JavaScript1.2">

    
    

    
  
It is possible to reduce the above testcase to this:
 
                          eval('08');

This statement produces different results in JS1.2 vs. JS1.5.


EXPERIMENTS:    	

  		JavaScript 1.2	                   JavaScript 1.5		
eval('a8');   Err: a8 is not defined            Err: a8 is not defined        
eval('08');   Err: missing ; before statement   8
eval('0a8');  Err: missing ; before statement   Err: missing ; before statement


    
    

    
  
I should also note this case:

              JavaScript 1.2                     JavaScript 1.5               
eval('8');         8                                   8


METHOD
Edit and run this type of HTML; check in the JavaScript console for errors:

<HTML>
<SCRIPT language="JavaScript1.2">
      eval('8');
</SCRIPT>

<SCRIPT language="JavaScript">
      eval('8');
</SCRIPT>
</HTML>

    
    

    
  
Changing OS to "All" -

OS: Windows 98 → All

    
    

    
  
The problem is in the following part of the code:
jsparse.c: line: 178
 if ((tt == TOK_FUNCTION || lastExprType == TOK_FUNCTION) &&
            cx->version < JSVERSION_1_2) {
            /*
             * Checking against version < 1.2 and version >= 1.0
             * in the above line breaks old javascript, so we keep it
             * this way for now... XXX warning needed?
             */
            return JS_TRUE;
 }
#endif
        js_ReportCompileErrorNumber(cx, ts, JSREPORT_ERROR,
                                    JSMSG_SEMI_BEFORE_STMNT);

It seems that when you say <script= "JavaScript"> it sets the version below 1.2
and returns TRUE. while anything above 1.2 reports an error. I think it's
mandatory to end the line with ';' in Javascirpt1.2 and upwards, is it so?



    
    

    
  
cc'ing Brendan -

    
    

    
  
Mitesh: JS1.2 and later emphatically do not require semicolons between
statements or at the end of each statement.  Notice that in the reduced testcase
(usable in the js shell), eval('08') with version(120) does fail with the error
whose source report call you cite, but there are no two statements.

Notice that eval('07') with version(120) does not fail.  The bug is in the
scanner, where 08 (an invalid octal number) is processed.  Patch coming up.

/be
Assignee: rogerl → brendan
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: js1.5

    
    

    
  
So the ; was wanted because the scanner was returning two tokens, 0 and 8, with
nothing in between.  I don't think JS ever did that, and it's silly to do it for
old, non-ECMA versions (not only is it incompatible -- what I just said with "I
don't think JS ever did that", but it walks right into this token-splitting bug
that no one wants).

/be
Status: NEW → ASSIGNED

    
    

    
  

      
      
      
      

        Attached patch
          
          
        proposed fix
        — 
          — Splinter Review
      

    


  

    
    

    
  
r= and a= quick, this is a js1.5 bug.

/be

    
    

    
  
r=mccabe - looks like the sense of the JSVERSION_IS_ECMA test was reversed.  If
JSVERSION_IS_ECMA, strict conformance would require a radix of 8, not 10.  To be
practical, I don't think we want to outlaw 08 anytime soon.

    
    

    
  
patch looks fine to me.
One side question:
Then why the 'WellTerminated()' function check for the versions and allows older
JS to return true?


    
    

    
  
Mitesh: WellTerminated is trying to allow very old (Nav2, Nav3) JS to continue
to say 'function f(){} function g(){}' and not get a missing ; error.  Before
the advent of lambda expressions (functions as expressions) in JS1.2, this was a
case where JS1.0 and JS1.1 did automatic semicolon insertion.

/be

    
    

    
  
mccabe, jband: gimme fresh r=/a= love on that latest patch.

/be

    
    

    
  
The warning I moved was only for the first non-octal digit after the leading 0,
but if we're allowing 08 and 088 (and treating them as decimal, which is the
right thing), we surely should allow (with a warning) 078 and not break it into
two tokens: 07 and 8.

/be

    
    

    
  
looks right to me a=jband. Can we add test coverage for these cases?

    
    

    
  
I will add the test coverage. Is it enough to check 01, 02, ..., 09? 
Or are there other key numbers to try? 

    
    

    
  
Phil: also try 078, 079, that should do it.  Longer strings of octal digits that 
suddenly switch to decimal: 0765432198 are good to.  These should all equal the 
value without the leading 0 (the decimal literal value).

/be

    
    

    
  
Also try multipe leading zeros... 

0000078 = decimal 78, but 0000077 is octal 77 (or decimal 63).

    
    

    
  
Seems like we need to update the warning message.

js> 079
5: warning: 09 is not a legal ECMA-262 numeric constant:
5: warning: 079
5: warning: ^
79


    
    

    
  
r=mccabe.

We might want to make the warning itself (nothing else, which was the problem
before) conditional on JSVERSION_IS_ECMA to avoid warnings on 1.0/1.1/1.2-pure
pages.

    
    

    
  
mccabe: what do you think a better warning message would be?

I don't mind the warning, for all versions.  It tends to push people toward 
making their scripts (old or new) ECMA-262 pure.

/be

    
    

    
  
So, no error on '0099' and interpreting as decimal is seen as a failed test in 
our test cases as checked in now. e.g...
http://lxr.mozilla.org/mozilla/source/js/tests/ecma/LexicalConventions/7.7.3-3-n
.js (there are other similar test failures)

see the bugsplat bug reference and quoted waldemar comment from that bug report.

I don't really have a preference here. Clearly trying to parse as separate 
statements was a bad bug. We need to be sure that we want to do the "assume they 
meant decimal" thing and fix the test cases if that is the right answer.

    
    

    
  
cc'ing waldemar.

Old versions of the engine allowed 08 and 09 and treated them as decimal.  But
old versions broke 078 (e.g.) up into two tokens, resulting in a syntax error. 
This seems quirky and a mite unfair: if the user is prefixing a decimal literal
with 0, why penalize him or her only some of the time?

The 'proposed fix' patch simply removes the JSVERSION_IS_ECMA test, so you will
get a warning if you use 08 or 087, but 078 still gets split into two tokens. 
This is backward compatible, if you ignore the warning.

The 'best fix' patch is not compatible: not only does it treat 098 as 98, but it
treats 078 as 78.  It warns in all cases where 8 or 9 appear in an (initially
octal) numeric literal that starts with 0.  Is it really the best fix?  Speak
fast, please -- pdt is clamping down on branch changes.

/be

    
    

    
  
Can I get an r= and an a=?

/be

    
    

    
  
r=rogerl

    
    

    
  
a=jband

Let's be sure to get those test cases whipped into shape.

    
    

    
  
Resoliciting r/a= for the jsparse.c patch.

/be

    
    

    
  
Yay, the warning went away :-)

But, it's a bit trickier for try/catch+/finally than us sleepyheads initially
thought. If there's a finally and it returns, there's no need for try nor catch+
to return, in fact, those return statements should be ignored, methinks.
Otherwise try should return, and so should any catch blocks.

try/catch+:
try and all catches return

try/finally:
either finally or try returns

try/catch+/finally:
either finally returns, or try and all catches return

So:
case TOK_TRY:
  ok = pn->pn_kid3 ? CheckFinalReturn(pn->pn_kid3) : JS_FALSE;
  if (ok)
    return ok;
  ok = CheckFinalReturn(pn->pn_kid1);
  if (pn->pn_kid2)
    ok &= CheckFinalReturn(pn->pn_kid2);
  return ok;

What do you think, Brendan?

    
    

    
  
Regis, my final answer:

+      case TOK_TRY:
+        /* If we have a finally block that returns, we are done. */
+        if (pn->pn_kid3 && CheckFinalReturn(pn->pn_kid3))
+            return JS_TRUE;
+
+        /* Else check the try block and any and all catch statements. */
+        ok = CheckFinalReturn(pn->pn_kid1);
+        if (pn->pn_kid2)
+            ok &= CheckFinalReturn(pn->pn_kid2);
+        return ok;

One further thought: should we give a strict warning if there *are* return
statements at the end of the try and/or any catch blocks, *and* there is a
finally ending in a return?  Won't the finally's return value carry the day? 
Shaver, comments?

I also fixed this code in the next-to-last ("best", sic) patch:

+      case TOK_CATCH:
+        ok = CheckFinalReturn(pn->pn_kid3);
+        for (pn2 = pn->pn_kid2; pn2; pn2 = pn2->pn_kid2)
+            ok &= CheckFinalReturn(pn2);
+        return ok;

to avoid recursively checking the pn2->pn_kid2 list(!), defeating the attempt to
iterate.  New version passes pn2->pn_kid3 (pn2's catch-block statements) to the
CheckFinalReturn in the loop body.

jag: if you find yourself writing ?: expressions of the form:

  ok = pn->pn_kid3 ? CheckFinalReturn(pn->pn_kid3) : JS_FALSE;
  if (ok)
    ...

you are really writing an && expression:

  ok = pn->pn_kid3 && CheckFinalReturn(pn->pn_kid3);
  if (ok)
    ...

But in this particular case, I avoided storing ok and let the compiler common
the boolean true (1) value.

IOW, for two booleans A and B, A ? B : false should be simplified to A && B, in
C code.  In JS it can be simplified that way too, but the result is not a
boolean: it's B if A converted to true, else A (which must have converted to
false).  && in JS is like Perl's &&, same for || -- these operators
short-circuit control flow (as in C) but also preserve value and type of the
deciding clause.

/be

    
    

    
  
I just filed bug 56716 on SpiderMonkey's failure to execute the finally block if
the try block returns.  ECMA says that finally must run, and that it determines
the Result of the whole try-finally.  So a JSOP_RETURN must not be generated in
a try with a finally -- instead, we must "gosub".

/be

    
    

    
  
*** Bug 56727 has been marked as a duplicate of this bug. ***

    
    

    
  
brendan: d'oh!

Nice catch btw on the catch check. We really should've gotten some sleep and
dealt with this with clear heads :-) r=jag on that last patch, fwiw. How about
using bug 56727 to track the try/catch/finally and throw return check patch?

Don't forget to check in the fix to check in the octal patch, btw ;-)

    
    

    
  
Still need a= from shaver or jband on the jsparse.c patch (sorry I didn't find
the bug on file already -- I'd have been fine using that one instead of badly
overloading this one, but I'm easy either way).

/be

    
    

    
  
jag and I just reasoned out the last change in the patch, so I'm comfortable
now: sr=shaver.


    
    

    
  
Fix checked in.

/be

    
    

    
  
And I think there's zero chance of [rtm++], so closing.

/be
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED

    
    

    
  
Verified.
Status: RESOLVED → VERIFIED

    
    

    
  
The following testcases will be removed from the JS test suite:

                ecma/LexicalConventions/7.7.3-3-n.js
                ecma/LexicalConventions/7.7.3-4-n.js
                ecma_2/Exceptions/lexical-043.js
                ecma_2/Exceptions/lexical-044.js
                ecma_2/Exceptions/lexical-045.js
                ecma_2/Exceptions/lexical-046.js


They expected expressions such as 079 to produce exceptions.
The following testcase has been added in their stead:

                js1_5/LexicalConventions/lexical-001.js


This test runs through approximately twenty expressions. In light of the
work done on the current bug, the test will expect an expression like 
079 to evaluate successfully as a decimal number, whereas something like 
077 will evaluate as an octal -  etc. etc.

    
    

    
  
The six outdated testcases above have now been removed from js/tests/

    
  
Blocks: 42086

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: