As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 392378 - Javascript Regular Expression Non-participating Capture Groups are inaccurate in edge cases.
: Javascript Regular Expression Non-participating Capture Groups are inaccurate...
Status: RESOLVED DUPLICATE of bug 369778
DUPEME
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
http://blog.stevenlevithan.com/archiv...
: 389717 435990 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-08-15 13:37 PDT by Kris Kowal
Modified: 2014-07-06 18:55 PDT (History)
11 users (show)
shaver: wanted‑next+
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.58 KB, patch)
2008-04-05 19:51 PDT, x0
crowderbt: review+
Details | Diff | Splinter Review
String.prototype.replace testcase for tests/ecma_3/String/15.5.4.11.js (10.08 KB, application/x-javascript)
2008-04-09 20:26 PDT, x0
no flags Details
3 more tests (10.36 KB, application/x-javascript)
2008-04-09 23:39 PDT, x0
no flags Details

Description User image Kris Kowal 2007-08-15 13:37:43 PDT
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en) AppleWebKit/522.11 (KHTML, like Gecko) Version/3.0.2 Safari/522.12
Build Identifier: Firefox 2.0.0.4

Javascript Regular Expression Non-participating Capture Groups are inaccurate in edge cases.  Steven Levithan wrote up a cross-browser test matrix comparing browser behaviors for edge case regular expressions to his interpretation of the ECMA specified behavior.  For details see his blog post:
http://blog.stevenlevithan.com/archives/npcg-javascript

Steve Levithan notes, in particular, that these cases fail for Firefox:

Input: "y".split(/(x)?\1y/)
Oracle:  ["", undefined, ""]
Actual: ["", "", ""]

Input: "y".split(/(x)?y/)
Oracle:  ["", undefined, ""]
Actual: ["", "", ""]

Input: "y".replace(/(x)?\1y/, function($0, $1){ return String($1); })
Oracle: "undefined"
Actual: ""

Input: "y".replace(/(x)?y/, function($0, $1){ return String($1); })
Oracle: "undefined"
Actual: ""

Input: "y".replace(/(x)?y/, function($0, $1){ return $1; })
Oracle: "undefined"
Actual: ""



Reproducible: Always
Comment 1 User image Brendan Eich [:brendan] 2007-08-15 13:46:09 PDT
Totally sounds like a known bug crowder and mrbkap are tracking, for sure ;-).

/be
Comment 2 User image x0 2008-04-05 19:48:28 PDT
*** Bug 389717 has been marked as a duplicate of this bug. ***
Comment 3 User image x0 2008-04-05 19:51:45 PDT
Created attachment 313880 [details] [diff] [review]
patch

Actually no RegExp bug, but bugs in String.prototype.replace and String.prototype.split.

Patch passes the test suite.
Comment 4 User image Brian Crowder 2008-04-05 20:01:09 PDT
MochiTests on this would be nice add'l info.  I'm going to steal the review from Brendan on this, though, in the hopes that he won't mind.  :)
Comment 5 User image Brian Crowder 2008-04-05 20:07:45 PDT
Comment on attachment 313880 [details] [diff] [review]
patch

Looks good to me, thanks.  Will land conditional upon a1.9+ and mochitest results.  If you haven't got time to run the mochitests, please let me know.
Comment 6 User image x0 2008-04-05 20:36:20 PDT
I don't know how to run the mochitests yet.
Comment 7 User image Jeff Walden [:Waldo] (remove +bmo to email) 2008-04-05 21:08:17 PDT
Assuming you have a Mozilla tree and Firefox built, just run:

python $objdir/_tests/testing/mochitest/runtests.py --autorun

