880752 - Console.jsm stdout/err output is joined with commas and nukes newlines

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[:Gijs (he/him)]

For Australis we're using Console.jsm for logging now. The output in the browser console looks awesome, but somehow stdout does stuff like this:

console.log: [CustomizeMode], Dumping drag data (CustomizeMode.jsm) {|  type: dragover|  target: [object XULElement](localName=vbox; id=customization-palette)|  currentTarget: [object XULElement](localName=vbox; id=customization-palette)|  dataTransfer[dropEffect]: move|  dataTransfer[effectAllowed]: move|  dataTransfer[files]: null|  dataTransfer[types]: [object DOMStringList]|  dataTransfer[mozItemCount]: 1|  dataTransfer[mozCursor]: auto|  dataTransfer[mozUserCancelled]: false|  dataTransfer[mozSourceNode]: [object XULElement]|}

Note the comma after CustomizeMode, and the pipes that substitute newlines. Here's a screenshot of a similar message in the browser console:

http://screencast.com/t/VtwIR4uQgOk

It'd be great if the stdout/err output looked less jumbled-together. :-)

(don't know if this is mac-specific so leaving like that for now)

Comment 1

[:Gijs (he/him)]

Attachment: Patch

I suppose I'm not 100% sure you want to take this. But it doesn't seem hard to change, so even if you say 'no' I figure I haven't got much to loose by way of writing a patch for this, other than the couple of minutes of my time. :-)

Comment 2

[Joe Walker [:jwalker]]

Comment on attachment 759984 [details] [diff] [review]
Patch

Review of attachment 759984 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry it's taken a while. I think we should do this.

Comment 3

[Joe Walker [:jwalker]]

I'll add this to my landing list.

Comment 4

[Joe Walker [:jwalker]]

Attachment: rebase

Comment 5

[Joe Walker [:jwalker]]

Comment on attachment 774529 [details] [diff] [review]
rebase

Review of attachment 774529 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/Console.jsm
@@ +147,5 @@
>    }
>  
> +  if (!aAllowNewLines) {
> +    let str = aThing.toString().replace(/\n/g, "|");
> +  }

I think what we mean is:

  let str = aThing.toString();
  if (!aAllowNewLines) {
    let str = str.replace(/\n/g, "|");
  }

Comment 6

[Joe Walker [:jwalker]]

Attachment: fix assignment to str bug

Comment 7

[Joe Walker [:jwalker]]

I think this patch is probably causing test failures.
See https://tbpl.mozilla.org/php/getParsedLog.php?id=25324388&tree=Try for information (non-windows builds).

Comment 8

[:Gijs (he/him)]

