Cannot show "window" object with inspect viewer in remote web console

RESOLVED FIXED in Firefox 19

Status

defect
RESOLVED FIXED
7 years ago
Last year

People

(Reporter: tetsuharu, Assigned: msucan)

Tracking

Trunk
Firefox 19

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [b2g])

Attachments

(1 attachment, 2 obsolete attachments)

[Enviroment]
* Firefox Desktop, Firefox Mobile for Android:
http://hg.mozilla.org/mozilla-central/rev/abe709bfc49a

[Step to reproduce]
1. Connect to Firefox Mobile with Remote Web Console.
2. Evaluate "window" in JSterm of remote web console.
3. Click the output of console, [object Window].

[Result]
Cannot show the sub-window which is object inspect viewer.
And occur the following error:
Error: Failed to retrieve the object properties from the server. Error: unknownError
Source File: chrome://browser/content/devtools/webconsole.js
Line: 2601

If I connected to "Global Console" of Firefox Mobile and run same steps (clicking [object ChromeWindow] at step 3), the following error is shown in Remote web console:
[Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIDOMWindow.crypto]"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame :: resource://gre/modules/devtools/WebConsoleUtils.jsm :: WCU_inspectObjectProperty :: line 519"  data: no] @ chrome://global/content/devtools/dbg-server.js:557
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#3369

