-remote can no longer handle commas or quotes

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Toolkit
Startup and Profile System
--
major
VERIFIED FIXED
12 years ago
9 years ago

People

(Reporter: Akkana Peck, Assigned: Gavin)

Tracking

({regression, verified1.8.0.10, verified1.8.1.2})

unspecified
mozilla1.9alpha1
regression, verified1.8.0.10, verified1.8.1.2
Points:
---
Bug Flags:
blocking1.8.1.2 +
blocking-firefox2 +
blocking1.8.0.10 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

12 years ago
Bug 280725 may have broken the -remote protocol in a couple of ways. It can no
longer handle urls with commas, nor with quotes.

Sample commandlines which used to work and don't any more:
firefox -remote
'openURL(http://www.news.com.au/story/0,10117,15739502-13762,00.html)'

firefox -remote
'openURL("http://www.news.com.au/story/0,10117,15739502-13762,00.html")'

(The latter is important for distinguishing urls with commas from arguments like
,new-tab).
The parsing code is now contained at
http://lxr.mozilla.org/mozilla/source/browser/components/nsBrowserContentHandler.js#165

I welcome attempts to make this parsing code match the original behavior.
Assignee: benjamin → nobody
(Reporter)

Comment 2

12 years ago
Bumping severity to major since -remote is basically no longer usable.
Severity: normal → major
(Reporter)

Comment 3

12 years ago
It turns out that firefox [url] will automatically call the currently running
mozilla. But if this isn't going to be fixed, it should be release noted so that
people who use it will know to change their configurations.

Filed bug 304477 on the release note issue, and bug 304469 on the request for a
-new-tab flag.
*** Bug 317255 has been marked as a duplicate of this bug. ***
I guess http://www.mozilla.org/unix/remote.html is the closest thing to a spec that exists? It doesn't say how URLs with commas should be handled, I guess that means "they must be quoted". -remote is now dealt with per-app, so this should probably be a Firefox bug.
Created attachment 205380 [details] [diff] [review]
patch

what about this one?
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #205380 - Flags: first-review?(benjamin)
Comment on attachment 205380 [details] [diff] [review]
patch

nit: new Array(2) should probably just be

var remoteParams = [];
Attachment #205380 - Flags: first-review?(benjamin) → first-review+
patch applied for trunk with review comments
Checking in nsBrowserContentHandler.js;
/cvsroot/mozilla/browser/components/nsBrowserContentHandler.js,v  <--  nsBrowserContentHandler.js
new revision: 1.19; previous revision: 1.18
done

This still has to checked in to the 1.8 branch if open again for Firefox 2 development.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Flags: blocking-aviary2?
Resolution: --- → FIXED
Created attachment 205672 [details] [diff] [review]
how about not chopping off the last character of the URL?

When there is a comma, this was chopping off the last character of the URL.
Attachment #205672 - Flags: first-review?(benjamin)
urgs, yes, it's not the index but the length
Attachment #205672 - Flags: first-review?(benjamin) → first-review+
Comment on attachment 205672 [details] [diff] [review]
how about not chopping off the last character of the URL?

Checked in.
Flags: blocking1.8.1?
Flags: blocking1.8.0.1?
We want this for Fx2 at least, which is the same as 1.8.1 (we need to sort out those flags, yes).  I don't think this is in scope for 1.8.0.x, but I'm not directly involved in that process.
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking-aviary2?
Flags: blocking-aviary2+
I think that this is in scope for 1.5.0.x, especially since it is a small fix that the linux distros really want.

Comment 14

12 years ago
*** Bug 320053 has been marked as a duplicate of this bug. ***
Attachment #205380 - Flags: approval1.8.1?
Attachment #205380 - Flags: approval1.8.0.1?
Attachment #205672 - Flags: approval1.8.1?
Attachment #205672 - Flags: approval1.8.0.1?

Comment 15

12 years ago
Let's bake a couple more days on the trunk and then consider for 1.8.0.1.
Flags: blocking1.8.0.1? → blocking1.8.0.1+
Comment on attachment 205380 [details] [diff] [review]
patch

