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

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 years ago
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
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).
Assignee

Comment 2

2 years ago
(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?
(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...
Assignee

Comment 4

2 years ago
Simply replaces HasProperty with HasOwnProperty in fun_enumerate, MappedArgumentsObject::obj_enumerate, and UnmappedArgumentsObject::obj_enumerate.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8873860 - Flags: review?(jdemooij)
Comment on attachment 8873860 [details] [diff] [review]
bug1369680.patch

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

Nice find!
Attachment #8873860 - Flags: review?(jdemooij) → review+

Comment 7

2 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a6aa43c2ff31
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.