191633 - Rhino tokenizer should not use recursion

Status: VERIFIED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Igor Bukanov]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.1) Gecko/20021003
Build Identifier: Rhino CVS from 2003-02-02

Currently Rhino tokenizer calls itself recursively to process single line
comments which can lead to too deep recursion when processing many lines of //
comments.

Reproducible: Always

Steps to Reproduce:
1.Run Rhino shell against the following attached JavaScript file

Actual Results:  
Exception in thread "main" java.lang.StackOverflowError
        at org.mozilla.javascript.TokenStream.getToken(TokenStream.java:753)
        at org.mozilla.javascript.TokenStream.getToken(TokenStream.java:1197)


Expected Results:  
Single true should be printed

Comment 1

[Igor Bukanov]

Attachment: Test case: calling eval on a string with 40000 lines of // comments

Comment 2

[Igor Bukanov]

Attachment: Replace tail recursion in TokenStream.getToken by a loop

The patch also removes unused TokenStream.isJSIdentifier() and adds few
optimizations not to call java.lang.Character methods for ASCII characters.

In addition patch makes sure that different occurrences of the same string
returned as the same String instance which allows to speedup the interpreter
mode as different functions from a single script use the same string instances
for equals strings which reduces the need to call String.equals in various
runtime hash tables.

Comment 3

[Igor Bukanov]

As this bug is not a regression since 1.5R3, its fixing should wait 1.5R4 release.

Comment 4

[Phil Schwartau]

Testcase added to JS testsuite:

      mozilla/js/tests/js1_5/Regress/regress-191633.js

This passes in SpiderMonkey, but fails in Rhino as Igor indicated.
Since the failure output is so enormous, I have temporarily added
the test to the rhino-n.tests skip list:

      /cvsroot/mozilla/js/tests/rhino-n.tests,v  <--  rhino-n.tests
      new revision: 1.52; previous revision: 1.51

We can remove it from the skip list once this fix is checked in -

Comment 5

[Igor Bukanov]

I commited the fix.

Comment 6

[Phil Schwartau]

Verified FIXED.

The above testcase now passes in the Rhino shell. I tested
optimization levels -1, 0, 1, and 9.

I will now remove the testcase from the rhino-n.tests skip list -

Comment 7

[Phil Schwartau]

rhino-n.tests has now been updated:

Checking in rhino-n.tests;
/cvsroot/mozilla/js/tests/rhino-n.tests,v  <--  rhino-n.tests
new revision: 1.54; previous revision: 1.53
done

Comment 8

[Igor Bukanov]

Targeting as resolved against 1.5R5