a=dveditz for drivers, please add fixed1.8.0.1 and fixed1.8.1 keywords when checked in
Attachment #205380 - Flags: approval1.8.1?
Attachment #205380 - Flags: approval1.8.1+
Attachment #205380 - Flags: approval1.8.0.1?
Attachment #205380 - Flags: approval1.8.0.1+
Comment on attachment 205672 [details] [diff] [review]
how about not chopping off the last character of the URL?

a=dveditz for drivers.
please add fixed1.8.0.1 and fixed1.8.1 keywords when checked in
Attachment #205672 - Flags: approval1.8.1?
Attachment #205672 - Flags: approval1.8.1+
Attachment #205672 - Flags: approval1.8.0.1?
Attachment #205672 - Flags: approval1.8.0.1+
checked in in both branches
Keywords: fixed1.8.0.1, fixed1.8.1
*** Bug 336613 has been marked as a duplicate of this bug. ***

Comment 20

11 years ago
-remote is still choking on URLs generated by the Vignette CMS in 1.5.0.3, i.e. "http://www.spiegel.de/auto/fahrberichte/0,1518,414380,00.html" is opened as "http://www.spiegel.de/auto/fahrberichte/0,1518,414380".
As per bug #336613, I'm re-opening this. :(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I guess it would be possible to check whether remoteParams[1] is equal to one of the parameters we support (new-window, new-tab), and append it to remoteParams[0] if it isn't, but that could theoretically break people that rely on us doing nothing for unsupported parameters. That's probably not something worth worrying about.

Comment 23

11 years ago
in 1.5.0.4, -remote openurl always opens a new tab by default.
i.e no additional tab argument is given. 

from the "spec"

1. openURL(URL) and openFile(URL)
Opens the specified document without prompting.

2. openURL(URL,new-tab)
    Creates a new tab displaying the specified document. (Available in 1.0.1, 1.1 and beyond) 

1. always behaves like 2 now. The script I use to keep the browser open from ssl
timeout now opens a new tab each refresh, and is not useful anymore. 

Comment 24

11 years ago
Meanwhile, the comma problem persists. Having to patch nsBrowserContentHandler.js on every new Firefox install on a Linux box is getting a bit old.

Comment 25