(In reply to Joe Walker [:jwalker] from comment #7)
> I think this patch is probably causing test failures.
> See https://tbpl.mozilla.org/php/getParsedLog.php?id=25324388&tree=Try for
> information (non-windows builds).

You're right, I checked locally.

It's because I broke things as you outlined in comment 5, but the fix should have omitted the 'let' in the block, because now 'str' is redeclared and empty when we hit that case, which breaks console.debug, which doesn't show up in the console because our reporting of errors in event handlers / timeouts is broken. :-(

New patch coming in a sec...

Comment 9

[:Gijs (he/him)]

Attachment: Console.jsm stdout/err output is joined with commas and nukes newlines;

Comment 10

[:Gijs (he/him)]

Comment on attachment 776993 [details] [diff] [review]
Console.jsm stdout/err output is joined with commas and nukes newlines;

Carrying over r+

Comment 11

[:Gijs (he/him)]

Landed in fx-team: https://hg.mozilla.org/projects/ux/rev/5bc40c94645c

Comment 12

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #11)
> Landed in fx-team: https://hg.mozilla.org/projects/ux/rev/5bc40c94645c

... and backed out because jetpack has tests that depend on this output. Grrrrrr.

https://hg.mozilla.org/integration/fx-team/rev/489187b2309d

Comment 13

[:Gijs (he/him)]

Looks like:

TEST-UNEXPECTED-FAIL | tests/test-plain-text-console.testPlainTextConsole | PlainTextConsole.log() must work. ("console.log: addon-sdk: testing 1 Array [2,3,4]\n" != "console.log: addon-sdk: testing, 1, Array [2,3,4]\n")
TEST-UNEXPECTED-FAIL | tests/test-plain-text-console.testPlainTextConsole | PlainTextConsole.info() must work. ("console.info: addon-sdk: testing 1 Array [2,3,4]\n" != "console.info: addon-sdk: testing, 1, Array [2,3,4]\n")
TEST-UNEXPECTED-FAIL | tests/test-plain-text-console.testPlainTextConsole | PlainTextConsole.warn() must work. ("console.warn: addon-sdk: testing 1 Array [2,3,4]\n" != "console.warn: addon-sdk: testing, 1, Array [2,3,4]\n")
TEST-UNEXPECTED-FAIL | tests/test-plain-text-console.testPlainTextConsole | PlainTextConsole.log() must stringify undefined. ("console.log: addon-sdk: testing undefined\n" != "console.log: addon-sdk: testing, undefined\n")
TEST-UNEXPECTED-FAIL | tests/test-plain-text-console.testPlainTextConsole | PlainTextConsole.log() must stringify null. ("console.log: addon-sdk: testing null\n" != "console.log: addon-sdk: testing, null\n")
TEST-UNEXPECTED-FAIL | tests/test-plain-text-console.testPlainTextConsole | PlainTextConsole.log() doesn't printify custom toString. ("console.log: addon-sdk: testing {}\n" != "console.log: addon-sdk: testing, {}\n")
TEST-UNEXPECTED-FAIL | tests/test-plain-text-console.testPlainTextConsole | PlainTextConsole.log() must stringify custom bad toString. ("console.log: addon-sdk: testing {}\n" != "console.log: addon-sdk: testing, {}\n")


Alexandre, how do we proceed here? Are the modifications I'm proposing OK for jetpack? (I think so, but hey...) And, do we need to time a jetpack merge to fixing this, or can we dual-land a fix to jetpack on fx-team, or...?

Comment 14

[Alexandre Poirot [:ochameau]]

Sure your modification makes sense, we just need to tweak jetpack test accordingly.
I fade away from jetpack so I'm not up to date with how to handle such scenario.
I think we can land SDK fixes on mozilla-central (or indirectly other branches like fx-team, ux, integration) and these fixes will be cherry picked at some point in SDK git repo.

Otherwise, you can submit a pull request on github:
https://github.com/mozilla/addon-sdk
and try to land both patches synchronously, but that will most likely do some orange on TBPL before both patches get checked in.

Mossop can confirm or tell exactly how to proceed.

Comment 15

[Dave Townsend [:mossop]]

Yeah if you land in fx-team or somewhere, either ping us or we'll likely spot it anyway and we can uplift to our repo so we don't overwrite it later.

Comment 16

[:Gijs (he/him)]

(In reply to Dave Townsend (:Mossop) from comment #15)
> Yeah if you land in fx-team or somewhere, either ping us or we'll likely
> spot it anyway and we can uplift to our repo so we don't overwrite it later.

Great! Relanded as:

remote:   https://hg.mozilla.org/integration/fx-team/rev/728643fc9e3c
remote:   https://hg.mozilla.org/integration/fx-team/rev/1d2f3eb69d26

(the second being the modifications to jetpack)

Comment 17

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/a2b5119f1eb0a034d5121dd13402f34fbea23621
Bug 880752 - Fix jetpack tests to account for new stringified output, r=Mossop

Comment 18

[Dave Townsend [:mossop]]

(In reply to :Gijs Kruitbosch (PTO Aug 8-Aug 18) from comment #16)
> (In reply to Dave Townsend (:Mossop) from comment #15)
> > Yeah if you land in fx-team or somewhere, either ping us or we'll likely
> > spot it anyway and we can uplift to our repo so we don't overwrite it later.
> 
> Great! Relanded as:
> 
> remote:   https://hg.mozilla.org/integration/fx-team/rev/728643fc9e3c
> remote:   https://hg.mozilla.org/integration/fx-team/rev/1d2f3eb69d26
> 
> (the second being the modifications to jetpack)

For future reference you do need to get review even for test changes before landing. This one's trivial so take my post-r+.

Comment 19

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

can this land?

Comment 20

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

oops, already fixed-in-fx-team! move along.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/728643fc9e3c
https://hg.mozilla.org/mozilla-central/rev/1d2f3eb69d26

Comment 22

[:Gijs (he/him)]

(In reply to Dave Townsend (:Mossop) from comment #18)
> (In reply to :Gijs Kruitbosch (PTO Aug 8-Aug 18) from comment #16)
> > (In reply to Dave Townsend (:Mossop) from comment #15)
> > > Yeah if you land in fx-team or somewhere, either ping us or we'll likely
> > > spot it anyway and we can uplift to our repo so we don't overwrite it later.
> > 
> > Great! Relanded as:
> > 
> > remote:   https://hg.mozilla.org/integration/fx-team/rev/728643fc9e3c
> > remote:   https://hg.mozilla.org/integration/fx-team/rev/1d2f3eb69d26
> > 
> > (the second being the modifications to jetpack)
> 
> For future reference you do need to get review even for test changes before
> landing. This one's trivial so take my post-r+.

Oops! I misunderstood your comment earlier. Very sorry; thanks for the post-r+!