Bug 31689 – RegExp#exec's returned Array-like object behaves differently from regular Arrays

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[J Smith]

Created attachment 43524 [details]
RegExpMatchesArray::fillArrayInstance patch

RegExpMatchesArray objects returned from RegExp#exec behave differently than regular Arrays when using the "in" operator and the Array#forEach method. 

The RegExp#exec method returns an Array of captured matches from within the tested String. For optional captures, "undefined" is inserted into the Array as a missing match. The returned Array seems to be correct when you try to access the values directly using Array notation but using "in" or forEach will skip over any "undefined" values as if they aren't there.

For instance:

m = /(a)(_)?.+(c)(_)?.+/.exec("abcd");
console.log(m);
console.log(2 in m);
m.forEach(function(x) {
  console.log(x);
});

Should produce:

["abcd", "a", undefined, "c", undefined]
true
abcd
a
undefined
c
undefined

However, WebKit produces:

["abcd", "a", undefined, "c", undefined]
false
abcd
a
c

Using a regular Array similar to the one produced by RegExp#exec produces the expected result, so there is some inconsistency here.

The expected behaviour is based on what is produced by the Rhino, KJS and V8 engines. (In V8, the undefined values are inexplicably labeled "null" in their console output but checking with the strict equality operator shows that the undefined values are indeed undefined and not null. In IE8... well, the results are stranger still, as it acts in the exact opposite manner that WebKit does and produces empty Strings instead of undefineds or nulls to boot!)

A quick fix (small patch attached for the sake of convenience) appears to be to add an extra condition to JavaScriptCore/runtime/RegExpConstructor.cpp in RegExpMatchesArray::fillArrayInstance at line 135 to make sure that those undefined values are inserted properly:

131     for (unsigned i = 0; i <= lastNumSubpatterns; ++i) {
132         int start = d->lastOvector()[2 * i];
133         if (start >= 0)
134             JSArray::put(exec, i, jsSubstring(exec, d->lastInput, start, d->lastOvector()[2 * i + 1] - start));
135         else
136             JSArray::put(exec, i, jsUndefined());
137     }

[Alexey Proskuryakov]

Thank you for the bug report, and for the proposed fix.

Would you be willing to submit a patch for review, as described in <http://webkit.org/coding/contributing.html>?

[J Smith]

Not a problem. Should any new layout tests be added for the fix? This is my first WebKit patch so I'm not entirely sure of the procedure...

[Alexey Proskuryakov]

Good question! Yes, a layout test needs to be added to LayoutTests/fast/js. To make one, add a test script to LayoutTests/fast/js/script-tests, and then use make-script-test-wrappers script to generate an HTML wrapper. Expected results are generated by run-webkit-tests itself.

Looks like a link in <http://webkit.org/coding/contributing.html> promises to explain this, but <http://webkit.org/quality/testwriting.html> fails to do that.

[J Smith]

Okay, I've created a patch, tests and such included. Hopefully everything is correct and in the right places. Just let me know how it looks and if everything is okay...

[J Smith]

Created attachment 43769 [details]
Patch for RegExpMatchesArray

[Eric Seidel (no email)]

http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree explains more about test writing, but it has long been on my list of TODOs to enhance both that page and the webkit.org/quality/testwriting.html page.

[Eric Seidel (no email)]

Ideally JavaScriptCore/runtime/RegExpConstructor.cpp would be updated with your Copyright line.  And the ChangeLog would include your full name (unless your first name is actually 'J').  Overall the patch seems good to me.  I'm not sure I understand the test case fully, but I would have to stare at it more.

