Modify Object.{freeze,seal,isFrozen,isSealed} to avoid the problematic "caller" method on Functions. (issue 11312247)

[ad...@chromium.org]

Reviewers: rossberg,

Message:
This approach is pretty hacky, but other approaches I tried were worse. We
can't
simply create a %GetOwnPropertyAttributes method, since we need (for freeze
and
seal) to actually get ahold of the current value. That's how I settled on
this
minimal approach.

Description:
Modify Object.{freeze,seal,isFrozen,isSealed} to avoid the
problematic "caller"
method on Functions.

Given that we know the property is always non-writable and non-configurable,
there's no need to ever check it or redefine it in these methods.

BUG=v8:2407


Please review this at https://codereview.chromium.org/11312247/

SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/v8natives.js
A + test/mjsunit/regress/regress-2407.js


Index: src/v8natives.js
diff --git a/src/v8natives.js b/src/v8natives.js
index
20fc74dc440dcf28dd64ece1de30133611aadf0b..7c8381812bb86158ad554bd6e774f42cab974eb5
100644
--- a/src/v8natives.js
+++ b/src/v8natives.js
@@ -1154,6 +1154,17 @@ function ProxyFix(obj) {
ObjectDefineProperties(obj, props);
}

+function GetOwnPropertyNamesForSealOrFreeze(obj) {
+ var names = ObjectGetOwnPropertyNames(obj);
+ if (IS_SPEC_FUNCTION(obj)) {
+ // The "caller" property on Functions is always non-writable and
+ // non-configurable, so there's no need to check it or redefine it.
+ // Thus, we skip it to avoid the strict-mode check the getter enforces.
+ names = names.filter(function(name) { return name != "caller"; });
+ }
+ return names;
+}
+

// ES5 section 15.2.3.8.
function ObjectSeal(obj) {
@@ -1163,7 +1174,7 @@ function ObjectSeal(obj) {
if (%IsJSProxy(obj)) {
ProxyFix(obj);
}
- var names = ObjectGetOwnPropertyNames(obj);
+ var names = GetOwnPropertyNamesForSealOrFreeze(obj);
for (var i = 0; i < names.length; i++) {
var name = names[i];
var desc = GetOwnProperty(obj, name);
@@ -1185,7 +1196,7 @@ function ObjectFreeze(obj) {
if (%IsJSProxy(obj)) {
ProxyFix(obj);
}
- var names = ObjectGetOwnPropertyNames(obj);
+ var names = GetOwnPropertyNamesForSealOrFreeze(obj);
for (var i = 0; i < names.length; i++) {
var name = names[i];
var desc = GetOwnProperty(obj, name);
@@ -1221,7 +1232,7 @@ function ObjectIsSealed(obj) {
if (%IsJSProxy(obj)) {
return false;
}
- var names = ObjectGetOwnPropertyNames(obj);
+ var names = GetOwnPropertyNamesForSealOrFreeze(obj);
for (var i = 0; i < names.length; i++) {
var name = names[i];
var desc = GetOwnProperty(obj, name);
@@ -1242,7 +1253,7 @@ function ObjectIsFrozen(obj) {
if (%IsJSProxy(obj)) {
return false;
}
- var names = ObjectGetOwnPropertyNames(obj);
+ var names = GetOwnPropertyNamesForSealOrFreeze(obj);
for (var i = 0; i < names.length; i++) {
var name = names[i];
var desc = GetOwnProperty(obj, name);
Index: test/mjsunit/regress/regress-2407.js
diff --git a/test/mjsunit/regress/regress-crbug-135066.js
b/test/mjsunit/regress/regress-2407.js
similarity index 77%
copy from test/mjsunit/regress/regress-crbug-135066.js
copy to test/mjsunit/regress/regress-2407.js
index
1aeca8b1a32d678ba7274c60230a77fdda97f6aa..eba5ee0f1d84cd5af06e473789731e5b80c40fa7
100644
--- a/test/mjsunit/regress/regress-crbug-135066.js
+++ b/test/mjsunit/regress/regress-2407.js
@@ -25,29 +25,25 @@
// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

-// Filler long enough to trigger lazy parsing.
-var filler = "//" + new Array(1024).join('x');
+// Regression test for http://code.google.com/p/v8/issues/detail?id=2407.
+//
+// Test that Object.{seal,isSealed,freeze,isFrozen} don't throw when called
+// on non-strict Function objects with a strict "caller" property.

-// Test strict eval in global context.
-eval(
- "'use strict';" +
- "var x = 23;" +
- "var f = function bozo1() {" +
- " return x;" +
- "};" +
- "assertSame(23, f());" +
- filler
-);
+function nonStrict(fun) {
+ fun(nonStrict);
+}

-// Test default eval in strict context.
-(function() {
+function strict(fun) {
"use strict";
- eval(
- "var y = 42;" +
- "var g = function bozo2() {" +
- " return y;" +
- "};" +
- "assertSame(42, g());" +
- filler
- );
-})();
+ nonStrict(fun);
+}
+
+var problematicFunctions = [
+ Object.isSealed,
+ Object.isFrozen,
+ Object.seal,
+ Object.freeze,
+];
+
+assertDoesNotThrow(problematicFunctions.forEach(strict));

[ross...@chromium.org]

Hm, I'm not sure that this hack is the right way to approach the problem. It
actually points at a deeper issue with caller poisoning and our
implementation
of it, which attempts to fix a hole in the spec (cf.
https://bugs.ecmascript.org/show_bug.cgi?id=310). I sent a mail to
es-discuss in
the hope of finding a resolution.

For the time being, I'd suggest simply commenting out the check in
Object.observe, which is not crucial anyway.


https://codereview.chromium.org/11312247/diff/1/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/11312247/diff/1/src/v8natives.js#newcode1159
src/v8natives.js:1159: if (IS_SPEC_FUNCTION(obj)) {
IS_FUNCTION probably is more adequate here, since it shouldn't be a
proxy.

https://codereview.chromium.org/11312247/

[a...@chromium.org]

https://codereview.chromium.org/11312247/diff/1/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/11312247/diff/1/src/v8natives.js#newcode1159
src/v8natives.js:1159: if (IS_SPEC_FUNCTION(obj)) {

On 2012/11/16 12:28:55, rossberg wrote:
> IS_FUNCTION probably is more adequate here, since it shouldn't be a
proxy.

There are more poison pills than functionObj.caller

function.arguments
arguments.caller
arguments.callee

https://codereview.chromium.org/11312247/

[ross...@chromium.org]

https://codereview.chromium.org/11312247/diff/1/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/11312247/diff/1/src/v8natives.js#newcode1159
src/v8natives.js:1159: if (IS_SPEC_FUNCTION(obj)) {

On 2012/11/16 14:38:06, arv wrote:
> On 2012/11/16 12:28:55, rossberg wrote:
> > IS_FUNCTION probably is more adequate here, since it shouldn't be a
proxy.

> There are more poison pills than functionObj.caller

> function.arguments
> arguments.caller
> arguments.callee

Yes, but this issue affects only "caller", because it is due to its
special casing in 15.3.5.4.

https://codereview.chromium.org/11312247/

[a...@chromium.org]

Thanks. Very clear.

Adam, maybe add a comment to see 15.3.5.4?

https://codereview.chromium.org/11312247/