646490 - Step five of RegExp.prototype.exec not run for non-global, must be due to side-effects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sam Sneddon [:gsnedders]]

User-Agent:       Opera/9.80 (X11; Linux x86_64; U; en-GB) Presto/2.7.62 Version/11.01
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b13pre) Gecko/20110322 Firefox/4.0b13pre

RegExp.lastIndex has ToInteger run on it regardless of whether it will be used or not — ToInteger calls valueOf/toString if used on an object. This is never called.

Reproducible: Always

Steps to Reproduce:
var re = /./, called = 0;
re.lastIndex = {valueOf: function() { called++; return 0; }};
re.exec(".");
re.lastIndex = {toString: function() { called++; return "0"; }};
re.exec(".");
re.lastIndex = {
  valueOf: function() { called++; return 0; },
  toString: function() { called--; }
};
re.exec(".");
print(called === 3 ? "PASS" : "FAIL, got " + called);
Actual Results:  
FAIL, got 0

Expected Results:  
PASS

Comment 1

[Sam Sneddon [:gsnedders]]

Note this is a regression: Firefox 3.6 passes. (Likewise does IE8/9, and Chrome 12. This is a known bug in Opera (present as of 11).)

Comment 2

[David Mandelin [:dmandelin]]

Would you be able to bisect to a regression range using the nightly builds?

Comment 3

[Alice0775 White]

Regression window(cached m-c hourly):
Works:
http://hg.mozilla.org/mozilla-central/rev/ad33abeb9305
Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b3pre) Gecko/20100731 Minefield/4.0b3pre ID:20100731130608
Fails:
http://hg.mozilla.org/mozilla-central/rev/e9c9b7d21a0c
Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b3pre) Gecko/20100801 Minefield/4.0b3pre ID:20100801030350
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ad33abeb9305&tochange=e9c9b7d21a0c

Regression window(TM nightly):
Works:
http://hg.mozilla.org/tracemonkey/rev/cb3ed8e233b8
Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b2pre) Gecko/20100723 Minefield/4.0b2pre ID:20100723043358
Fails:
http://hg.mozilla.org/tracemonkey/rev/943de0cf6f0a
Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b2pre) Gecko/20100724 Minefield/4.0b2pre ID:20100724043348
Pushlog:
http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=cb3ed8e233b8&tochange=943de0cf6f0a

In local build on ubuntu(from TM repository)
build from 498f412bfa8f : fails
build from c421366b52a5 : works
Triggered by:
498f412bfa8f	Jeff Walden — Bug 465199 - RegExp.lastIndex setting shouldn't coerce to integer (should happen during internal use of the property instead). r=cdleary

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Patch and tests

If I ever write a patch for a standardized method, and I don't fix it by adding step-by-step comments as I've now done, r- with extreme prejudice.

Geoffrey, mind if I add your test to our test suite, which in recent years has been PD by default?

Comment 5

[Sam Sneddon [:gsnedders]]

PD? Public domain? No problem.

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 523233 [details] [diff] [review]
Patch and tests

-regexp_exec_sub(JSContext *cx, JSObject *obj, uintN argc, Value *argv, JSBool test, Value *rval)
+ExecuteRegExp(JSContext *cx, ExecType execType, uintN argc, Value *vp)

A comment that says we use the execType for RegExp execution efficiency reasons seems like it would clarify a bit, because the spec has no difference between the algorithms for the two operations.

-    if (!InstanceOf(cx, obj, &js_RegExpClass, argv))
+    /* Step 1. */
+    JSObject *obj = ToObject(cx, &vp[1]);
+    if (!obj || !InstanceOf(cx, obj, &js_RegExpClass, vp + 2))

I'm a little confused -- where does InstanceOf create a TypeError? The spec says: 'a TypeError exception is thrown if the this value is not an object or an object for which the value of the [[Class]] internal property is not "RegExp".'

+    /* Step 5. */
+    jsdouble i;
+    if (lastIndex.isInt32()) {
+        i = lastIndex.toInt32();
+    } else {
+        if (lastIndex.isDouble())
+            i = lastIndex.toDouble();
+        else if (!ValueToNumber(cx, lastIndex, &i))
+            return false;
+        i = js_DoubleToInteger(i);
+    }

Any reason not to break this out into a ECMAToInteger?

+/*
+ * 3. Let length be the length of S.
+ * 4. Let lastIndex be the result of calling the [[Get]] internal method of R with argument "lastIndex".
+ * 5. Let i be the value of ToInteger(lastIndex).
+ */
+r = /b/;
+r.lastIndex = { valueOf: {}, toString: {} };
+expectThrowTypeError(function() { r.exec("foopy"); });
+r.lastIndex = { valueOf: function() { throw new TypeError(); } };
+expectThrowTypeError(function() { r.exec("foopy"); });

It'd be nice to have a test helper that says, "execute this lambda with (a range of values, expected results from ToInteger)" to test the full range of ToInteger behavior instead of ad-hoc'ing it thusly. *shrug*

+/*
+ * 6. Let global be the result of calling the [[Get]] internal method of R with argument "global".
+ * 7. If global is false, then let i = 0.
+ */

This part is really testing step 11 and 15 IIUC. I think testing steps that aren't observable at a language level in their own sections is probably worse than globbing them together or using short descriptions of why it tests those internal steps. I'm not sure if using short descriptions is generally preferable to copying the spec into the tests verbatim, but it's something to think about.

The section reference is also wrong in ECMAv5 15.10.6.3 step 1: it wants to refer to 15.10.6.2 instead of itself.

Nice work!

Comment 7

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/0cb3a41065e1

Comment 8

[Jeff Walden [:Waldo]]

The ToInteger factoring-out got moved to bug 647385.

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/0cb3a41065e1