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)

defect
Not set
normal

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"
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.
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
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
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)
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)
This lets the variable view show up
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)
(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
(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.
(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).
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.
(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)
Depends on: 1383158
Depends on: 1383161
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Whiteboard: [browser-debugging-issues]
Still working on getting all of the necessary boilerplate in place for a test
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.
(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.
(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 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+
(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.
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 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+
>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.
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.
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
https://hg.mozilla.org/mozilla-central/rev/90c5dd7af31d
https://hg.mozilla.org/mozilla-central/rev/020aa7f3789c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
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]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.