632275 - Web Console: Error message when click on an object to create an inspect window

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[B.J. Herbison]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13 ( .NET CLR 3.5.30729; .NET4.0C)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13 ( .NET CLR 3.5.30729; .NET4.0C)

I get an error message when I click on an object to create an inspect window, but not with the command "inspect (document)". The message appears for HTML documents, but not from a XULDocument like the Add-ons Manager or about:config.

I see the error with Windows 7, Windows XP, and Ubuntu 10.10, and I saw the message with the 6 February and 7 February builds.

Reproducible: Always

Steps to Reproduce:
1. Visit a page (about:blank, about:home, about:support, cnn.com, yahoo.com, google.com, ...).
2. Open Web Console with control-shift-K.
3. Enter the command "document".
4. Click on [Object HTMLDocument].

Actual Results:  
The inspect window appears, but there is also a message in the web console:

[21:29:26.183] Non-standard document.width was used. Use standard document.body.clientWidth instead. @ http://www.google.com/

Expected Results:  
The inspect window appears without an error message in the web console.

Comment 1

[Maniac Vlad Florin (:vladmaniac)]

Thanks for reporting this B.J. !

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

Regression window: 

Build Worked:

Mozilla/5.0 (Windows NT 5.1; rv:2.0b8pre) Gecko/20101010 Firefox/4.0b8pre

http://hg.mozilla.org/mozilla-central/rev/26c47ba8064f

Build broken: 


Mozilla/5.0 (Windows NT 5.1; rv:2.0b7) Gecko/20100101 Firefox/4.0b7

http://hg.mozilla.org/mozilla-central/rev/297086a0fb61

Pretty strange that beta7 RC is broken, and still beta8pre worked..
hope it helps

Comment 3

[Mihai Sucan [:msucan]]

Funny bug.

