Closed Bug 646490 Opened 12 years ago Closed 12 years ago

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


(Core :: JavaScript Engine, defect)

2.0 Branch
Not set



Tracking Status
blocking-fx --- ?


(Reporter: gsnedders, Assigned: Waldo)



(Keywords: regression, Whiteboard: fixed-in-tracemonkey)


(1 file)

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.lastIndex = {toString: function() { called++; return "0"; }};
re.lastIndex = {
  valueOf: function() { called++; return 0; },
  toString: function() { called--; }
print(called === 3 ? "PASS" : "FAIL, got " + called);
Actual Results:  
FAIL, got 0

Expected Results:  
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).)
Would you be able to bisect to a regression range using the nightly builds?
Ever confirmed: true
Regression window(cached m-c hourly):
Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b3pre) Gecko/20100731 Minefield/4.0b3pre ID:20100731130608
Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b3pre) Gecko/20100801 Minefield/4.0b3pre ID:20100801030350

Regression window(TM nightly):
Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b2pre) Gecko/20100723 Minefield/4.0b2pre ID:20100723043358
Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b2pre) Gecko/20100724 Minefield/4.0b2pre ID:20100724043348

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
Blocks: 465199
Keywords: regression
Version: unspecified → 2.0 Branch
OS: Linux → All
Attached patch Patch and testsSplinter Review
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?
Assignee: general → jwalden+bmo
Attachment #523233 - Flags: review?(geoffers+mozilla)
Attachment #523233 - Flags: review?(cdleary)
PD? Public domain? No problem.
blocking-fx: --- → ?
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 step 1: it wants to refer to instead of itself.

Nice work!
Attachment #523233 - Flags: review?(cdleary) → review+
Attachment #523233 - Flags: review?(geoffers+mozilla)
Whiteboard: fixed-in-tracemonkey
The ToInteger factoring-out got moved to bug 647385.
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.