Closed
Bug 1205353
Opened 6 years ago
Closed 5 years ago
Wrong result for string.replace with backslash-dollar (/\$/) inside JS debugger.
Categories
(DevTools :: Debugger, defect)
Tracking
(firefox46 verified)
VERIFIED
FIXED
Firefox 46
Tracking | Status | |
---|---|---|
firefox46 | --- | verified |
People
(Reporter: shlomif, Assigned: shlomif)
Details
(Whiteboard: [polish-backlog])
Attachments
(4 files, 1 obsolete file)
777 bytes,
application/xhtml+xml
|
Details | |
94.68 KB,
image/png
|
Details | |
6.68 KB,
patch
|
vporof
:
feedback+
|
Details | Diff | Splinter Review |
6.41 KB,
patch
|
vporof
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:40.0) Gecko/20100101 Firefox/40.0 Build ID: 20150826023504 Steps to reproduce: If I debug a JaveScript program using the debugger for web scripts (Tools -> Web Developer -> Debugger) and then enter '$$$'.replace(/\$\$\$/, 'foo') as a watch expression. Actual results: I am getting '$$$foo'. Expected results: I should be getting 'foo'. It works fine in the web console, and in the chromium-browser debugger and also in perl-5.22.0-6.mga6. Thanks for "<hashtag>" from Freenode for inspiring the discovery of this bug and some people on #firefoz on Moznet for investigating.
Assignee | ||
Comment 1•6 years ago
|
||
Note that using \\$ (double backslash) instead of \$ (for all occurences) fixes the problem.
Comment 2•6 years ago
|
||
I'm not sure I understand the issue you're having. Your testcase does: function test() { return '$$$'.replace( new RegExp('\$\$\$', 'g'), '$$$' ); // should result in the same '$$$' as before. } that is effectively the same as: function test() { return '$$$'.replace( /$$$/g, '$$$' ); // should result in the same '$$$' as before. } (because your escape '\'s are in a string, not in the regular expression, and so they have 0 effect) which actually produces '$$$$$', both in the console and in your code (which assigns to innerText which doesn't work on Firefox, so the result is invisible...). That result actually doesn't make sense to me. 6 $ would have made sense, but why 5? match() says a 0-length match was made, so where does the extra dollar sign go? This seems unrelated to what you actually describe in comment #0 which says you're using the watch functionality in the debugger, but where the output would make sense if using new Regexp('\$\$\$'), but wouldn't make sense if using /\$\$\$/. Can you clarify?
Flags: needinfo?(shlomif)
Assignee | ||
Comment 3•6 years ago
|
||
Hi Gijs, (In reply to :Gijs Kruitbosch from comment #2) > I'm not sure I understand the issue you're having. Your testcase does: > > function test() { > return '$$$'.replace( > new RegExp('\$\$\$', 'g'), > '$$$' > ); > // should result in the same '$$$' as before. > } > > that is effectively the same as: > > function test() { > return '$$$'.replace( > /$$$/g, > '$$$' > ); > // should result in the same '$$$' as before. > } > > (because your escape '\'s are in a string, not in the regular expression, > and so they have 0 effect) > > which actually produces '$$$$$', both in the console and in your code (which > assigns to innerText which doesn't work on Firefox, so the result is > invisible...). That result actually doesn't make sense to me. 6 $ would have > made sense, but why 5? match() says a 0-length match was made, so where does > the extra dollar sign go? This may be a different bug. I just carried over the reproducing test case from what "<hashtag>" (= the IRC nickname who discussed it with me) And I explored and placed it here, but it appears it can be invoked in other contexts in different pages without this code while in the debugger. > > This seems unrelated to what you actually describe in comment #0 which says > you're using the watch functionality in the debugger, but where the output > would make sense if using new Regexp('\$\$\$'), but wouldn't make sense if > using /\$\$\$/. Yes, it is unrelated. I'll attach a different test case soon. Regards, -- Shlomi Fish > > Can you clarify?
Flags: needinfo?(shlomif)
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
This screenshot shows the watch expression giving the wrong result inside the debugger.
Updated•6 years ago
|
Attachment #8661895 -
Attachment is obsolete: true
Comment 6•6 years ago
|
||
Eddy, do you know what's going on here? I can confirm that this produces "$$$foo" in the debugger, but bringing up the console while stopped on the breakpoint using [esc] and putting it in there does produce 'foo' as expected. Do we do some kind of escaping/processing on the input in the watch panel that would explain this?
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools: Debugger
Ever confirmed: true
Flags: needinfo?(ejpbruel)
Comment 7•6 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #2) > <something like "$$$".replace(/$$$/g, "$$$") > > which actually produces '$$$$$', <snip> > That result actually doesn't make sense to me. 6 $ would have > made sense, but why 5? match() says a 0-length match was made, so where does > the extra dollar sign go? Err, I temporarily forgot that the second parameter to str.replace *also* assigns a special meaning to "$". See https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/String/replace#Specifying_a_string_as_a_parameter - this makes sense of the result of the (now obsolete) first attached testcase.
Assignee | ||
Comment 8•6 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #7) > (In reply to :Gijs Kruitbosch from comment #2) > > <something like "$$$".replace(/$$$/g, "$$$") > > > which actually produces '$$$$$', <snip> > > That result actually doesn't make sense to me. 6 $ would have > > made sense, but why 5? match() says a 0-length match was made, so where does > > the extra dollar sign go? > > Err, I temporarily forgot that the second parameter to str.replace *also* > assigns a special meaning to "$". See > https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/ > Global_Objects/String/replace#Specifying_a_string_as_a_parameter - this > makes sense of the result of the (now obsolete) first attached testcase. Thanks for the investigation and clarification. And thanks for confirming the original reported bug.
Comment 9•6 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #6) > Eddy, do you know what's going on here? I can confirm that this produces > "$$$foo" in the debugger, but bringing up the console while stopped on the > breakpoint using [esc] and putting it in there does produce 'foo' as > expected. Do we do some kind of escaping/processing on the input in the > watch panel that would explain this? I'm not an expert on the console, so I have no idea what could be going on here. Perhaps bgrins would have a better idea.
Flags: needinfo?(ejpbruel) → needinfo?(bgrinstead)
Comment 10•6 years ago
|
||
(In reply to Eddy Bruel [:ejpbruel] from comment #9) > (In reply to :Gijs Kruitbosch from comment #6) > > Eddy, do you know what's going on here? I can confirm that this produces > > "$$$foo" in the debugger, but bringing up the console while stopped on the > > breakpoint using [esc] and putting it in there does produce 'foo' as > > expected. Do we do some kind of escaping/processing on the input in the > > watch panel that would explain this? > > I'm not an expert on the console, so I have no idea what could be going on > here. Perhaps bgrins would have a better idea. The incorrect result here (for "$$$".replace(/\$\$\$/, "foo")) would be "foo" (the console result), not "$$$foo" (which is what happens in the debugger).
Flags: needinfo?(ejpbruel)
Comment 11•6 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #10) > (In reply to Eddy Bruel [:ejpbruel] from comment #9) > > (In reply to :Gijs Kruitbosch from comment #6) > > > Eddy, do you know what's going on here? I can confirm that this produces > > > "$$$foo" in the debugger, but bringing up the console while stopped on the > > > breakpoint using [esc] and putting it in there does produce 'foo' as > > > expected. Do we do some kind of escaping/processing on the input in the > > > watch panel that would explain this? > > > > I'm not an expert on the console, so I have no idea what could be going on > > here. Perhaps bgrins would have a better idea. > > The incorrect result here (for "$$$".replace(/\$\$\$/, "foo")) would be > "foo" (the console result), not "$$$foo" (which is what happens in the > debugger). Gah, too many negations. The *correct* result is the console one, and the incorrect one is the debugger watch result, so this is something to do with how the debugger evals watches.
Comment 12•6 years ago
|
||
It seems like this is a debugger-specific issue. Most likely the watch input is somehow escaping the value. I can reproduce by pasting this into a watch expression: "$$$".replace(/\$/, '') which returns "$$$" but running that in the console returns "$$" as expected.
Flags: needinfo?(bgrinstead)
Whiteboard: [polish-backlog]
Assignee | ||
Comment 13•5 years ago
|
||
This is a patch against hg 273465:ec628289d8b4ed310463a0729c3e60a7798dfcac to fix the problem. I ran the mochitest on the -02.js test script and it failed without the code change and passed with the code change. Now it seems it is passing all tests in: $ ./mach mochitest devtools/client/debugger/test/ 2>&1 | tee ~/firefox-mochitest-devtools-client-debugger-test-FULL.txt ; n --msg finished runtests.py | Running tests: end. TEST-INFO | checking window state Browser Chrome Test Summary Passed: 10024 Failed: 0 Todo: 1 *** End BrowserChrome Test Results *** SUITE-END | took 412s Yay! ============= I hereby place this patch under the Public Domain-CC-Zero/MIT X11/MPL/GPL/LGPL/any-other licence of your choice. That put aside, crediting me (= Shlomi Fish) and linking to my personal web site - http://www.shlomifish.org/ - in the commit message/etc. will be appreciated (but not absolutely necessary due to its Public Domain nature). Please look into applying this patch.
Comment on attachment 8691112 [details] [diff] [review] [PATCH] Fix the problem (and related ones) - with a test Review of attachment 8691112 [details] [diff] [review]: ----------------------------------------------------------------- Setting a reviewer so it's not lost, feel free to redirect.
Attachment #8691112 -
Flags: review?(nfitzgerald)
Comment 15•5 years ago
|
||
Comment on attachment 8691112 [details] [diff] [review] [PATCH] Fix the problem (and related ones) - with a test Review of attachment 8691112 [details] [diff] [review]: ----------------------------------------------------------------- I'm not really familiar with how we implemented watch expressions, but I think Victor is. ::: devtools/client/debugger/test/mochitest/browser_dbg_watch-expressions-02.js @@ +67,5 @@ > gWatch.addExpression("throw new Error(\"bazinga\")"); > gWatch.addExpression("({ get error() { throw new Error(\"bazinga\") } }).error"); > gWatch.addExpression("throw { get name() { throw \"bazinga\" } }"); > + > + // info("getAllStrings() == ««" + gWatch.getAllStrings().join('<foo>') + "»»"); We shouldn't be committing commented out code, so if all these things are unnecessary, we should just remove them.
Attachment #8691112 -
Flags: review?(nfitzgerald) → review?(vporof)
Assignee | ||
Comment 16•5 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #15) > Comment on attachment 8691112 [details] [diff] [review] > [PATCH] Fix the problem (and related ones) - with a test > > Review of attachment 8691112 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm not really familiar with how we implemented watch expressions, but I > think Victor is. > > ::: > devtools/client/debugger/test/mochitest/browser_dbg_watch-expressions-02.js > @@ +67,5 @@ > > gWatch.addExpression("throw new Error(\"bazinga\")"); > > gWatch.addExpression("({ get error() { throw new Error(\"bazinga\") } }).error"); > > gWatch.addExpression("throw { get name() { throw \"bazinga\" } }"); > > + > > + // info("getAllStrings() == ««" + gWatch.getAllStrings().join('<foo>') + "»»"); > > We shouldn't be committing commented out code, so if all these things are > unnecessary, we should just remove them. Well, the patch I submitted could use some cleanup as it's in a preliminary version. I added the info()/getAllStrings() call for debugging (it required a little research) and later left it commented-out for reference , but I'm OK with it being completely removed.
Comment 17•5 years ago
|
||
Comment on attachment 8691112 [details] [diff] [review] [PATCH] Fix the problem (and related ones) - with a test Review of attachment 8691112 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/debugger/debugger-controller.js @@ +1143,4 @@ > // Make sure all quotes are escaped in the expression's syntax, > // and add a newline after the statement to avoid comments > // breaking the code integrity inside the eval block. > + aString.replace(/\\/g, "\\\\").replace(/"/g, "\\$&") + "\" + " + "'\\n'" + " + \"" + You might know the story about `get_temporary_buffer`. Well, `get_temporary_buffer` was (and still is) a C++ STL helper that had to do something, the exact details of which are irrelevant to the morale of the story itself. Suffice to say that a correct implementation would have depended on certain environmental variables (notably: the operating system), to which a pragmatic solution at the time was to just write a placeholder that behaved correctly but did what it was supposed to do in a very inefficient way. In other words, it was a hack, which the author documented as such and urged the implementation to be replaced. I shall leave a page of a rather useful book (relatively speaking, depending on the interested party) here for further consideration: https://pbs.twimg.com/media/CSWF852UEAAPG6o.jpg:large As a careful observer might have noticed the comment "TODO: handle all of this server-side: Bug 832470, comment 14" in this function's documentation. Without even going through the trouble of `hg blame`ing this, the simple fact that the bug number starts with an 8 and has only 6 digits hints to the fact that this is relatively old, on a devtools timescale. Looking back at this code, I feel nostalgia but also sadness. This regex was a horrible hack that was put in place because we didn't have proper server-side support for evaluating watch expressions. *** Shlomi, your patch is correct. We should land it. Thank you for contributing! I'd like to see the changes suggested by Nick made in the test.
Attachment #8691112 -
Flags: review?(vporof) → feedback+
Assignee | ||
Comment 18•5 years ago
|
||
(In reply to Victor Porof [:vporof][:vp] from comment #17) > Comment on attachment 8691112 [details] [diff] [review] > [PATCH] Fix the problem (and related ones) - with a test > > Review of attachment 8691112 [details] [diff] [review]: > ----------------------------------------------------------------- > > Shlomi, your patch is correct. We should land it. Thank you for > contributing! I'd like to see the changes suggested by Nick made in the test. Thanks for reviewing the patch, Victor! And you're welcome. That put aside, do you want me to revise the patch or should someone else do that? -- Shlomi Fish
Comment 19•5 years ago
|
||
You should revise the patch.
Assignee | ||
Comment 20•5 years ago
|
||
This patch removes the commented out code per Victor’s request. I didn't test it yet, in order to verify that I didn't break anything, because firefox is still building but will do so as soon as I humanly can.
Assignee | ||
Comment 21•5 years ago
|
||
(In reply to Shlomi Fish from comment #20) > Created attachment 8697545 [details] [diff] [review] > firefox-debugger-backslash-dollar-mochitest-patch-4.diff > > This patch removes the commented out code per Victor’s request. I didn't > test it yet, in order to verify that I didn't break anything, because > firefox is still building but will do so as soon as I humanly can. OK, hg default with this patch passes all tests of: $ ./mach mochitest devtools/client/debugger/test/ Browser Chrome Test Summary Passed: 9395 Failed: 0 Todo: 1
Updated•5 years ago
|
Flags: needinfo?(ejpbruel)
Attachment #8697545 -
Flags: review?(vporof)
Updated•5 years ago
|
Attachment #8697545 -
Flags: review?(vporof) → review+
Assignee | ||
Comment 22•5 years ago
|
||
Hi all! Why wasn't the patch applied yet? Is there anything I can do to speed up the process?
Comment 23•5 years ago
|
||
(In reply to Shlomi Fish from comment #22) > Hi all! > > Why wasn't the patch applied yet? Is there anything I can do to speed up the > process? Sorry, it looks like it slipped through the cracks. We don't have a fully automated system for doing checkins (yet - we're working on it!). I've pushed your patch to try: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a709efa027d and set the checkin-needed flag.
Keywords: checkin-needed
Comment 24•5 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/b4600877d959
Keywords: checkin-needed
Assignee | ||
Comment 25•5 years ago
|
||
Hi :Gijs, (In reply to :Gijs Kruitbosch from comment #23) > (In reply to Shlomi Fish from comment #22) > > Hi all! > > > > Why wasn't the patch applied yet? Is there anything I can do to speed up the > > process? > > Sorry, it looks like it slipped through the cracks. We don't have a fully > automated system for doing checkins (yet - we're working on it!). > Ah, I see. So it's good that I wrote the reminder. > I've pushed your patch to try: > > remote: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a709efa027d > > and set the checkin-needed flag. Thanks! ♥!
Comment 26•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b4600877d959
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Assignee | ||
Comment 27•5 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #26) > https://hg.mozilla.org/mozilla-central/rev/b4600877d959 Many thanks for applying the patch! That was quick. ♥!
Comment 28•5 years ago
|
||
(In reply to Shlomi Fish from comment #27) > (In reply to Carsten Book [:Tomcat] from comment #26) > > https://hg.mozilla.org/mozilla-central/rev/b4600877d959 > > Many thanks for applying the patch! That was quick. ♥! Thank you for writing it! :-)
Assignee: nobody → shlomif
Assignee | ||
Comment 29•5 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #28) > > Many thanks for applying the patch! That was quick. ♥! > > Thank you for writing it! :-) You're welcome! The pleasure is all mine: http://www.shlomifish.org/humour/fortunes/show.cgi?id=hacker-sees-bug .
Comment 30•5 years ago
|
||
[bugday-20160323] Status: RESOLVED,FIXED -> UNVERIFIED Comments: Passed '$$$'.replace(/\$\$\$/, 'foo') into add watch expression. Component: Name Firefox Version 46.0b4 Build ID 20160322075646 Update Channel beta User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 OS Windows 7 SP1 x86_64 Expected Results: Unable to verify the result i.e. $$$ as expected. Actual Results: Unable to get the expression when passing '$$$'.replace(/\$\$\$/, 'foo'),
Assignee | ||
Comment 31•5 years ago
|
||
Hi Mayur Patil, (In reply to Mayur Patil from comment #30) > [bugday-20160323] > > Status: RESOLVED,FIXED -> UNVERIFIED > > Comments: > Passed '$$$'.replace(/\$\$\$/, 'foo') into add watch expression. > > Component: > Name Firefox > Version 46.0b4 > Build ID 20160322075646 > Update Channel beta > User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 > Firefox/46.0 > OS Windows 7 SP1 x86_64 > > Expected Results: > Unable to verify the result i.e. $$$ as expected. > > Actual Results: > Unable to get the expression when passing '$$$'.replace(/\$\$\$/, 'foo'), First of all, this bug is still marked as RESOLVED/FIXED. So I don't know what effect the comment was supposed to have. Secondly, I've now tested it in latest firefox nightly (48.0a1) on Mageia Linux 6 x86-64 and it seems to be OK there: http://www.shlomifish.org/Files/files/images/firefox-nightly-bug-1205353.png I am getting «"foo"» as the result of the evaluated expression which is the expected and correct result. Which result are you getting? Can you provide a screenshot?
Updated•5 years ago
|
Status: RESOLVED → VERIFIED
Comment 32•5 years ago
|
||
(In reply to Shlomi Fish from comment #31) > Hi Mayur Patil, > > (In reply to Mayur Patil from comment #30) > > [bugday-20160323] > > > > Status: RESOLVED,FIXED -> UNVERIFIED > > > > Comments: > > Passed '$$$'.replace(/\$\$\$/, 'foo') into add watch expression. > > > > Component: > > Name Firefox > > Version 46.0b4 > > Build ID 20160322075646 > > Update Channel beta > > User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 > > Firefox/46.0 > > OS Windows 7 SP1 x86_64 > > > > Expected Results: > > Unable to verify the result i.e. $$$ as expected. > > > > Actual Results: > > Unable to get the expression when passing '$$$'.replace(/\$\$\$/, 'foo'), > > First of all, this bug is still marked as RESOLVED/FIXED. So I don't know > what effect the comment was supposed to have. Secondly, I've now tested it > in latest firefox nightly (48.0a1) on Mageia Linux 6 x86-64 and it seems to > be OK there: > > http://www.shlomifish.org/Files/files/images/firefox-nightly-bug-1205353.png > > I am getting «"foo"» as the result of the evaluated expression which is the > expected and correct result. Which result are you getting? Can you provide a > screenshot? Mayur seems to now comment on lots of bugs that were fixed for 45/46 and if they can't / don't know how to verify them posts comments like this. They also don't seem to CC themselves on these bugs, so I don't know that they'll have seen your comment. I don't think there's anything to worry about, and so I'm marking this as verified per your screenshot. :-)
Updated•3 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•