web console stops printing output after attempting to print a string with non-printable characters

RESOLVED FIXED in Firefox 52

Status

()

Firefox
Developer Tools: Shared Components
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: keeler, Assigned: Honza)

Tracking

Trunk
Firefox 52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

STR:

Open the web console
Type (including quotes) '\x00'
Hit enter
Type anything else (and hit enter again)

Expected results:

The console should probably display something for the '\x00' string, but that's open to interpretation. It should definitely keep displaying other things, though.

Actual results:

The console just goes silent, and won't display anything else until you close/reopen it.
Component: Developer Tools → Developer Tools: Console
This is likely an issue with Reps, which both the console and debugger use to output values received from the backend. Honza, do you have any thoughts? Feel free to bounce it back to console if I'm wrong about that.
Component: Developer Tools: Console → Developer Tools: Shared Components
Flags: needinfo?(odvarko)
(Assignee)

Comment 2

2 years ago
Created attachment 8802536 [details] [diff] [review]
bug1310801.patch

The problem is in StringRep. The attached patch removes non-printable characters before rendering in HTML using regular expression. But, keeping \n and \t.

This certainly fixes the problem, but I am not 100% sure if the reg exp is comprehensive enough. We might also want to use something as follows:

http://stackoverflow.com/questions/11598786/how-to-replace-non-printable-unicode-characters-javascript
(see the first answer)

Or am I missing something? Is there a simple solution?

Honza
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)
Attachment #8802536 - Flags: review?(chevobbe.nicolas)
Comment on attachment 8802536 [details] [diff] [review]
bug1310801.patch

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

The regex is not that easy to understand, but since you added a comment, that's fine.
I wonder if we could do this replace in the `cropString` function. 
Since it is used for the comment rep and the text node one, it would handle this issue for those, which would be nice.
Also, this need some tests to ensure the problem is fixed.

::: devtools/client/shared/components/reps/string.js
@@ +50,5 @@
>  
> +      // Remove all non-printable characters.
> +      // eslint-disable-next-line no-control-regex
> +      let re = new RegExp("[\x00-\x08\x0B\x0C\x0E-\x1F\x80-\x9F]", "g");
> +      formattedString = formattedString.replace(re, "");

In the current console, we display a "" character for those, maybe we still want to have it ?
Attachment #8802536 - Flags: review?(chevobbe.nicolas) → review-
(Assignee)

Comment 4

2 years ago
Created attachment 8802992 [details] [diff] [review]
bug1310801.patch

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> Comment on attachment 8802536 [details] [diff] [review]
> bug1310801.patch
> 
> Review of attachment 8802536 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The regex is not that easy to understand, but since you added a comment,
> that's fine.
I yet improved the comment a bit.

> I wonder if we could do this replace in the `cropString` function. 
> Since it is used for the comment rep and the text node one, it would handle
> this issue for those, which would be nice.
Good point, done.

> Also, this need some tests to ensure the problem is fixed.
I extended the test for StringRep.

> 
> ::: devtools/client/shared/components/reps/string.js
> @@ +50,5 @@
> >  
> > +      // Remove all non-printable characters.
> > +      // eslint-disable-next-line no-control-regex
> > +      let re = new RegExp("[\x00-\x08\x0B\x0C\x0E-\x1F\x80-\x9F]", "g");
> > +      formattedString = formattedString.replace(re, "");
> 
> In the current console, we display a "" character for those, maybe we still
> want to have it ?
The characters has been mangled, but I agree we can display something instead of it . Do you have any specific char in mind. What's the code?

Honza
Attachment #8802536 - Attachment is obsolete: true
Attachment #8802992 - Flags: review?(chevobbe.nicolas)
> The characters has been mangled, but I agree we can display something instead of it . Do you have any specific char in mind. What's the code?

Seems that I can't copy it. If you can, enter "\x00" in the old console frontend (can be the jsconsole since it doesn't use the new frontend yet).
Comment on attachment 8802992 [details] [diff] [review]
bug1310801.patch

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

Seems good to me !

::: devtools/client/shared/components/reps/rep-utils.js
@@ +47,5 @@
>  
>      // Make sure it's a string.
>      text = text + "";
>  
> +    // Remove all non-printable characters except of

Nice indeed
Attachment #8802992 - Flags: review?(chevobbe.nicolas) → review+
(Assignee)

Comment 7

2 years ago
Created attachment 8803302 [details] [diff] [review]
bug1310801.patch

Thank for the review!

Patch updated: unicode replacement characters u+fffd used to replace non-printable characters.

Honza
Attachment #8802992 - Attachment is obsolete: true
Attachment #8803302 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Updated

2 years ago
Duplicate of this bug: 1311915

Comment 9

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb432564f4d8
Remove non-printable characters. r=nchevobbe
Keywords: checkin-needed
Blocks: 1308219
Priority: -- → P2
(Assignee)

Comment 11

2 years ago
Created attachment 8805082 [details] [diff] [review]
bug1310801.patch

Test fixed

Honza
Attachment #8803302 - Attachment is obsolete: true
Attachment #8805082 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 13

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68f683a2034b
Remove non-printable characters. r=nchevobbe
Keywords: checkin-needed

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/68f683a2034b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
I have reproduced this bug with Nightly 52.0a1 (2016-10-17) (64-bit) on Windows 7 , 64 bit !

This bug's fix is verified with latest Nightly
 
Build ID        20161102030205
User Agent      Mozilla/5.0 (WindowsNT 6.1; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0  
[bugday-20161102]
You need to log in before you can comment on or make changes to this bug.