This could conceivably just assign nullptr and return NS_OK instead.
(In reply to Josh Matthews [:jdm] from comment #1)
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.
> cpp#3369
> 
> This could conceivably just assign nullptr and return NS_OK instead.

This makes sense to me. I believe this is ddahl's baby IIANM.
This affects Firefox OS as well.
Whiteboard: [b2g]
Assignee

Comment 4

7 years ago
Posted patch quick patch (obsolete) — Splinter Review
Did this based on Josh's suggestion. It works: window.crypto is now null in b2g builds and I don't get any exceptions when inspecting the window object in b2g.

Is this good enough?
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Attachment #673903 - Flags: review?(bzbarsky)
Wouldn't the actual correct behavior be to not have the property on window at all?

That said, why do we have this ifdef in the first place?
Assignee

Comment 6

7 years ago
(In reply to Boris Zbarsky (:bz) from comment #5)
> Wouldn't the actual correct behavior be to not have the property on window
> at all?

Indeed. Can you please explain where how I can do that? (just a quick overview)

Thanks!

> That said, why do we have this ifdef in the first place?

hg blame points to bug 561244.
Ah, e10s.  <sigh>.

The best way to do that is to put the property on a separate interface that the window implements and make that interface conditional on either a pref or compile-time stuff.   There are existing examples of the former for Window in nsDOMClassInfo.cpp.
Assignee

Comment 8

7 years ago
(In reply to Boris Zbarsky (:bz) from comment #7)
> Ah, e10s.  <sigh>.
> 
> The best way to do that is to put the property on a separate interface that
> the window implements and make that interface conditional on either a pref
> or compile-time stuff.   There are existing examples of the former for
> Window in nsDOMClassInfo.cpp.

Thanks for your explanation. I'd love to learn how to do this, but for that I'd need more help. I don't want to take your time. If this is a good-first-bug, I would appreciate a possible mentor (suggestions are welcome!), otherwise I can unassign myself from the bug and let someone else take it.
I have no problem spending time mentoring this.  ;)  I don't think it would be a hard bug to fix, so if you want to go for it...  Which part of comment 7 was unclear?
Assignee

Comment 10

7 years ago
That's great! Thank you!

I'm somewhat confused about the interplay of nsGlobalWindow, nsDOMClassInfo and nsIDOMWindow.idl. I see nsGlobalWindow inherits from a few interfaces. In nsDOMClassInfo I found nsIDOMCrypto added, but it's not turned off at compile time - other interfaces have their entries added with ifdefs around. Then there's nsIDOMWindow.idl with all of the properties, including the crypto one - none of the props have ifdefs around.

I tried adding the ifdef around Crypto map entry, in nsDOMClassInfo and I still get window.crypto. I tried return false for eDOMClassInfo_Crypto_id in ConstructorEnabled() of nsDOMClassInfo, with no luck.

I'm not sure where to add the new/separate interface, what to inherit from (I assume nsIDOMCrypto?) and what to put in it, and how to use it in nsDOMClassInfo. I tried to follow the pattern of other properties that can be disabled via prefs (in nsDOMClassInfo) - with conditional map entries. Still no luck.

I'd like to not add a pref, just continue to use the compile-time option to disable crypto.
> I'm somewhat confused about the interplay of nsGlobalWindow, nsDOMClassInfo and
> nsIDOMWindow.idl.

nsGlobalWindor is an implementation class that implements a bunch of interfaces.  nsDOMClassInfo lists which ones it exposes to script.  nsIDOMWindow is one of those interfaces.

> I see nsGlobalWindow inherits from a few interfaces. 

Indeed.

> In nsDOMClassInfo I found nsIDOMCrypto added

That's for the object returned from the .crypto getter.

> I'm not sure where to add the new/separate interface, what to inherit from (I assume
> nsIDOMCrypto?) and what to put in it, and how to use it in nsDOMClassInfo.

You want to add a new interface with some name.  Maybe nsIWindowCrypto?

Then you want to move the "crypto" attribute from nsIDOMWindow to this new interface.

Then you want to implement this interface on nsGlobalWindow (mostly this means inheriting from it, QIing to it, and the right NS_DECL bit for it).

And finally, you want to do something like this in nsDOMClassInfo.cpp:

  #ifdef MOZ_DISABLE_DOMCRYPTO
  static const bool domCryptoEnabled = false;
  #else
  static const bool domCryptoEnabled = true;
  #endif

and then change DOM_CLASSINFO_WINDOW_MAP_ENTRIES to include:

  DOM_CLASSINFO_MAP_CONDITIONAL_ENTRY(nsIWindowCrypto, domCryptoEnabled)

You could also skip this whole domCryptoEnabled bit and simply use ifdefs on DOM_CLASSINFO_WINDOW_MAP_ENTRIES, but since you can't #ifdef inside a macro you'd have to do a bit more surgery on it.  You're welcome to do it that way if you want, of course!
Oh, and the new interface can just go in the same dir as nsIDOMWindow and would inherit from nsISupports.
Assignee

Comment 13

7 years ago
Posted patch proposed fix (obsolete) — Splinter Review
This is working for me - Firefox for desktop builds and b2g desktop builds.

Please let me know if I need to make further changes, and/or if I should also ask someone else for review.

Thanks for your help - this was new ground for me and I really liked learning.
Attachment #673903 - Attachment is obsolete: true
Attachment #673903 - Flags: review?(bzbarsky)
Attachment #674306 - Flags: review?(bzbarsky)
Comment on attachment 674306 [details] [diff] [review]
proposed fix

You need to change the iid of nsIDOMWindow.

r=me with that.  Thanks!
Attachment #674306 - Flags: review?(bzbarsky) → review+
Assignee

Comment 15

7 years ago
Posted patch updated fixSplinter Review
Updated based on the review comment.

Thank you Boris!

Green try push:
https://tbpl.mozilla.org/?tree=Try&rev=1492bea5915a

Landed in fx-team:

https://hg.mozilla.org/integration/fx-team/rev/9f2ef525135b
Attachment #674306 - Attachment is obsolete: true
Assignee

Updated

7 years ago
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [b2g] → [b2g][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9f2ef525135b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [b2g][fixed-in-fx-team] → [b2g]
Target Milestone: --- → Firefox 19
Just wanted Mihai to look at the "revert nsIWindowCrypto" patch on bug 683262 - we will have a proper crypto property in mobile now, but still not in b2g (yet) - does this cause any problems for devtools?
Assignee

Comment 18

7 years ago
David: I applied all of the patches from bug 683262. 'window' object inspection still works. The only difference I see is that window.crypto is now an object, but it is not inspectable.  What matters here is that it doesn't throw when we read the property.

Thank you for the ping.
(In reply to Mihai Sucan [:msucan] from comment #18)
> David: I applied all of the patches from bug 683262. 'window' object
> inspection still works. The only difference I see is that window.crypto is
> now an object, but it is not inspectable.  What matters here is that it
> doesn't throw when we read the property.
> 
> Thank you for the ping.

Thanks, Mihai!

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.