...where $objdir is the value for MOZ_OBJDIR, if you set it, and otherwise is /probably/ just your source directory (but I'm not entirely certain, to be honest).
Comment 8 User image x0 2008-04-07 06:15:04 PDT
Thanks, Jeff, that's easy and also doesn't take too long (20 minutes). Mochitests passed.

There are (at least) 2 more instances in the code where an empty string for non-matched parens is returned: The $n replacements in String.prototype.replace and the global RegExp.$n properties.

In String.prototype.replace that behavior is accordingly to the spec, and the global RegExp properties are non-standard. Konqueror and Opera have also an empty string there (don't know about IE; Konqueror doesn't have the properties until the first use of a RegExp).

BTW, RegExp.$n goes only up to RegExp.$9. Opera has also only the first 9 paren matches as global properties, but Konqueror even has RegExp["$1.8e19"] (limit is slightly less than 2^64).
Comment 9 User image Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-04-07 11:42:37 PDT
Wanted for the next time -- the day before RC1 freeze isn't the time to be taking these non-blocker non-regression wins, delicious though they look.
Comment 10 User image Brian Crowder 2008-04-07 11:43:06 PDT
Comment on attachment 313880 [details] [diff] [review]
patch

Per discussion with shaver, deferring until post-1.9 (like, 1.9.1)
Comment 11 User image Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-04-07 11:43:31 PDT
Comment on attachment 313880 [details] [diff] [review]
patch

a-: non-regression non-blocker to repair edge case.  dot next will enjoy this much more.
Comment 12 User image Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-04-07 11:45:59 PDT
I should also add that it would be great to get that edge-case test set into a suite somewhere.
Comment 13 User image Brian Crowder 2008-04-09 17:02:31 PDT
bclary:  Would be great to get this known-failure into js/tests, if you can lend a hand.
Comment 14 User image Bob Clary [:bc:] 2008-04-09 17:43:33 PDT
crowder, Sure. I have a major update to go in tonight. I'll do this one after that lands.
Comment 15 User image x0 2008-04-09 20:26:57 PDT
Created attachment 314767 [details]
String.prototype.replace testcase for tests/ecma_3/String/15.5.4.11.js

This test fails also for bug 427733 and bug 427823, which I found while writing it. It doesn't test the split case.

tests/ecma_2/String/replace-001.js seems to be obsolete; String.prototype.replace wasn't in ECMAScript 2 final.

Code like this isn't exactly an edge case:

  str.replace(/(\d+)|Infinity|NaN/g,
              function (x, p) { return p != null ? +p + 1 : "???" });

But you can easily work around it by writing

  str.replace(/(\d+)|Infinity|NaN/g,
              function (x, p) { return p ? +p + 1 : "???" });

instead (that's what I do anyways), and most cases where you would need to compare explicitly against null like

  str.replace(/\s+($)?/g, function (x, p) { return p == null ? " " : "" });

don't work per spec (they do in Perl).
Comment 16 User image x0 2008-04-09 23:39:16 PDT
Created attachment 314788 [details]
3 more tests
Comment 17 User image Brian Crowder 2008-04-10 06:00:59 PDT
x00000000:  Thanks for the tests here!
Comment 18 User image Bob Clary [:bc:] 2008-04-16 09:32:06 PDT
/cvsroot/mozilla/js/tests/ecma_3/String/regress-392378.js,v  <--  regress-392378.js
initial revision: 1.1

Checking in ecma_3/String/15.5.4.11.js;
/cvsroot/mozilla/js/tests/ecma_3/String/15.5.4.11.js,v  <--  15.5.4.11.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/public-failures.txt,v  <--  public-failures.txt
new revision: 1.65; previous revision: 1.64

Comment 19 User image Bob Clary [:bc:] 2008-05-27 20:10:47 PDT
*** Bug 435990 has been marked as a duplicate of this bug. ***
Comment 20 User image Bob Clary [:bc:] 2009-06-02 08:24:01 PDT
back into the pool.
Comment 21 User image Till Schneidereit [till] 2014-05-25 14:40:45 PDT
Marking as duplicate of bug 369778. That one doesn't have a patch, but it is very slightly newer and has current-era discussion going on.

*** This bug has been marked as a duplicate of bug 369778 ***

Note You need to log in before you can comment on or make changes to this bug.