Closed Bug 1273235 Opened 8 years ago Closed 8 years ago

Inspecting proxy object with ownKeys trap throws internal error

Categories

(DevTools :: Object Inspector, defect)

33 Branch
defect
Not set
normal

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

(Keywords: regression)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160516030211

Steps to reproduce:

Open web console and enter
    new Proxy({}, {ownKeys: () => ['a', 'b']});

Click the returned object to inspect it.


Actual results:

Object inspector only shows the property `a` but not `b`.

Browser console shows this error:
    Handler function DebuggerClient.requester request callback threw an exception: TypeError: descriptor is undefined
    Stack: Scope.prototype.addItems@resource://devtools/client/shared/widgets/VariablesView.jsm:1373:9
    VariablesViewController.prototype._populateProperties/<@resource://devtools/client/shared/widgets/VariablesViewController.jsm:396:7
    DebuggerClient.requester/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:296:9
    exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
    emitOnObject@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:112:9
    emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:89:38
    Request.prototype.emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:1234:29
    DebuggerClient.prototype.onPacket/emitReply@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:1018:29
    DevTools RDP*DebuggerClient.prototype.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:724:5
    DebuggerClient.requester/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:284:12
    exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
    VariablesViewController.prototype._populateProperties@resource://devtools/client/shared/widgets/VariablesViewController.jsm:376:5
    VariablesViewController.prototype._populateFromObject@resource://devtools/client/shared/widgets/VariablesViewController.jsm:369:12
    VariablesViewController.prototype.populate@resource://devtools/client/shared/widgets/VariablesViewController.jsm:624:7
    VariablesViewController.prototype.setSingleVariable@resource://devtools/client/shared/widgets/VariablesViewController.jsm:761:21
    JSTerm.prototype._updateVariablesView@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/jsterm.js:768:34
    JSTerm.prototype.openVariablesView/onContainerReady@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/jsterm.js:585:7
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7
    Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11
    this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7
    this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7
    JSTerm.prototype._addVariablesViewSidebarTab/onTabReady@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/jsterm.js:647:7
    EventEmitter_once/handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:108:9
    EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:163:11
    ToolSidebar.prototype.addTab/onIFrameLoaded@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/sidebar.js:248:7
    EventListener.handleEvent*ToolSidebar.prototype.addTab@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/sidebar.js:251:5
    JSTerm.prototype._addVariablesViewSidebarTab@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/jsterm.js:660:7
    JSTerm.prototype.openVariablesView@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/jsterm.js:616:21
    ConsoleOutput.prototype.openVariablesView@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/console-output.js:360:5
    Widgets.JSObject.prototype<.openObjectInVariablesView@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/console-output.js:2555:5
    Widgets.JSObject.prototype<._onClick@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/console-output.js:2587:5
    WebConsoleFrame.prototype._addMessageLinkCallback/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/webconsole.js:2710:7
    EventListener.handleEvent*WebConsoleFrame.prototype._addMessageLinkCallback@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/webconsole.js:2688:5
    Messages.BaseMessage.prototype._addLinkCallback@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/console-output.js:551:5
    Widgets.JSObject.prototype<._anchor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/console-output.js:2540:5
    Widgets.JSObject.prototype<._renderObjectPrefix@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/console-output.js:2408:5
    .render@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/console-output.js:3469:5
    Widgets.JSObject.prototype<.render@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/console-output.js:2375:7
    Messages.Extended.prototype<._renderObjectActor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/console-output.js:1329:20
    Messages.Extended.prototype<._renderValueGrip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/console-output.js:1209:16
    Messages.Extended.prototype<._renderBodyPiece@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/console-output.js:1173:12
    Messages.Extended.prototype<.render@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/console-output.js:1139:26
    ConsoleOutput.prototype._onFlushOutputMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/console-output.js:244:12
    WebConsoleFrame.prototype._outputMessageFromQueue@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/webconsole.js:2220:16
    WebConsoleFrame.prototype._flushMessageQueue@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/webconsole.js:2109:20
    Line: 1373, column: 9


Expected results:

I'm not sure what exactly should happen when inspecting a proxy which says it has some own properties but the target doesn't have them. But no error, of course.
Blocks: 1007334
Component: Untriaged → Developer Tools: Object Inspector
Keywords: regression
Not sure if instead of coercing the descriptor to a boolean I should explicitly check if it's undefined, or maybe use `Object(descriptor) === descriptor` to check if it's an object, or maybe being object coercible is enough.
Attachment #8753015 - Flags: review?(past)
Assignee: nobody → oriol-bugzilla
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 8753015 [details] [diff] [review]
When inspecting a property, check if the descriptor is undefined before attempting to access its value

Makes sense to me, but Eddy has worked more with this code lately.
Attachment #8753015 - Flags: review?(past) → review?(ejpbruel)
Comment on attachment 8753015 [details] [diff] [review]
When inspecting a property, check if the descriptor is undefined before attempting to access its value

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

Patch looks good to me, but ugh this is weird. Are proxies allowed to do this, according to the spec?
Attachment #8753015 - Flags: review?(ejpbruel) → review+
(In reply to Eddy Bruel [:ejpbruel] from comment #3)
> Are proxies allowed to do this, according to the spec?

In this case the target is extensible, so [[OwnPropertyKeys]] can return a list which contains other values than the own properties of the target object, without violating invariants imposed in http://www.ecma-international.org/ecma-262/6.0/#sec-6.1.7.3 and http://www.ecma-international.org/ecma-262/6.0/#sec-9.5.12

So I think it's allowed.
Keywords: checkin-needed
I fixed it up so I could land this, but in the future, please make sure the patch contains the commit message.
Flags: needinfo?(oriol-bugzilla)
(In reply to Wes Kocher (:KWierso) from comment #5)
> I fixed it up so I could land this, but in the future, please make sure the
> patch contains the commit message.

OK, thanks, didn't know that. How can I do it?
Flags: needinfo?(oriol-bugzilla)
https://hg.mozilla.org/mozilla-central/rev/1942762d99b3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
By the way, the patch also fixes 

    new Proxy({a:1,b:2}, { getOwnPropertyDescriptor: ()=>{} })
Thanks for the patch, Oriol!

Eddy: Oriol's right, proxies are allowed to do this. As a rule of thumb, it's OK for a proxy to claim any result so long as the target object *could* be modified to make the result true. In this case, the proxy first claims to have two properties (no problem there, you could add proxies to the target) and then it claims not to have those properties (no problem there either - the target actually doesn't have them).
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]].
I have reproduced this bug with Nightly 49.0a1 (2016-05-16)  on windows 7 , 64 Bit!

This bug's fix is verified with latest Developer Edition, Nightly


Build ID   20160708004052
User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0


Build ID   20160714030208
User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0


But I could not find the bug's fix in Beta 48.0a1
(In reply to Maruf Rahman from comment #13)
> But I could not find the bug's fix in Beta 48.0a1

Yes, the target milestone is Firefox 49.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: