Bug 40980 – Web Inspector: null is not properly handled when evaluated in console (V8 only)

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Kosyakov]

Injected script does not handle evaluation of null values properly. 
  
> null
TypeError: Cannot read property 'constructor' of null

[Andrey Kosyakov]

Created attachment 59369 [details]
patch

[Andrey Kosyakov]

Created attachment 59384 [details]
patch (more different types of objects added)

[Pavel Feldman]

Comment on attachment 59384 [details]
patch (more different types of objects added)

WebCore/inspector/front-end/InjectedScript.js:604
 +              objectText = type;
So you always return "function" for functions that have poor toString. Old code was smarter (before it got regressed). It was doing Object.prototype.toString(obj) on objects like this (which was evolved into "[object Function]" or similar). Then, className was extracting the "Function" part. I think we should fix the regression in the InjectedScript._toString first (see Safari 4 code for that). Note that there were some reservations wrt Object lifetime that were making Object.prototype.toString call problematic.

Maybe we should simplify the code as you suggested, but I don't think returning "function" for all objects like this is a good idea.


WebCore/inspector/front-end/InjectedScript.js: 
 +          if (typeof obj !== "object")
I think this will break chromium interactive tests. Take a look at r58956. I know that typeof null is in fact "object", so the fix in r58956 itself was arguable. Probably this needs a more strict check such as  (obj === null || typeof obj !== "object")

[Eric Seidel (no email)]

Comment on attachment 59369 [details]
patch

Cleared Yury Semikhatsky's review+ from obsolete attachment 59369 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Andrey Kosyakov]

Created attachment 59497 [details]
patch (fixed abbreviation, always use toString for all functions)

[Andrey Kosyakov]

(In reply to comment #3)
> (From update of attachment 59384 [details])
> WebCore/inspector/front-end/InjectedScript.js:604
>  +              objectText = type;
> So you always return "function" for functions that have poor toString. Old code was smarter (before it got regressed). It was doing Object.prototype.toString(obj) on objects like this (which was evolved into "[object Function]" or similar). Then, className was extracting the "Function" part. I think we should fix the regression in the InjectedScript._toString first (see Safari 4 code for that). Note that there were some reservations wrt Object lifetime that were making Object.prototype.toString call problematic.
> 
> Maybe we should simplify the code as you suggested, but I don't think returning "function" for all objects like this is a good idea.

As agreed offline -- there's not much sense in displaying "Function" rather than "function" for some functions; We choose to use function's toString() whatever it is.

> WebCore/inspector/front-end/InjectedScript.js: 
>  +          if (typeof obj !== "object")
> I think this will break chromium interactive tests. Take a look at r58956. I know that typeof null is in fact "object", so the fix in r58956 itself was arguable. Probably this needs a more strict check such as  (obj === null || typeof obj !== "object")

I don't think so -- the problem was because we were calling _className always, without regard to what _type() returns. Now we only call it for objects, and only if these are not nulls (null is treated as special type "null" by _type()).

[Pavel Feldman]

Comment on attachment 59497 [details]
patch (fixed abbreviation, always use toString for all functions)

WebCore/inspector/front-end/InjectedScript.js:604
 +              objectText = /.*/.exec(obj)[0].replace(/ +$/g, "");
did you mean exec(objectText) here?

LayoutTests/inspector/console-tests-expected.txt:37
 +  console-tests.html:22null console-message console-js-source console-log-level
I would expect to see console-formatted-null in the list of styles.

LayoutTests/platform/chromium/inspector/console-tests-expected.txt:1
 +  CONSOLE MESSAGE: line 9: log
Given that there is a handful of mismatches wrt WebKit, I would extract them into separate test so that we could reuse most of the standard expectations.

[Andrey Kosyakov]

Created attachment 59510 [details]
patch (separated tests with platform-specific expectations, fixed nits)

Re adding styles for type-specific formatting to tests expectation -- we agreed not to do it now, as it is logically unrelated to this fix.

[Yury Semikhatsky]

Comment on attachment 59510 [details]
patch (separated tests with platform-specific expectations, fixed nits)

WebCore/inspector/front-end/InjectedScript.js:604
 +              objectText = /.*/.exec(objectText)[0].replace(/ +$/g, "");
Please add a comment describing what the regexp does or better rewrite the code to make its intent clear.

[Yury Semikhatsky]

Comment on attachment 59510 [details]
patch (separated tests with platform-specific expectations, fixed nits)

Clearing flags on attachment: 59510

Committed r61748: <http://trac.webkit.org/changeset/61748>

[Yury Semikhatsky]

All reviewed patches have been landed.  Closing bug.