Last Comment Bug 298960 - -remote can no longer handle commas or quotes
: -remote can no longer handle commas or quotes
Status: VERIFIED FIXED
: regression, verified1.8.0.10, verified1.8.1.2
Product: Toolkit
Classification: Components
Component: Startup and Profile System (show other bugs)
: unspecified
: All All
: -- major with 1 vote (vote)
: mozilla1.9alpha1
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
Mentors:
: 317255 320053 336613 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-06-27 17:58 PDT by Akkana Peck
Modified: 2008-07-31 04:30 PDT (History)
14 users (show)
jaymoz: blocking1.8.1.2+
mconnor: blocking‑firefox2+
jaymoz: blocking1.8.0.10+
gavin.sharp: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.12 KB, patch)
2005-12-09 00:37 PST, Wolfgang Rosenauer [:wolfiR]
dveditz: approval1.8.0.1+
dveditz: approval1.8.1+
Details | Diff | Splinter Review
how about not chopping off the last character of the URL? (1.20 KB, patch)
2005-12-12 15:36 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
dveditz: approval1.8.0.1+
dveditz: approval1.8.1+
Details | Diff | Splinter Review
patch (2.73 KB, patch)
2006-12-18 11:27 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
updated patch (3.79 KB, patch)
2007-01-09 09:58 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
benjamin: review+
Details | Diff | Splinter Review
for checkin (fix typo) (3.79 KB, patch)
2007-01-09 11:42 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
jaymoz: approval1.8.1.2+
jaymoz: approval1.8.0.10+
Details | Diff | Splinter Review

Description Akkana Peck 2005-06-27 17:58:16 PDT
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).
Comment 1 Benjamin Smedberg [:bsmedberg] 2005-06-29 10:59:48 PDT
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.
Comment 2 Akkana Peck 2005-06-29 12:11:51 PDT
Bumping severity to major since -remote is basically no longer usable.
Comment 3 Akkana Peck 2005-08-12 12:17:32 PDT
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.
Comment 4 Benjamin Smedberg [:bsmedberg] 2005-11-21 06:03:20 PST
*** Bug 317255 has been marked as a duplicate of this bug. ***
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-11-22 13:47:56 PST
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.
Comment 6 Wolfgang Rosenauer [:wolfiR] 2005-12-09 00:37:18 PST
Created attachment 205380 [details] [diff] [review]
patch

what about this one?
Comment 7 Benjamin Smedberg [:bsmedberg] 2005-12-10 14:18:34 PST
Comment on attachment 205380 [details] [diff] [review]
patch

nit: new Array(2) should probably just be

var remoteParams = [];
Comment 8 Wolfgang Rosenauer [:wolfiR] 2005-12-11 17:41:26 PST
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.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-12-12 15:36:13 PST
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.
Comment 10 Wolfgang Rosenauer [:wolfiR] 2005-12-13 09:06:10 PST
urgs, yes, it's not the index but the length
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-12-13 09:21:31 PST
Comment on attachment 205672 [details] [diff] [review]
how about not chopping off the last character of the URL?

Checked in.
Comment 12 Mike Connor [:mconnor] 2005-12-14 09:29:57 PST
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.
Comment 13 Benjamin Smedberg [:bsmedberg] 2005-12-14 09:31:07 PST
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 James Ross 2005-12-15 05:49:46 PST
*** Bug 320053 has been marked as a duplicate of this bug. ***
Comment 15 Mike Schroepfer 2005-12-19 15:27:50 PST
Let's bake a couple more days on the trunk and then consider for 1.8.0.1.
Comment 16 Daniel Veditz [:dveditz] 2006-01-04 11:42:32 PST
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
Comment 17 Daniel Veditz [:dveditz] 2006-01-04 11:43:16 PST
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
Comment 18 Wolfgang Rosenauer [:wolfiR] 2006-01-04 12:47:19 PST
checked in in both branches
Comment 19 Philip Withnall (unavailable) 2006-05-04 10:14:15 PDT
*** Bug 336613 has been marked as a duplicate of this bug. ***
Comment 20 Eike Hein 2006-05-04 10:34:57 PDT
-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".
Comment 21 Philip Withnall (unavailable) 2006-05-04 10:38:56 PDT
As per bug #336613, I'm re-opening this. :(
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-05-04 16:49:02 PDT
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 testuga20 2006-06-09 08:30:32 PDT
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 Eike Hein 2006-06-09 08:41:51 PDT
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 Magnus Melin 2006-12-06 04:56:08 PST
All the blocking statuses doesn't seem to have helped here... 
Comment 26 Daniel Veditz [:dveditz] 2006-12-16 23:13:59 PST
(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.
Comment 27 Magnus Melin 2006-12-17 07:56:54 PST
Removing keywords to reopen on branches (too).
Comment 28 Magnus Melin 2006-12-17 08:01:31 PST
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.
Comment 29 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-12-18 11:27:22 PST
Created attachment 249020 [details] [diff] [review]
patch

This patch looks for ,new-tab or ,new-window specifically.
Comment 30 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-12-20 08:06:11 PST
Hmm, perhaps I should also strip any leading/trailing whitespace.
Comment 31 Benjamin Smedberg [:bsmedberg] 2006-12-21 12:51:54 PST
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 Jay Patel [:jay] 2006-12-22 15:15:05 PST
Gavin:  Please submit a valid patch when you're ready and ask for approval on the branches.  Thanks!
Comment 33 Daniel Herzog 2007-01-03 14:45:46 PST
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.
Comment 34 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-01-09 09:58:24 PST
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).
Comment 35 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-01-09 10:21:00 PST
Comment on attachment 250964 [details] [diff] [review]
updated patch

The previous cases were tested on both linux and windows.
Comment 36 Benjamin Smedberg [:bsmedberg] 2007-01-09 11:34:01 PST
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!
Comment 37 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-01-09 11:41:32 PST
mozilla/browser/components/nsBrowserContentHandler.js 	1.32
Comment 38 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-01-09 11:42:19 PST
Created attachment 250970 [details] [diff] [review]
for checkin (fix typo)
Comment 39 Daniel Veditz [:dveditz] 2007-01-10 14:49:27 PST
Can we get this verified on trunk before we approve it for the branches?
Comment 40 Carsten Book [:Tomcat] - PTO-back Sept 4th 2007-01-10 16:42:56 PST
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
Comment 41 Magnus Melin 2007-01-14 06:31:42 PST
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?
Comment 42 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-01-14 12:44:11 PST
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 Jay Patel [:jay] 2007-01-16 14:31:28 PST
Comment on attachment 250970 [details] [diff] [review]
for checkin (fix typo)

Approved for both branches, a=jay for drivers.  Please land this asap.  Thanks!
Comment 44 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-01-16 14:40:33 PST
mozilla/browser/components/nsBrowserContentHandler.js 	1.12.2.5.2.4
Comment 45 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-01-16 14:42:58 PST
mozilla/browser/components/nsBrowserContentHandler.js 	1.12.2.15
Comment 46 Tracy Walker [:tracy] 2007-01-31 15:01:44 PST
verified1.8.0.10, verified1.8.1.2, with builds from 20070131

Note You need to log in before you can comment on or make changes to this bug.