Technically I think that the test case violates the webkit style guidlines ({ on a new line after a function name), although we aren't very clear about our JavaScript styling.  Thank you very much for the patch!

[J Smith]

Is there anything like the "WebKit Group" or something the copyright could be assigned to? I have no problems assigning copyright over for a two-liner, especially if it makes licensing easier...

The test case itself just ensures that the RegExpMatchesArrays that are created by RegExp#exec insert the "undefined" entries where necessary, whether they're at the beginning of a match Array, in the middle or the end. It also tests the "in" operator and forEach function to make sure that the RegExpMatchesArray is behaving in the same manner as a plain old JavaScript Array. In the end, the three test functions should test the equivalence of Arrays to RegExpMatchesArrays with regards to the "in" operator and the forEach function.

I just noticed that I didn't include my forEach function tests, so I should add those as well.

I'll upload a new patch with the fixes for formatting and my missing forEach test. For the copyright statement, I'll add it, but I feel rather sheepish for adding a copyright statement for a two-liner. Again, if there's any group that accepts copyright assignment on WebKit's behalf, I can certainly assign copyright over if it's easier for licensing...

(On a somewhat related note with regards to DumpRenderTree, I'm having problems building that tool and am getting "make: *** No rule to make target .../Debug/DumpRenderTree" errors. I've only been able to run tests in LayoutTests manually. Any hints for a WebKit newbie?)

[J Smith]

Created attachment 43795 [details]
Patch for RegExpMatchesArray with corrections

[Eric Seidel (no email)]

(In reply to comment #8)
> Is there anything like the "WebKit Group" or something the copyright could be
> assigned to? I have no problems assigning copyright over for a two-liner,
> especially if it makes licensing easier...

No, for better or worse, there is no "WebKit Group".  Most folks just list themselves, or their employer (if they have an explicit copyright assignment agreement with their employer).

> I'll upload a new patch with the fixes for formatting and my missing forEach
> test. For the copyright statement, I'll add it, but I feel rather sheepish for
> adding a copyright statement for a two-liner. Again, if there's any group that
> accepts copyright assignment on WebKit's behalf, I can certainly assign
> copyright over if it's easier for licensing...

Thank you.

> (On a somewhat related note with regards to DumpRenderTree, I'm having problems
> building that tool and am getting "make: *** No rule to make target
> .../Debug/DumpRenderTree" errors. I've only been able to run tests in
> LayoutTests manually. Any hints for a WebKit newbie?)

Which port are you building?  If you're using make instead of build-webkit and build-dumprendertree I assume that you're using the gtk port.

run-webkit-tests calls build-dumprendertree on most ports and correctly builds DumpRenderTree if needed.  Not all ports have correctly wired up the scripts though, and it's possible you're using one of the ports which is missing this functionality.

[Eric Seidel (no email)]

Comment on attachment 43795 [details]
Patch for RegExpMatchesArray with corrections

This looks right to me.  Thanks again for the patch!

[Oliver Hunt]

(In reply to comment #11)
> (From update of attachment 43795 [details])
> This looks right to me.  Thanks again for the patch!

Still no expected results :-/

[WebKit Commit Bot]

Comment on attachment 43795 [details]
Patch for RegExpMatchesArray with corrections

Rejecting patch 43795 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit']" exit_code: 1
Last 500 characters of output:
ojects/build/WebCore.build/Release/WebCore.build/Objects-normal/i386/RenderLayerBacking.o /Users/eseidel/Projects/CommitQueue/WebCore/rendering/RenderLayerBacking.cpp normal i386 c++ com.apple.compilers.gcc.4_2
	Distributed-CompileC /Users/eseidel/Projects/build/WebCore.build/Release/WebCore.build/Objects-normal/i386/JSWebGLRenderingContextCustom.o /Users/eseidel/Projects/CommitQueue/WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(14 failures)

[Eric Seidel (no email)]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 43795 [details] [details])
> > This looks right to me.  Thanks again for the patch!
> 
> Still no expected results :-/

Oops.  My bad.  Looks like the commit-queue also found a compile failure.  Sadly the commit-queue doesn't know how to upload the entire compile results yet.  :(  See bug 28286.

[Eric Seidel (no email)]

Actually, looks like the compile failure was bogus.  I'll leave the patch cq- though due to the missing results.  The false rejection was caused by bug 30098 (which I have a fix for, I just haven't landed it yet).

[J Smith]

Ah, yeah, I guess those would help. I'll add those to the patch and re-up as soon as my local build is done. I think my problem with DumpRenderTree may be because I'm building JavaScriptCore in Xcode but running build-dumprendertree and run-webkit-tests from a terminal. Probably shouldn't be mixing those I guess. (My build environment is OSX 10.6.2 on an MBP, fwiw...)

[Oliver Hunt]

(In reply to comment #16)
> Ah, yeah, I guess those would help. I'll add those to the patch and re-up as
> soon as my local build is done. I think my problem with DumpRenderTree may be
> because I'm building JavaScriptCore in Xcode but running build-dumprendertree
> and run-webkit-tests from a terminal. Probably shouldn't be mixing those I
> guess. (My build environment is OSX 10.6.2 on an MBP, fwiw...)

build-dumprendertree is called by run-webkit-tests.  The issue is likely that you need to do build-webkit so that the entire stack is built.

--Oliver

[J Smith]

Created attachment 43804 [details]
Patch for RegExpMatchesArray with expected test results

Okay, here's the updated patch with the expected test results.

Since this attachment upload page says "Please also add your copyright (name and year) to the relevant files for changes that are more than 10 lines of code", I guess I can forgo the copyright statement as the patch is pretty trivial.

Hopefully everything's in place now...

[Eric Seidel (no email)]

Comment on attachment 43804 [details]
Patch for RegExpMatchesArray with expected test results

The ChangeLog is doesn't mention the -expected.txt file, but that's OK. :)  Looks good, I'll add it to the commit-queue again for automated landing.

Thanks, and congrats on your first WebKit patch!

[WebKit Commit Bot]

Comment on attachment 43804 [details]
Patch for RegExpMatchesArray with expected test results

Rejecting patch 43804 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	ChangeLog
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/runtime/RegExpConstructor.cpp
	A	LayoutTests/fast/js/regexp-in-and-foreach-handling-expected.txt
	A	LayoutTests/fast/js/regexp-in-and-foreach-handling.html
	A	LayoutTests/fast/js/script-tests/regexp-in-and-foreach-handling.js

[Eric Seidel (no email)]

Comment on attachment 43804 [details]
Patch for RegExpMatchesArray with expected test results

I think that rejection was bogus.  Looks like bug 31734.

[WebKit Commit Bot]

Comment on attachment 43804 [details]
Patch for RegExpMatchesArray with expected test results

Rejecting patch 43804 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	ChangeLog
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/runtime/RegExpConstructor.cpp
	A	LayoutTests/fast/js/regexp-in-and-foreach-handling-expected.txt
	A	LayoutTests/fast/js/regexp-in-and-foreach-handling.html
	A	LayoutTests/fast/js/script-tests/regexp-in-and-foreach-handling.js

[Eric Seidel (no email)]

A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:

    The following files contain tab characters:

        trunk/LayoutTests/fast/js/script-tests/regexp-in-and-foreach-handling.js

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/libexec/git-core//git-svn line 469

Ahha!  The fact that that comment is missing from teh bug  is a recent bugzilla-tool regression caused by zoltan's change.  I'll update bug 31734

[Eric Seidel (no email)]

The end result is that one of your files has a tab in it.  Due to bug 31734, the correct reason for the failure was not correctly posted in the bug.  In order to get this landed, either someone will have to land it by hand (removing the tab), or you'll have to upload a new patch w/o a tab in it.  I'm sorry that our current tooling does not warn you about stray tabs earlier. :(  We really need to make our tools more friendly to new contributers!

[J Smith]

Created attachment 43821 [details]
Patch for RegExpMatchesArray with corrected tabs

Okay, deep breath here, J.

Expected results for tests -- check.
ChangeLog completed -- check.
No tabs for indentation -- check.
Fingers crossed -- check.

Here goes nothin'...

[WebKit Commit Bot]

Comment on attachment 43821 [details]
Patch for RegExpMatchesArray with corrected tabs

Clearing flags on attachment: 43821

Committed r51369: <http://trac.webkit.org/changeset/51369>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Yay!  Congrats again on your first change to webkit.  Sorry our tools gave you trouble!