Closed
Bug 1382833
Opened 7 years ago
Closed 7 years ago
Browser Console properties pane doesn't work for a remote <browser>
Categories
(DevTools :: Console, defect)
DevTools
Console
Tracking
(firefox56 fixed)
RESOLVED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: MattN, Assigned: bgrins)
References
Details
(Keywords: reproducible, Whiteboard: [browser-debugging-issues])
Attachments
(4 files)
STR: 1) Open the Browser Console with chrome debugging enabled 2) Type: gBrowser.selectedBrowser 3) Click on "<browser" to inspect the properties Expected result: Show all properties of the <browser> in the sidebar Actual result: Empty sidebar Error messages: >error occurred while processing 'prototypeAndProperties: Error: unsafe CPOW usage forbidden Stack: grip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/object.js:95:22 objectGrip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:477:12 createValueGrip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/object.js:2243:14 createValueGrip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:426:12 createValueGrip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:470:29 _propertyDescriptor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/object.js:490:22 onPrototypeAndProperties@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/object.js:271:29 onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1799:15 send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:570:13 exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14 exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14 Line: 95, column: 22 main.js:1662 _unknownError resource://devtools/server/main.js:1662:5 onPacket resource://devtools/server/main.js:1801:29 send/< resource://devtools/shared/transport/transport.js:570:13 exports.makeInfallible/< resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14 exports.makeInfallible/< resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14 > "onPacket threw an exception: Error: Server did not specify an actor, dropping packet: {"error":"unknownError","message":"error occurred while processing 'prototypeAndProperties: Error: unsafe CPOW usage forbidden\nStack: grip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/object.js:95:22\nobjectGrip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:477:12\ncreateValueGrip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/object.js:2243:14\ncreateValueGrip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:426:12\ncreateValueGrip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:470:29\n_propertyDescriptor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/object.js:490:22\nonPrototypeAndProperties@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/object.js:271:29\nonPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1799:15\nsend/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:570:13\nexports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14\nexports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14\nLine: 95, column: 22"} Stack: onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:944:9 send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:570:13 exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14 exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14 Line: 944, column: 9"
Assignee | ||
Comment 1•7 years ago
|
||
Note: this will certainly also affect the OI once the new frontend is enabled in the Browser Console (bug 1347127). The weird thing though, is that if I run this in about:addons: `window.getInterface(Components.interfaces.nsIDocShell).chromeEventHandler.ownerGlobal.gBrowser.selectedBrowser` then expand the browser in the OI I don't see an error. And, after doing that all of a sudden running `gBrowser.selectedBrowser` and inspecting it works in the Browser Console.
Assignee | ||
Comment 2•7 years ago
|
||
Also, this works in the Browser Toolbox's console with a bunch of warnings: "unsafe/forbidden CPOW usage", as opposed to the "operation not possible on dead CPOW" exception we see in the Browser Console. The warning message originates from JavaScriptParent::allowMessage at https://dxr.mozilla.org/mozilla-central/source/js/ipc/JavaScriptParent.cpp?#88
Assignee | ||
Comment 3•7 years ago
|
||
This is throwing on the call to this.obj.isExtensible() here when it is creating a grip for a Window object in the debugger: https://github.com/mozilla/gecko-dev/blob/6aae95fab209597024c22c99525234ba2c462060/devtools/server/actors/object.js#L95
Assignee | ||
Comment 4•7 years ago
|
||
Jim, is it expected that a Debugger.Object will ever throw on a call to isExtensible() (or isSealed / isFrozen)? We are seeing an exception "unsafe CPOW usage forbidden" in the object grip when attempting to inspect gBrowser.selectedBrowser in the variables view (my best guess is on the `contentWindowAsCPOW` or `contentDocumentAsCPOW` properties).
Flags: needinfo?(jimb)
Assignee | ||
Comment 5•7 years ago
|
||
Update: it's the gBrowser.selectedBrowser._contentWindow property that is causing an issue. It can be "fixed" by try/catching the call to isExtensible, etc and early returning the grip (which means that individual property can't be inspected but at least it doesn't break inspection of the rest of the object). I'd like to find a better way to at least detect this, since I assume there are potential references to unsafe CPOWs anywhere a Debugger.Object is used. Is it possible/reasonable to somehow allow unsafe CPOWs if devtools is requesting them, so that firefox devs can inspect these objects in the Browser Console? Otherwise I guess the best case is to find any place where Debugger.Objects are used in the backend, check for unsafe CPOWs, and somehow render them differently with a new object grip.
Flags: needinfo?(wmccloskey)
Assignee | ||
Comment 6•7 years ago
|
||
This lets the variable view show up
Comment 7•7 years ago
|
||
It's never supposed to. When the D.O's referent is a proxy, it's not clear how the D.O can do its job without letting the proxy handlers run - but those are debuggee code. So it's best to always check isProxy first, and only call isExtensible, etc. when isProxy returns false. But even when the referent is a proxy, Debugger.Object.prototype.isExtensible etc. shouldn't throw. So I think there are two changes here: 1) The server should check isProxy first, and not call isExtensible etc. on proxies. I hope this won't lose too much information. 2) The Debugger should deal with proxies in some reasonable way.
There's a Cu.isCrossProcessWrapper method that you can use to determine if something is a CPOW. The best thing to do in that case is hide or ignore it.
Flags: needinfo?(wmccloskey)
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Jim Blandy :jimb from comment #7) > It's never supposed to. > > When the D.O's referent is a proxy, it's not clear how the D.O can do its > job without letting the proxy handlers run - but those are debuggee code. So > it's best to always check isProxy first, and only call isExtensible, etc. > when isProxy returns false. > > But even when the referent is a proxy, > Debugger.Object.prototype.isExtensible etc. shouldn't throw. > > So I think there are two changes here: > > 1) The server should check isProxy first, and not call isExtensible etc. on > proxies. I hope this won't lose too much information. We are doing this already (at least for this particular caller): https://github.com/mozilla/gecko-dev/blob/6aae95fab209597024c22c99525234ba2c462060/devtools/server/actors/object.js#L89-L98
Reporter | ||
Comment 10•7 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #8) > There's a Cu.isCrossProcessWrapper method that you can use to determine if > something is a CPOW. The best thing to do in that case is hide or ignore it. I would have said we should show the property name with a fallback value like "(CPOW)" which is similar to what we do for optimized out properties. It will be misleading to simply omit a property that you can see code is successfully using.
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #10) > (In reply to Bill McCloskey (:billm) from comment #8) > > There's a Cu.isCrossProcessWrapper method that you can use to determine if > > something is a CPOW. The best thing to do in that case is hide or ignore it. Jim, will a call to obj.unsafeDereference() ever throw? If not, sounds like we could at least prevent the error with an early return when Cu.isCrossProcessWrapper(obj.unsafeDereference()). We should at least make sure the Debugger.Object implementation doesn't throw across any of its methods, even when encountering a CPOW, though, since this could affect all callers of the debugger api. > I would have said we should show the property name with a fallback value > like "(CPOW)" which is similar to what we do for optimized out properties. > It will be misleading to simply omit a property that you can see code is > successfully using. I think we can handle this in the devtools object grip code pretty easily (if we detect a CPOW then force the class to be something like `"CPOW: " + this.obj.class`), which would show "CPOW: Window" in this particular case).
Comment 12•7 years ago
|
||
Okay, so `Debugger.Object.prototype.isProxy` asks specifically whether the referent is a *scripted* proxy, the sort you create with `new Proxy`. It doesn't actually cover SpiderMonkey's internal ProxyObject concept, which is what wrappers and CPOWs are built on. So now I understand why that isProxy test isn't enough.
Comment 13•7 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #11) > (In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking > you) from comment #10) > > (In reply to Bill McCloskey (:billm) from comment #8) > > > There's a Cu.isCrossProcessWrapper method that you can use to determine if > > > something is a CPOW. The best thing to do in that case is hide or ignore it. > > Jim, will a call to obj.unsafeDereference() ever throw? If not, sounds like > we could at least prevent the error with an early return when > Cu.isCrossProcessWrapper(obj.unsafeDereference()). I think that should work. However, you should make sure Cu.isCrossProcessWrapper itself doesn't throw, even when presented with an opaque wrapper like the ones used in workers. > We should at least make sure the Debugger.Object implementation doesn't > throw across any of its methods, even when encountering a CPOW, though, > since this could affect all callers of the debugger api. I looked over the Debugger isExtensible code again. All that's happening is that it gets the D.O's referent and asks it if it's extensible. That's an operation that can throw; consider what should happen when we ask an opaque wrapper whether it's extensible; that's none of our business, right? I'm no longer so sure that D.O.p.isExtensible and friends shouldn't throw. The question is, given the variety of referents a D.O could have, and the ways they're allowed to behave, what's the most useful API for the server? How should D.O helpfully reflect the fact that the referent is unwilling to say whether it's extensible or not? Throwing an exception isn't an unreasonable choice: it has the advantage of letting the caller see the exact exception the referent threw. I'm open to suggestions, but at the moment I'd say that the grip code should be prepared for inspection to throw, simply because that's something that SpiderMonkey objects are allowed to do.
Flags: needinfo?(jimb)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
Whiteboard: [browser-debugging-issues]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
Still working on getting all of the necessary boilerplate in place for a test
Reporter | ||
Comment 16•7 years ago
|
||
As a note (perhaps obviously) this bug also affects the autocomplete popup so it would be good go to double-check this patch also fixes that.
Assignee | ||
Comment 17•7 years ago
|
||
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #16) > As a note (perhaps obviously) this bug also affects the autocomplete popup > so it would be good go to double-check this patch also fixes that. You mean how the popup doesn't show up when you type `gBrowser.selectedBrowser.`? I see this too, but I don't think it's related to CPOWs - it happens even when opening about:preferences or another page where the contentWindow is not a CPOW.
Comment hidden (mozreview-request) |
Reporter | ||
Comment 19•7 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #17) > (In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking > you) from comment #16) > > As a note (perhaps obviously) this bug also affects the autocomplete popup > > so it would be good go to double-check this patch also fixes that. > > You mean how the popup doesn't show up when you type > `gBrowser.selectedBrowser.`? I see this too, but I don't think it's related > to CPOWs - it happens even when opening about:preferences or another page > where the contentWindow is not a CPOW. Yeah, that's what I meant. Is that a known bug?
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8888915 [details] Bug 1382833 - Don't throw when dealing with CPOWs in the Object Actor; https://reviewboard.mozilla.org/r/159938/#review165366 ::: devtools/server/actors/object.js:95 (Diff revision 2) > g.class = "Proxy"; > g.proxyTarget = this.hooks.createValueGrip(this.obj.proxyTarget); > g.proxyHandler = this.hooks.createValueGrip(this.obj.proxyHandler); > } else { > - g.class = this.obj.class; > + try { > + g.class = DevToolsUtils.isCPOW(this.obj) ? ("CPOW: " + this.obj.class) : this.obj.class; Ideally, we'd have a separate field on the grip, like: cross_process: true instead of decorating the class name. Then the UI can be translated, etc. If this has to be visible directly in the user interface, could we make this "Cross-process" or something less obscure than "CPOW"?
Attachment #8888915 -
Flags: review?(jimb) → review+
Assignee | ||
Comment 21•7 years ago
|
||
(In reply to Jim Blandy :jimb from comment #20) > Comment on attachment 8888915 [details] > Bug 1382833 - Don't throw when dealing with CPOWs in the Object Actor; > > https://reviewboard.mozilla.org/r/159938/#review165366 > > ::: devtools/server/actors/object.js:95 > (Diff revision 2) > > g.class = "Proxy"; > > g.proxyTarget = this.hooks.createValueGrip(this.obj.proxyTarget); > > g.proxyHandler = this.hooks.createValueGrip(this.obj.proxyHandler); > > } else { > > - g.class = this.obj.class; > > + try { > > + g.class = DevToolsUtils.isCPOW(this.obj) ? ("CPOW: " + this.obj.class) : this.obj.class; > > Ideally, we'd have a separate field on the grip, like: cross_process: true > instead of decorating the class name. Then the UI can be translated, etc. Good point, I'll update it > If this has to be visible directly in the user interface, could we make this > "Cross-process" or something less obscure than "CPOW"? I don't have a strong opinion on the string we use, but AFAIK this should only be visible to Firefox devs (in chrome-priv contexts), where the "CPOW" term seems pretty well publicized.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 24•7 years ago
|
||
I tried to add this as a mochitest-chrome test and got a lot of the way there, but then I realized that mochitest-chrome runs in non-e10s. The next best thing I could come up with was to use a Browser Console mochitest-browser test and work directly with the webconsole client / object client rather than clicking around the UI
Comment 25•7 years ago
|
||
mozreview-review |
Comment on attachment 8889629 [details] Bug 1382833 - Add regression test for CPOWs in the Browser Console; https://reviewboard.mozilla.org/r/160644/#review165946 ::: devtools/client/webconsole/test/browser_console.js:177 (Diff revision 1) > + actor: cpowEval.result.actor, > + }); > + > + // Before the fix for Bug 1382833, this wouldn't resolve due to a CPOW error > + // in the ObjectActor. > + let prototypeAndProperties = yield objectClient.getPrototypeAndProperties(); I wish there were a way to make tests fail when the server throws an exception, instead of just hanging. But I guess that's out of scope for this bug.
Attachment #8889629 -
Flags: review?(jimb) → review+
Comment 26•7 years ago
|
||
>I don't have a strong opinion on the string we use, but AFAIK this should only be visible to Firefox devs (in chrome-priv contexts), where the "CPOW" term seems pretty well publicized.
Good point - I agree.
Assignee | ||
Comment 27•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8888915 [details] Bug 1382833 - Don't throw when dealing with CPOWs in the Object Actor; https://reviewboard.mozilla.org/r/159938/#review165366 > Ideally, we'd have a separate field on the grip, like: cross_process: true instead of decorating the class name. Then the UI can be translated, etc. > > If this has to be visible directly in the user interface, could we make this "Cross-process" or something less obscure than "CPOW"? I spent some time stepping through this and the variables view code, and ultimately settled on continuing to modify the class. It was hard for me to be sure we would be always handling this properly for all types of objects in the vview. And since the vview is only used still in the Browser Console (and only until we switch the BC to use the new console frontend) I didn't want to spend more time on it.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•7 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a094b0611622bec4af334a17e4c058bf911d066
Comment 31•7 years ago
|
||
Pushed by bgrinstead@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/90c5dd7af31d Don't throw when dealing with CPOWs in the Object Actor;r=jimb https://hg.mozilla.org/integration/autoland/rev/020aa7f3789c Add regression test for CPOWs in the Browser Console;r=jimb
Comment 32•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/90c5dd7af31d https://hg.mozilla.org/mozilla-central/rev/020aa7f3789c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Comment 33•7 years ago
|
||
I have reproduced this bug with Nightly 56.0a1 (2017-07-20)on Windows 7, 64 Bit! This bug's fix is verified with latest Beta! Build ID : 20170903140023 User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [testday-20170901]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•