Closed Bug 298960 Opened 19 years ago Closed 18 years ago

-remote can no longer handle commas or quotes

Categories

(Toolkit :: Startup and Profile System, defect)

Product:
Toolkit
Component:
Startup and Profile System
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: akkzilla, Assigned: Gavin)

References

Details

(Keywords: regression, verified1.8.0.10, verified1.8.1.2)

Attachments

(1 file, 4 obsolete files)

for checkin (fix typo)
3.79 KB, patch
jay
: approval1.8.1.2+
jay
: approval1.8.0.10+
Details | Diff | Splinter Review 
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
Bumping severity to major since -remote is basically no longer usable.
Severity: normal → major
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.
Attached patch patch (obsolete) — — Splinter Review 
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
Closed: 19 years ago
Flags: blocking-aviary2?
Resolution: --- → FIXED
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.
*** 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?
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. ***
-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.
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. 
Meanwhile, the comma problem persists. Having to patch nsBrowserContentHandler.js on every new Firefox install on a Linux box is getting a bit old.
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?
Removing keywords to reopen on branches (too).
Keywords: fixed1.8.0.1, fixed1.8.1
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.
Attached patch patch (obsolete) — — Splinter Review 
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()
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+
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.
Attached patch updated patch (obsolete) — — Splinter Review 
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
Closed: 19 years ago → 18 years ago
Flags: in-testsuite?
Resolution: --- → FIXED

        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

    
    

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