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

RESOLVED WORKSFORME

Status

()

defect
RESOLVED WORKSFORME
12 years ago
3 months ago

People

(Reporter: atsukan, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Reporter

Description

12 years ago
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
Reporter

Updated

12 years ago
Summary: firefox -remote 'openURL(<URL>)' doesn't work when <URL> a closing parenthesis ")" → firefox -remote 'openURL(<URL>)' doesn't work when <URL> contains a closing parenthesis ")"

Comment 1

12 years ago
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.
Status: UNCONFIRMED → NEW
Component: General → Startup and Profile System
Ever confirmed: true
QA Contact: general → startup
(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

12 years ago
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.
Assignee: nobody → elmar.ludwig
Status: NEW → ASSIGNED
Attachment #292816 - Flags: review?(gavin.sharp)

Comment 4

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

Switching review request to Benjamin Smedberg.
Attachment #292816 - Flags: review?(gavin.sharp) → review?(benjamin)
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

12 years ago
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

12 years ago
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.
Attachment #292816 - Flags: review?(benjamin)
Product: Firefox → Toolkit

Comment 8

10 years ago
I won't have any time for this, so moving to default owner.
Assignee: elmar.ludwig → nobody
Status: ASSIGNED → NEW

Comment 9

3 months ago

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.

Status: NEW → RESOLVED
Closed: 3 months ago
Depends on: 1080319
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.