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)

defect
Not set
normal

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.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8731083 - Flags: review?(evilpies)
Attachment #8731083 - Attachment is obsolete: true
Attachment #8731083 - Flags: review?(evilpies)
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 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+
Attachment #8731083 - Attachment is obsolete: true
Attachment #8731102 - Attachment is obsolete: true
Blocks: 1257758
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+
https://hg.mozilla.org/mozilla-central/rev/16e429ad5696
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.