298960 - -remote can no longer handle commas or quotes

Status: VERIFIED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Akkana Peck]

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]

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]

Bumping severity to major since -remote is basically no longer usable.

Comment 3

[Akkana Peck]

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]

*** Bug 317255 has been marked as a duplicate of this bug. ***

Comment 5

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

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]]

Attachment: patch

what about this one?

Comment 7

[Benjamin Smedberg]

Comment on attachment 205380 [details] [diff] [review]
patch

nit: new Array(2) should probably just be

var remoteParams = [];

Comment 8

[Wolfgang Rosenauer [:wolfiR]]

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-4, no longer working on Mozilla)]

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

urgs, yes, it's not the index but the length

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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]]

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]

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]

*** Bug 320053 has been marked as a duplicate of this bug. ***

Comment 15

[Mike Schroepfer]

Let's bake a couple more days on the trunk and then consider for 1.8.0.1.

Comment 16

[Daniel Veditz [:dveditz]]

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]]

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]]

checked in in both branches

Comment 19

[Philip Withnall (unavailable)]

*** Bug 336613 has been marked as a duplicate of this bug. ***

Comment 20

[Eike Hein]

-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)]

As per bug #336613, I'm re-opening this. :(

Comment 22

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

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]

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]

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 [:mkmelin]]

All the blocking statuses doesn't seem to have helped here... 

Comment 26

[Daniel Veditz [:dveditz]]

(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 [:mkmelin]]

Removing keywords to reopen on branches (too).

Comment 28

[Magnus Melin [:mkmelin]]

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: [e-mail]]]

Attachment: patch

This patch looks for ,new-tab or ,new-window specifically.

Comment 30

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

Hmm, perhaps I should also strip any leading/trailing whitespace.

Comment 31

[Benjamin Smedberg]

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]]

Gavin:  Please submit a valid patch when you're ready and ask for approval on the branches.  Thanks!

Comment 33

[Daniel Herzog]

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: [e-mail]]]

Attachment: 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: [e-mail]]]

Comment on attachment 250964 [details] [diff] [review]
updated patch

The previous cases were tested on both linux and windows.

Comment 36

[Benjamin Smedberg]

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: [e-mail]]]

mozilla/browser/components/nsBrowserContentHandler.js 	1.32

Comment 38

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

Attachment: for checkin (fix typo)

Comment 39

[Daniel Veditz [:dveditz]]

Can we get this verified on trunk before we approve it for the branches?

Comment 40

[Carsten Book [:Tomcat]]

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 [:mkmelin]]

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: [e-mail]]]

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]]

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: [e-mail]]]

mozilla/browser/components/nsBrowserContentHandler.js 	1.12.2.5.2.4

Comment 45

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

mozilla/browser/components/nsBrowserContentHandler.js 	1.12.2.15

Comment 46

[Tracy Walker [:tracy]]

verified1.8.0.10, verified1.8.1.2, with builds from 20070131