This warning started showing up since we pushed the patches for window IDs (bug 603706 or bug 606498 - can't remember which). I didn't notice back then, but the problem is really simple: when you open the Object Inspector for the "document" object, you get the list of all properties, including document.width. Whenever you read document.width you also get a warning.

The warning origin is that of the document being inspected, hence it is displayed in the Web Console.

Not sure what we can do about this, actually. Should we skip the listing of "width" from HTMLDocument objects? Or do we mark this bug as WONTFIX?

Comment 4

[Rob Campbell [:rc] (:robcee)]

annoying. I've seen it too. It'd be nice if we could somehow suppress that error message. Or annotate it to explain why it's showing up.

Comment 5

[Mihai Sucan [:msucan]]

(In reply to comment #4)
> annoying. I've seen it too. It'd be nice if we could somehow suppress that
> error message. Or annotate it to explain why it's showing up.

I would suggest suppressing the message by avoiding to read document.width at all. It's deprecated and we shouldn't show it in document object inspection. If this sounds fine for you, I can submit a patch.

An uglier hack would be to suppress CSS parser messages while the Property Panel opens up for HTMLDocument objects.

Comment 6

[Kevin Dangoor]

Is there any way to notice that document.width is a getter and not just a simple attribute? I worry about this now that getters are going to become more common... we don't want to call getter functions.

Beyond that, I think Mihai's suggestion of treating document.width specially is okay for at least eliminating this error.

Comment 7

[Rob Campbell [:rc] (:robcee)]

for (var key in document) { 
  if (document.__lookupGetter__(key)) // do something with this key
}

We could get a list of getters for a given object with this and then do something special with them.

Comment 8

[Mihai Sucan [:msucan]]

Attachment: proposed fix

This is the proposed patch.

It skips document.width/height and shows Getter when the property has a getter, so we don't execute non-native getters, to not affect the web application state. Also included a mochitest.

(Wrt. a better UI for all this... we'll need to do it once we redesign/rewrite the property panel UI.)

Looking forward to your feedback. Thanks!

Comment 9

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 518736 [details] [diff] [review]
proposed fix

+const NATIVE_FUNCTION_REGEX = /^function\s*[a-z0-9_A-Z]*\(\)\s*\{\s*\[native code\]\s*\}$/;

Please comment this so future versions of ourselves and others can figure out what you're matching with a minimum of consternation.

I think this can be simplified a little too. Can we not replace [a-z0-9_A-Z]* with \w* ?

https://developer.mozilla.org/en/Core_JavaScript_1.5_Guide/Regular_Expressions

+    // Also skip non-native getters.
+    getter = aObject.__lookupGetter__(propName);
+    if (getter && !isNativeFunction(getter)) {
+      value = "";
+      presentable = {type: TYPE_OTHER, display: "Getter"};
+    }

Kind of weird. I guess we won't see a value next to the property name for these? Probably sensible since we can't be sure of side-effects.

Tests look good.

f+ with nits addressed.

Comment 10

[Mihai Sucan [:msucan]]

(In reply to comment #9)
> Comment on attachment 518736 [details] [diff] [review]
> proposed fix
> 
> +const NATIVE_FUNCTION_REGEX = /^function\s*[a-z0-9_A-Z]*\(\)\s*\{\s*\[native
> code\]\s*\}$/;
> 
> Please comment this so future versions of ourselves and others can figure out
> what you're matching with a minimum of consternation.

Yeah.

> I think this can be simplified a little too. Can we not replace [a-z0-9_A-Z]*
> with \w* ?

Agreed. It seems to work.

> https://developer.mozilla.org/en/Core_JavaScript_1.5_Guide/Regular_Expressions
> 
> +    // Also skip non-native getters.
> +    getter = aObject.__lookupGetter__(propName);
> +    if (getter && !isNativeFunction(getter)) {
> +      value = "";
> +      presentable = {type: TYPE_OTHER, display: "Getter"};
> +    }
> 
> Kind of weird. I guess we won't see a value next to the property name for
> these? Probably sensible since we can't be sure of side-effects.

The code needs that value and I can't give it any value, so I used "". Added a comment to mention that value is not displayed.


> f+ with nits addressed.

Thanks! Will update the patch now.

Comment 11

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated the patch as requested by Robert. Thanks for the f+!

Asking for review from Dolske.

Comment 12

[Mihai Sucan [:msucan]]

Comment on attachment 519171 [details] [diff] [review]
updated patch

Asking for review from Dave Camp. Thanks!

Comment 13

[Dave Camp (:dcamp)]

Comment on attachment 519171 [details] [diff] [review]
updated patch

>+function isNativeFunction(aFunction)
>+{
>+  return NATIVE_FUNCTION_REGEX.test(aFunction.toSource());
>+}

As discussed in IRC, checking (typeof aFunction == 'function' && 'prototype' in aFunction) is probably a better test for native-function-ness.

We also discussed in IRC that a better approach to solving the general problem (trusting browser-provided getters but not content-provided getters) would be to enumerate the xray wrapper first (trusting getters on that) and then enumerate the unwrapped object (not trusting getters).  Mihai's going to file a follow up bug on that.

With the change to isNativeFunction() and a comment pointing at the followup bug for enumerating the unwrapped object, I'd give my f+/r+/whatever I'm qualified to give+.

Comment 14

[Mihai Sucan [:msucan]]

Attachment: patch update 2

Updated the patch as requested. Thanks for your review, Dave!

I reported the follow up bug 647235.

Please let me know if further changes are needed.

Comment 15

[Dave Camp (:dcamp)]

Comment on attachment 523604 [details] [diff] [review]
patch update 2

There's one more catch of using 'is this a native function' as the heuristic for deciding "safe" (dom-provided) getters, which is that functions created with Function.prototype.bind() are considered native.  So a getter created with obj.__defineGetter__('foo', bar.bind({})) will be executed with this test.

This still strikes me as better than what's there now, and there's a followup filed for figuring out a better solution.  r+, forwarding to dolske for a binding r+.

Comment 16

[Mihai Sucan [:msucan]]

Comment on attachment 523604 [details] [diff] [review]
patch update 2

Changing reviewer to Shawn.

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> >+function isNativeFunction(aFunction)
> >+{
> >+  return NATIVE_FUNCTION_REGEX.test(aFunction.toSource());
> >+}
> 
> As discussed in IRC, checking (typeof aFunction == 'function' && 'prototype' in
> aFunction) is probably a better test for native-function-ness.
I do not believe this to be correct, actually.  I ran some tests in the console:
[15:00:07.201] function foo() { let i = 0; return i++; }
[15:00:07.204] undefined
--
[15:00:12.785] foo()
[15:00:12.787] 0
--
[15:00:23.257] typeof foo
[15:00:23.259] "function"
--
[15:00:32.329] 'prototype' in foo
[15:00:32.331] true
--
[15:08:34.723] foo.toSource()
[15:08:34.725] "function foo() {var i = 0;return i++;}"

My function foo meets the two criteria that the current patch has, but is clearly not a native function.  I'm assuming that by "native function" we mean implemented in C++ code.

Comment 18

[Dave Camp (:dcamp)]

(In reply to comment #13)
> As discussed in IRC, checking (typeof aFunction == 'function' && 'prototype' in
> aFunction) is probably a better test for native-function-ness.

Err, to clarify - I meant !('prototype' in aFunction), that was a typo (and it's correct in the patch).

Comment 19

[Shawn Wilsher :sdwilsh]

dcamp has pointed out that his comment contained a typo that the patch did not (namely, that it should be negated).  I've confirmed that this works:
[15:15:25.317] dump.toSource()
[15:15:25.319] "function dump() {[native code]}"
--
[15:15:35.517] typeof dump
[15:15:35.519] "function"
--
[15:15:44.789] 'prototype' in dump
[15:15:44.791] false

Comment 20

[Shawn Wilsher :sdwilsh]

Comment on attachment 523604 [details] [diff] [review]
patch update 2

>+++ b/toolkit/components/console/hudservice/PropertyPanel.jsm
> /**
>+ * Tells if the given function is native or not.
>+ *
>+ * @param function aFunction
More verbosity please

>+ * @return boolean
>+ *         True if the given funct
I think you are missing some text here

>+++ b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_632275_getters_document_width.js
>@@ -0,0 +1,92 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Web Console test suite.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * The Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2011
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Mihai Sucan <[e-mail]>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
http://www.mozilla.org/MPL/boilerplate-1.1/pd-c please

r=sdwilsh

Comment 21

[Mihai Sucan [:msucan]]

Attachment: patch update 3

Updated as requested. I don't know how I managed to break that jsdoc comment - fixed now. Thanks Shawn!

Comment 22

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 525451 [details] [diff] [review]
patch update 3

http://hg.mozilla.org/projects/devtools/rev/1fe294269d54

Comment 23

[Rob Campbell [:rc] (:robcee)]

backed out in http://hg.mozilla.org/projects/devtools/rev/0bcb4052e18c

likely culprit of the backout is this bug.

run a full mochitest-browser-chrome suite to see the error.

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_inspector_domPanel.js | domPanel shows the correct value for rand1302649245940
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_inspector_domPanel.js | domPanel shows the correct tagName
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_inspector_highlighter.js | Test timed out

Comment 24

[Mihai Sucan [:msucan]]

Attachment: [checked-in][in-devtools] patch update 4

It seems that __lookupGetter__ is not available on some DOM objects that are used by the DOM Inspector tool.

Updated the patch. Tests pass locally, but I will push to the try server.

Comment 25

[Mihai Sucan [:msucan]]

Green results:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=3c89418a5d28

Comment 26

[Mihai Sucan [:msucan]]

Comment on attachment 525732 [details] [diff] [review]
[checked-in][in-devtools] patch update 4

Pushed:
http://hg.mozilla.org/projects/devtools/rev/cbd3cdb82da2

Comment 27

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 525732 [details] [diff] [review]
[checked-in][in-devtools] patch update 4

http://hg.mozilla.org/mozilla-central/rev/1fe294269d54

Comment 28

[Rob Campbell [:rc] (:robcee)]

http://hg.mozilla.org/mozilla-central/rev/cbd3cdb82da2

Comment 29

[AndreiD[QA]]

VERIFIED FIXED ON:
Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110511 Firefox/6.0a1
Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110512 Firefox/6.0a1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110511 Firefox/6.0a1