Closed Bug 1274657 Opened 4 years ago Closed 3 years ago

Inspecting proxy object should show [[ProxyHandler]] and [[ProxyTarget]] instead of executing traps

Categories

(DevTools :: Object Inspector, defect)

defect
Not set

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

Attachments

(1 file, 2 obsolete files)

Currently, inspecting a proxy object executes some of its traps. This can have the collateral effect of modifying the target object or the future behavior of the proxy traps, and thus the displayed data can be completely incoherent.

In the same way that getters of accessor properties are not executed during inspection, proxy traps shouldn't neither.

As proposed by Jason Orendorff in bug 1273235 comment #12:
> It might be better, when displaying a proxy, for devtools to refuse to play
> the proxy's stupid game, and show Proxy specially as an object with two
> properties, [[ProxyHandler]] and [[ProxyTarget]].
Depends on: 1282257
Assignee: nobody → oriol-bugzilla
Basically, if the object is a proxy, lie and make a grip with an invented class "Proxy". Add a pair of internal properties with the target and the handler. And avoid getOwnPropertyNames, isExtensible, getOwnPropertyDescriptor, etc.

Well, bug 1282257 has not landed yet, but is something like this appropriate?
Attachment #8772685 - Flags: feedback?(jlong)
Comment on attachment 8772685 [details] [diff] [review]
When inspecting a proxy, show the [[ProxyHandler]] and [[ProxyTarget]] instead of executing traps

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

Looks good to me!
Attachment #8772685 - Flags: feedback?(jlong) → feedback+
I have added tests.
Attachment #8772685 - Attachment is obsolete: true
Attachment #8774084 - Flags: review?(jlong)
Argh, I forgot to update a comment.
Attachment #8774084 - Attachment is obsolete: true
Attachment #8774084 - Flags: review?(jlong)
Attachment #8774085 - Flags: review?(jlong)
Comment on attachment 8774085 [details] [diff] [review]
When inspecting a proxy, show the [[ProxyHandler]] and [[ProxyTarget]] instead of executing traps

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

Nice, looks good

::: devtools/client/debugger/test/mochitest/browser_dbg_variables-view-07.js
@@ +46,5 @@
> +    data.expand();
> +    yield expanded;
> +    if (property === "<target>") {
> +      for(let [subprop, subdata] of data) if(subprop === "name") {
> +        is(subdata.value, "target", "The value of '<target>' should be the [[ProxyTarget]]");

Isn't the display value "<target>" not "target"?

::: devtools/client/shared/widgets/VariablesViewController.jsm
@@ +342,5 @@
> +      let deferred = defer();
> +      deferred.resolve();
> +      return deferred.promise;
> +    }
> +    

Nit: trailing space
Attachment #8774085 - Flags: review?(jlong) → review+
(In reply to James Long (:jlongster) from comment #5)
> Isn't the display value "<target>" not "target"?

Yes, but that is testing the value, not the pseudo-property name. I would have preferred something like `data.value === target`, but `data.value` is a grip, and even if I wrapped `target` in another grip, they would be different references.

So I added `target.name = "target"`, and then I iterate the subproperties of `proxy["<target>"]`. Since `"target"` is a primitive there is no problem when comparing.

It's not perfect, but in that test there isn't any other object with `.name = "target"`. And all traps of the proxy throw errors, so the proxy can't be confused with the target that way.

So I guess I can add checkin-needed.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/2e03460fc927
When inspecting a proxy, show the [[ProxyHandler]] and [[ProxyTarget]] instead of executing traps. r=jlong
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2e03460fc927
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Duplicate of this bug: 1190605
Depends on: 1291315
See Also: → 1298697
See Also: → 1388831
Duplicate of this bug: 664867
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.