1369680 - Functions and arguments shouldn't use HasProperty to trigger reflection of lazy properties

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

Test case:
function enumerateArgs() {
    var a = arguments;
    Object.setPrototypeOf(a, new Proxy(Object.prototype, {
        has(t, pk) {
            print("Has", String(pk));
            return Reflect.has(t, pk)
        }
    }));
    delete a.length;
    for (var k in a);
}
enumerateArgs();


function enumerateFunction() {
    function f() {}
    Object.setPrototypeOf(f, new Proxy(Object.prototype, {
        has(t, pk) {
            print("Has", String(pk));
            return Reflect.has(t, pk)
        }
    }));
    delete f.length;
    for (var k in f);
}
enumerateFunction();
---


Expected: "Has length" is not printed
Actual: "Has length" is printed for both test functions

Comment 1

[Jan de Mooij [:jandem]]

While looking into bug 1364816 I noticed the browser does something similar for the global object (calling JS_HasUCProperty). It may be nice to overhaul all this stuff at the same time (change the enumerate hook to just return the lazy properties or something).

Comment 2

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> While looking into bug 1364816 I noticed the browser does something similar
> for the global object (calling JS_HasUCProperty). It may be nice to overhaul
> all this stuff at the same time (change the enumerate hook to just return
> the lazy properties or something).

I was thinking about just replacing HasProperty with HasOwnProperty for the function/arguments case. Is it possible for the browser components to use JSNewEnumerateOp?

Comment 3

[Jan de Mooij [:jandem]]

(In reply to André Bargull from comment #2)
> I was thinking about just replacing HasProperty with HasOwnProperty for the
> function/arguments case.

Ah yes that makes sense.

> Is it possible for the browser components to use JSNewEnumerateOp?

Yeah I hope we can make that work somehow...

Comment 4

[André Bargull [:anba]]

Attachment: bug1369680.patch

Simply replaces HasProperty with HasOwnProperty in fun_enumerate, MappedArgumentsObject::obj_enumerate, and UnmappedArgumentsObject::obj_enumerate.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8873860 [details] [diff] [review]
bug1369680.patch

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

Nice find!

Comment 6

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c2f4711c9d7c5e898216a8a1a893dded9170aa5

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6aa43c2ff31
Use HasOwnProperty when resolving lazy properties to avoid triggering proxy traps in the proto-chain. r=jandem

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a6aa43c2ff31