1363588 - Chrome-logger stops outputting if null is an argument

Status: RESOLVED INVALID

(DevTools :: Console, defect, P3)

Description

[BKDotCom]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.81 Safari/537.36

Steps to reproduce:

ChromeLogger protocol (server side logging)

nutshell:I tried passing multiple "arguments" of different types to the group method/type
passing null to group kills the logger

a "row" such as the following
[ ["I'm a group", null], null "group" ] 


similarly
[ ["I'm a group", 123], null "group" ] 
only outputs "I'm a group"

seems "related":
https://bugzilla.mozilla.org/show_bug.cgi?id=1232615


Actual results:

chromelogger console output ceases after group entry with null argument is encountered
chromelogger group method only outputs first argument (unless argument is null, which "kills" the logger)


Expected results:

should behave just as console.group() and accept multiple/null parameter

Comment 1

[BKDotCom]

It seems that simply logging a null value prevents further output to the console.

[ ["hello world"], null, "log" ],
[ ["null kills", null], null, "log" ],
[ ["never seen"], null, "log" ]

Comment 2

[BKDotCom]

... a nested null value is fine

[ ["array with null", [null]], null, "log" ],
[ ["success!"], null, "info" ]

Comment 3

[Loic]

Could you provide a live testcase (like in bug 1232615), please.

Comment 4

[BKDotCom]

live testcase:
http://www.bradkent.com/ChromeLogger.php

Comment 5

[BKDotCom]

Found the problem

https://hg.mozilla.org/mozilla-central/file/tip/devtools/shared/webconsole/server-logger.js#!482

line 482

    if (typeof log == "object") {
      delete log.___class_name;
    }

the 'ol typeof null gotcha   (typeof null === "object")

ironically, this is directly related to my "contact the extension developer bug"
https://bugzilla.mozilla.org/show_bug.cgi?id=1375853

nutshell.. there is zero reason to delete the ___class_name property...   It's providing useful information.  Don't delete it simply because the current implementation doesn't do anything special with it.

lines 478 - 485 should be removed!

  // Remove special ___class_name property that isn't supported
  // by the current implementation. This property represents object class
  // allowing custom rendering in the console panel.
  for (let log of msg.logs) {
    if (typeof log == "object") {
      delete log.___class_name;
    }
  }

Note that the above doesn't attempt to remove the ___class_name property from nested objects (whether nested in array or object).   There's no reason to delete it from an object passed as a log argument.

Comment 6

[Greg Tatum [:gregtatum]]

Attachment: Bug 1363588 - Retain ___class_name property for messages in the WebConsole server logger;

Review commit: https://reviewboard.mozilla.org/r/158052/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158052/

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8887259 [details]
Bug 1363588 - Retain ___class_name property for messages in the WebConsole server logger;

https://reviewboard.mozilla.org/r/158052/#review163230

::: devtools/shared/webconsole/server-logger.js:481
(Diff revision 1)
>    msg.logs.unshift(...rebuiltLogArray);
>  
>    // Remove special ___class_name property that isn't supported
>    // by the current implementation. This property represents object class
>    // allowing custom rendering in the console panel.
>    for (let log of msg.logs) {

I'm also OK with removing the special case removal of `___class_name` here and then duping Bug 1375853 to this one. I'll leave it up to Honza

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8887259 [details]
Bug 1363588 - Retain ___class_name property for messages in the WebConsole server logger;

https://reviewboard.mozilla.org/r/158052/#review163520

Thanks for working on this!

Please see my inline comment.

Honza

::: devtools/shared/webconsole/server-logger.js:481
(Diff revision 1)
>    msg.logs.unshift(...rebuiltLogArray);
>  
>    // Remove special ___class_name property that isn't supported
>    // by the current implementation. This property represents object class
>    // allowing custom rendering in the console panel.
>    for (let log of msg.logs) {

Yes, ok for me too. Please update the patch and remove the loop entirely.

Comment 10

[Greg Tatum [:gregtatum]]

Comment on attachment 8887259 [details]
Bug 1363588 - Retain ___class_name property for messages in the WebConsole server logger;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158052/diff/1-2/

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8887259 [details]
Bug 1363588 - Retain ___class_name property for messages in the WebConsole server logger;

https://reviewboard.mozilla.org/r/158052/#review164096

Thanks for the patch Greg!

R+, but please fix the commit message (bgrins -> Honza)

Honza

Comment 12

[Greg Tatum [:gregtatum]]

I never followed through with landing this, but it looks like this feature has been removed. If I'm wrong please re-open this bug.