11 years ago
All the blocking statuses doesn't seem to have helped here... 
Flags: blocking1.8.1.1?
(In reply to comment #25)
> All the blocking statuses doesn't seem to have helped here... 

That's because it's marked "fixed" for the branches in the keyword field. As far as anyone checking the blocking flags knows this one is complete. If it's not actually fixed then those keywords shouldn't be there.

1.8.1.1 is done and tagged, moving blocking request to next release. But they're likely not to block if it's "already fixed" so please make the bug details match reality.

(In reply to comment #24)
> Having to patch nsBrowserContentHandler.js on every new Firefox install
> on a Linux box is getting a bit old.

Are you applying the patches in the bug, or a different one? If the ones in the bug, are you saying they didn't land even though the "fixed" keywords were added? If it's a different patch that works please attach it to this bug, that'll help make forward progress here.
Flags: blocking1.8.1.1? → blocking1.8.1.2?

Comment 27

11 years ago
Removing keywords to reopen on branches (too).
Keywords: fixed1.8.0.1, fixed1.8.1

Comment 28

11 years ago
Oh, and I don't know what bug details I would change. The checked in "fix" probably did something good, but the bug as stated in comment 0 is certainly still present, both cases.
Created attachment 249020 [details] [diff] [review]
patch

This patch looks for ,new-tab or ,new-window specifically.
Assignee: mozilla → gavin.sharp
Attachment #205380 - Attachment is obsolete: true
Attachment #205672 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #249020 - Flags: first-review?(benjamin)
Hmm, perhaps I should also strip any leading/trailing whitespace.
Attachment #249020 - Attachment is obsolete: true
Attachment #249020 - Flags: first-review?(benjamin)
Comment on attachment 249020 [details] [diff] [review]
patch

>Index: browser/components/nsBrowserContentHandler.js

>       catch (e) {
>+        dump(e);

Please make this Components.utils.reportError() instead of dump()

Comment 32

11 years ago
Gavin:  Please submit a valid patch when you're ready and ask for approval on the branches.  Thanks!
Flags: blocking1.8.1.2?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10+
Flags: blocking1.8.0.1+

Comment 33

11 years ago
I'd love to see this fixed, dozens of news pages are affected by this and links which for instance are opened from within IRC just wont work which is really annoying.
Created attachment 250964 [details] [diff] [review]
updated patch

I've tested the following cases with this patch:
openURL(http://www.news.com.au/,new-tab)
openURL(http://www.news.com.au/,new-window)
openURL(http://www.news.com.au/)
openURL(http://www.news.com.au/story/0,23599,20950090-2,00.html)
openURL(http://www.news.com.au/story/0,23599,20950090-2,00.html,new-tab)
openURL(http://www.news.com.au/story/0,23599,20950090-2,00.html,new-window)
openURL(http://www.news.com.au/, new-window)

The last case (invalid space before remote target) is the reason for the addition of the |if (a)| check and the the change to the thrown exception (so that the reportError() call shows something useful rather than the numeric value of NS_ERROR_ABORT).
Attachment #250964 - Flags: first-review?(benjamin)
Assignee: gavin.sharp → nobody
Status: ASSIGNED → NEW
Component: XRE Startup → Startup and Profile System
Flags: first-review?(benjamin)
Flags: first-review+
OS: Linux → All
Product: Toolkit → Firefox
QA Contact: nobody → startup
Hardware: PC → All
Assignee: nobody → gavin.sharp
Comment on attachment 250964 [details] [diff] [review]
updated patch

The previous cases were tested on both linux and windows.
Attachment #250964 - Flags: review?(benjamin)
Comment on attachment 250964 [details] [diff] [review]
updated patch

This is browser/ not toolkit/ so unit tests are not required, but they sure would be nice!
Attachment #250964 - Flags: review?(benjamin) → review+
mozilla/browser/components/nsBrowserContentHandler.js 	1.32
Status: NEW → RESOLVED
Last Resolved: 12 years ago11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Created attachment 250970 [details] [diff] [review]
for checkin (fix typo)
Attachment #250964 - Attachment is obsolete: true
Attachment #250970 - Flags: approval1.8.1.2?
Can we get this verified on trunk before we approve it for the branches?
Keywords: qawanted
Verified fixed on trunk with  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a2pre) Gecko/2007011015 Minefield/3.0a2pre on Linux 

Setting OS -> Linux because  -remote seems only to work on Linux/Unix Plattforms
Status: RESOLVED → VERIFIED
OS: All → Linux

Comment 41

11 years ago
Commas seems to be working now, thanks Gavin! The quotes case from comment 0 still fails however. E.g. 
firefox -remote 'openURL("http://www.news.com.au/story/0,23599,20950090-2,00.html")'

All I get is: "Error: Failed to send command: 500 command not parseable"

Is it supposed to work?
Attachment #250970 - Flags: approval1.8.0.10?
IIRC the quoted version didn't work in 1.5 either, so I'm not really that worried about it, unless there's evidence that it's needed for some reason.

Comment 43

11 years ago
Comment on attachment 250970 [details] [diff] [review]
for checkin (fix typo)

Approved for both branches, a=jay for drivers.  Please land this asap.  Thanks!
Attachment #250970 - Flags: approval1.8.1.2?
Attachment #250970 - Flags: approval1.8.1.2+
Attachment #250970 - Flags: approval1.8.0.10?
Attachment #250970 - Flags: approval1.8.0.10+
mozilla/browser/components/nsBrowserContentHandler.js 	1.12.2.5.2.4
Keywords: qawanted → fixed1.8.0.10
OS: Linux → All
Target Milestone: --- → Firefox 3 alpha1
mozilla/browser/components/nsBrowserContentHandler.js 	1.12.2.15
Keywords: fixed1.8.1.2
verified1.8.0.10, verified1.8.1.2, with builds from 20070131
Keywords: fixed1.8.0.10, fixed1.8.1.2 → verified1.8.0.10, verified1.8.1.2
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.