220367 - NPE when accessing RegExp.$1 after matching /(a)|(b)/ against "b"

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Igor Bukanov]

The following script gives NullPointerException:

var regEx = /(a)|(b)/;
var t = regEx.test( 'b');
print( "RegExp.$1: '"+RegExp.$1+"'");

The script should print instead:

RegExp.$1: ''

In general NPE happens whenever after successful match one accesses a captured
group that was not captured as one of the branches of |.

Comment 1

[Igor Bukanov]

Attachment: Fix: if in RegExpImpl.getParenSubString parens.get(i) is null, return emptySubstring

If a branch of | with capturing () does not match , then RegExpImpl.parens will
contain null for corresponding $<group-number> according to code in
NativeRegExp.executeRegExp(). The current code does not check for a possible
null in RegExpImpl.parens when calling toString and the patch fixes that by
making sure that getParenSubString return SubString.emptySubString in such
cases.

Comment 2

[Phil Schwartau]

Testcases added to JS testsuite:

  mozilla/js/tests/ecma_3/RegExp/regress-220367-001.js
  mozilla/js/tests/ecma_3/RegExp/regress-220367-002.js

The first one checks only the match array |str.match(regEx)|;
the second one checks the global properties |RegExp.$1|, |RegExp.$2|.
As Igor reported, the second one crashes in the current Rhino shell.

Comment 3

[Igor Bukanov]

I committed the fix

Comment 4

[Phil Schwartau]

With the fix, no longer crash on js/tests/ecma_3/RegExp/regress-220367-002.js.
However, we're getting a discrepancy in Section 3 of the test:


         Testcase ecma_3/RegExp/regress-220367-002.js failed:
         Failure messages were: 
         Section 3 of test -
         Expected value '', Actual value 'a'


For convenience, here is the body of the test:

var re = /(a)|(b)/;

re.test('a');
  status = inSection(1);
  actual = RegExp.$1;
  expect = 'a';
  addThis();

  status = inSection(2);
  actual = RegExp.$2;
  expect = '';
  addThis();

re.test('b');
  status = inSection(3);
  actual = RegExp.$1;
  expect = '';
  addThis();

  status = inSection(4);
  actual = RegExp.$2;
  expect = 'b';
  addThis();



For some reason, Rhino is not clearing out the value of RegExp.$1
It continues to hold the value 'a' from the previous re.test(). 

This differs from both SpiderMonkey and IE6, as can be seen via
the following javascript: URL

javascript: var msg = ''; var re = /(a)|(b)/; re.test('a'); msg += '$1= ' +
RegExp.$1 + '\n' +  '$2= ' +  RegExp.$2 + '\n\n\n'; re.test('b'); msg +=  '$1= '
+ RegExp.$1 + '\n' +  '$2= ' + RegExp.$2; alert(msg);

Comment 5

[Phil Schwartau]

Still getting the test failure mentioned in Comment #4 -

Comment 6

[Igor Bukanov]

reopening after wrong click on fixed button

Comment 7

[Igor Bukanov]

Attachment: Fix: use Java array to store parenthesis results

In the patch I replaced parens array in RegExpImpl from ObjArray to SubString[]
since reuse of parens that ObjArray provides does not justify complexity of
code to clean up it. The problem with old version was missed reset of initial
elements before calling ObjArray.setSize which was responsible for keeping the
old elements.

Comment 8

[Igor Bukanov]

Fixing for real

Comment 9

[Igor Bukanov]

Trageting as resolved against 1.5R5