Closed
Bug 1257102
Opened 8 years ago
Closed 8 years ago
Scripted proxy code should use GetMethod, not GetProperty, to get proxy traps
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: Waldo, Assigned: Waldo)
References
Details
Attachments
(1 file, 2 obsolete files)
Driveby observed during other work.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8731083 -
Flags: review?(evilpies)
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8731102 -
Flags: review?(evilpies)
Assignee | ||
Updated•8 years ago
|
Attachment #8731083 -
Attachment is obsolete: true
Attachment #8731083 -
Flags: review?(evilpies)
Comment 3•8 years ago
|
||
Comment on attachment 8731083 [details] [diff] [review] Patch Review of attachment 8731083 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/proxy/ScriptedDirectProxyHandler.cpp @@ +147,5 @@ > + > + if (IsCallable(func)) > + return true; > + > + JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_BAD_TRAP); Don't you need to actually pass a trap name here? @@ +247,4 @@ > bool > ScriptedDirectProxyHandler::isExtensible(JSContext* cx, HandleObject proxy, bool* extensible) const > { > + // steps 1-3 I would prefer if we used // Steps 1-3. // Step 4. etc. And I don't understand why you decided to group those three together. @@ +258,3 @@ > RootedObject target(cx, proxy->as<ProxyObject>().target()); > > + // steps 5-6 This doesn't look right. GetMethod is just Step 5 with the new "?" syntax. This also applies to some other places, especially were we had implemented a draft. ::: js/src/tests/ecma_6/Proxy/trap-null.js @@ +31,5 @@ > + construct: null, > +}; > + > +var complainingHandler = new Proxy(allTraps, { > + get(target, p, receiver) { \o/
Attachment #8731083 -
Attachment is obsolete: false
Comment 4•8 years ago
|
||
Comment on attachment 8731102 [details] [diff] [review] Invoking a trap on a proxy, where the handler has null as the value of that trap, should fall back to operating on the target just like undefined would. NOT REVIEWED YET Review of attachment 8731102 [details] [diff] [review]: ----------------------------------------------------------------- Please do fix the comments. I think on IRC we discussed using the ES7 draft as the reference.
Attachment #8731102 -
Flags: review?(evilpies) → review+
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8731975 -
Flags: review?(evilpies)
Assignee | ||
Updated•8 years ago
|
Attachment #8731083 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8731102 -
Attachment is obsolete: true
Comment 6•8 years ago
|
||
Comment on attachment 8731975 [details] [diff] [review] Patch (only changing behavior, massive step-recommenting in a future patch if this approach passes muster) Review of attachment 8731975 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/proxy/ScriptedDirectProxyHandler.cpp @@ +952,2 @@ > bool > ScriptedDirectProxyHandler::call(JSContext* cx, HandleObject proxy, const CallArgs& args) const Did you forget to split those out?
Attachment #8731975 -
Flags: review?(evilpies) → review+
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/16e429ad5696
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•