1205353 - Wrong result for string.replace with backslash-dollar (/\$/) inside JS debugger.

Status: VERIFIED FIXED

(DevTools :: Debugger, defect)

Description

[Shlomi Fish]

Attachment: triple-backslash-dollar-sign-JS-test.xhtml

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.

Comment 1

[Shlomi Fish]

Note that using \\$ (double backslash) instead of \$ (for all occurences) fixes the problem.

Comment 2

[:Gijs (he/him)]

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?

Comment 3

[Shlomi Fish]

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?

Comment 4

[Shlomi Fish]

Attachment: Sample XHTML file that can be used to reproduce the problem with the debugger (take2)

Comment 5

[Shlomi Fish]

Attachment: Screenshot of the offending problem - a watch expression in the debugger.

This screenshot shows the watch expression giving the wrong result inside the debugger.

Comment 6

[:Gijs (he/him)]

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?

Comment 7

[:Gijs (he/him)]

(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.

Comment 8

[Shlomi Fish]

(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

[Eddy Bruel [:ejpbruel]]

(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.

Comment 10

[:Gijs (he/him)]

(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).

Comment 11

[:Gijs (he/him)]

(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

[Brian Grinstead [:bgrins]]

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.

Comment 13

[Shlomi Fish]

Attachment: [PATCH] Fix the problem (and related ones) - with a test

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 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 16

[Shlomi Fish]

(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

[Victor Porof [:vporof][:vp]]

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.

Comment 18

[Shlomi Fish]

(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

[Victor Porof [:vporof][:vp]]

You should revise the patch.

Comment 20

[Shlomi Fish]

Attachment: 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.

Comment 21

[Shlomi Fish]

(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

Comment 22

[Shlomi Fish]

Hi all!

Why wasn't the patch applied yet? Is there anything I can do to speed up the process?

Comment 23

[:Gijs (he/him)]

(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.

Comment 24

[Pulsebot]

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

Comment 25

[Shlomi Fish]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b4600877d959

Comment 27

[Shlomi Fish]

(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

[:Gijs (he/him)]

(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! :-)

Comment 29

[Shlomi Fish]

(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

[Mayur Patil]

[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'),

Comment 31

[Shlomi Fish]

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?

Comment 32

[:Gijs (he/him)]

(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. :-)