Closed
Bug 1310801
Opened 8 years ago
Closed 8 years ago
web console stops printing output after attempting to print a string with non-printable characters
Categories
(DevTools :: Shared Components, defect, P2)
DevTools
Shared Components
Tracking
(firefox52 fixed)
RESOLVED
FIXED
Firefox 52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: keeler, Assigned: Honza)
References
Details
Attachments
(1 file, 3 obsolete files)
3.82 KB,
patch
|
Honza
:
review+
|
Details | Diff | Splinter Review |
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
Comment 1•8 years ago
|
||
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•8 years ago
|
||
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 3•8 years ago
|
||
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•8 years ago
|
||
(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)
Comment 5•8 years ago
|
||
> 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 6•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bb432564f4d8 Remove non-printable characters. r=nchevobbe
Keywords: checkin-needed
Comment 10•8 years ago
|
||
Backed out for test_reps_string.html failures. https://treeherder.mozilla.org/logviewer.html#?job_id=38059629&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/4c8b24548bef2430360a0f01e09f05358975e8e8
Updated•8 years ago
|
Blocks: enable-new-console
Priority: -- → P2
Assignee | ||
Comment 11•8 years ago
|
||
Test fixed Honza
Attachment #8803302 -
Attachment is obsolete: true
Attachment #8805082 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 12•8 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dd039b88b9485fca1bbb40310f19f10963f8b92 Honza
Comment 13•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/68f683a2034b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Comment 15•8 years ago
|
||
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]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•