Tooru Fujisawa [:arai] Assignee
	Description • 9 years ago

Separated from bug 1054755.

RegExp constructor in ES6 uses IsRegExp function, which will be introduced in bug 1054755.
Also, there are some differences related to patternIsRegExp.

	Tooru Fujisawa [:arai] Assignee
	Comment 1 • 9 years ago

Prepared 3 patches.

Part 1 factors out RegExpInitialize from CompileRegExpObject.
Also, regexp_construct_no_statics calls RegExpInitialize directly since it doesn't need remaining logic in CompileRegExpObject.


Then, I'm not sure whether Part 2 is the correct way.
In step 4.b.iii, spec requires comparing newTarget with pattern.constructor.
> If SameValue(newTarget, patternConstructor) is true, return pattern.

And following test fails if I use js::SameValue for patternConstructor and callee, because `r` is from different global.
https://dxr.mozilla.org/mozilla-central/source/js/src/jit-test/tests/basic/testCrossCompartmentTransparency.js#79
> test("new RegExp('1')", function(r) assertEq(r, RegExp(r)));

So, I added Proxy::fun_isNativeFunction and BaseProxyHandler::fun_isNativeFunction to compare the patternConstructor directly with js::regexp_construct.
If there is better way to compare values from different globals, please tell me :)


Part 3 adds step 4 and 6 to RegExp constructor.
In step 4.b.iii, I used IsNativeFunction instead of SameValue if patternConstructor is not a proxy, to make it consistent with proxy's case.

	Tooru Fujisawa [:arai] Assignee
	Comment 4 • 9 years ago

Hm, if "global" corresponds to "Realm" in ES6 spec, RegExp functions in two Realms are different function object, thus `SameValue(newTarget, patternConstructor)` in Step 4.b.iii returns false if `patternConstructor` comes from different global than active function object, right?
so, should following assertion be fail now?
  var r = newGlobal().eval("/a/"); assertEq(RegExp(r), r)

If it's true, now we can use js::SameValue directly.

	Tooru Fujisawa [:arai] Assignee
	Comment 5 • 9 years ago

in case comment #4 is correct, replacing previous Part 2 and Part 3.

	Tooru Fujisawa [:arai] Assignee
	Comment 6 • 9 years ago

try run for attachment 8588643 [details] [diff] [review]
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=75406c823d59

	Tooru Fujisawa [:arai] Assignee
	Comment 7 • 9 years ago

Changed Step 4.b.iii to use operator==

	Till Schneidereit [:till]
	Comment 8 • 9 years ago

Comment on attachment 8588096 [details] [diff] [review]
Part 1: Add RegExpInitialize.

Review of attachment 8588096 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, r=me.

::: js/src/builtin/RegExp.cpp
@@ +167,5 @@
> +        if (!ParseRegExpFlags(cx, flagStr, &flags))
> +            return false;
> +    }
> +
> +    /* Step 9. */

Nit: Steps 9-10.

	Till Schneidereit [:till]
	Comment 9 • 9 years ago

Comment on attachment 8590406 [details] [diff] [review]
Part 2: Use IsRegExp in RegExp constructor.

Review of attachment 8590406 [details] [diff] [review]:
-----------------------------------------------------------------

Beautiful. r=me with feedback addressed.

::: js/src/builtin/RegExp.cpp
@@ +191,5 @@
>      return true;
>  }
>  
>  /*
> + * ES6 draft rc4 21.2.5.1 steps 5-10.

21.2.3.1 here and everywhere else.

::: js/src/tests/ecma_6/RegExp/constructor-IsRegExp.js
@@ +1,4 @@
> +var BUGNUMBER = 0;
> +var summary = "RegExp constructor with pattern with @@match.";
> +
> +print(BUGNUMBER + ": " + summary);

Either just remove this, or remove the BUGNUMBER part, or add 1147817. Whatever is fine, but printing "0: [description" is confusing.

::: js/src/tests/ecma_6/RegExp/constructor-constructor.js
@@ +1,4 @@
> +var BUGNUMBER = 0;
> +var summary = "RegExp constructor should check pattern.constructor.";
> +
> +print(BUGNUMBER + ": " + summary);

Same here.

	Tooru Fujisawa [:arai] Assignee
	Comment 10 • 9 years ago

Thank you for reviewing!
Fixed step and bug numbers.

https://hg.mozilla.org/integration/mozilla-inbound/rev/224db47e1e20
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad1cd598bb35

	Carsten Book [:Tomcat]
	Comment 11 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/224db47e1e20
https://hg.mozilla.org/mozilla-central/rev/ad1cd598bb35

	Tooru Fujisawa [:arai] Assignee
	Comment 12 • 9 years ago

Updated following document:
  https://developer.mozilla.org/en-US/Firefox/Releases/40