407448 - firefox -remote 'openURL(<URL>)' doesn't work when <URL> contains a closing parenthesis ")"

Status: RESOLVED WORKSFORME

(Toolkit :: Startup and Profile System, defect)

Description

[Kanemura Atsunori]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.8.1.6) Gecko/20060201 Firefox/2.0.0.6 (Ubuntu-dapper)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.8.1.6) Gecko/20060201 Firefox/2.0.0.6 (Ubuntu-dapper)

http://lxr.mozilla.org/seamonkey/source/toolkit/components/remote/nsGTKRemoteService.cpp#370
FindChar should be RFind

Reproducible: Always

Steps to Reproduce:
1. Start firefox.
2. Type
$ firefox -remote 'openURL(http://www.google.com/search?q=)+google)'

Actual Results:  
$ firefox -remote 'openURL(http://www.google.com/search?q=)+google)'
Error: Failed to send command: 500 command not parseable


Expected Results:  
A new tab or window should open with the given URL.

We can avoid this error by replacing ")" by "%29"

I found the source of this bug in this line:
http://lxr.mozilla.org/seamonkey/source/toolkit/components/remote/nsGTKRemoteService.cpp#370
FindChar should be RFindChar, as in the correct parsing in:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/xremote/src/XRemoteService.cpp#94

Comment 1

[Elmar Ludwig]

Confirming and reassigning.

The cause of the problem is likely here:
http://lxr.mozilla.org/mozilla/source/browser/components/nsBrowserContentHandler.js#334

However, since the x-remote spec doesn't mention any way to quote special characters like ")", this may end up as WONTFIX.

Gavin:
Regarding your statement in bug 298960 comment #42, do you think that we want to support this use case? Being able to quote the URL would certainly help here.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #1)
> Gavin:
> Regarding your statement in bug 298960 comment #42, do you think that we want
> to support this use case? Being able to quote the URL would certainly help
> here.

I don't have a strong opinion. If someone steps up to do the work and ensure that we don't regress any of the current functionality I doubt that we'd reject the patch :)

Comment 3

[Elmar Ludwig]

Attachment: minimal patch v1

Since this bug is about ")" in particular and not quoting in general (which might be overkill), let's try a minimal fix here.

This patch changes the regexp in nsBrowserContentHandler.js to allow a closing parenthesis within the URL. The remote command from comment #0 works with this patch.

Comment 4

[Elmar Ludwig]

Comment on attachment 292816 [details] [diff] [review]
minimal patch v1

Switching review request to Benjamin Smedberg.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

The only reason I haven't reviewed this is because I haven't had the chance to test it properly. It'd be nice to at least get confirmation that you've tested it with the command lines from from here and bug 298960 comment 34. An automated test that covers those cases would be ideal - you might be able to do something hacky like use the browser-chrome test harness and subscript-load nsBrowserContentHandler, and then call handle() directly with a fake nsICommandLine object.

Comment 6

[Elmar Ludwig]

If you prefer to still review this, feel free to do so. I just though you didn't have the time. Regarding the tests:

I have verified that the command line from this bug (and some variations like opening in a new tab) as well as the ones listed in bug 298960 comment 34 work with this patch, although I'm not sure I understand your remark about the last case in that comment.

Alas, my JS hacking skills are probably not good enough to adapt the existing browser-chrome test harness to create an automated test for this. I could try, but that might take a while...

Comment 7

[Benjamin Smedberg]

Comment on attachment 292816 [details] [diff] [review]
minimal patch v1

I decline to review this without a unit test of some sort... probably an xpcshell one would be sufficient.

Comment 8

[Elmar Ludwig]

I won't have any time for this, so moving to default owner.

Comment 9

[:Gijs (he/him)]

I don't think this way of passing arguments is still a thing. Looks like it got removed in bug 1080319. You can use the various other commandline switches to pass URLs.