389687 - Need to implement nsIContentDispatchChooser

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Shawn Wilsher :sdwilsh]

Bug 385065 added a new interface to ask a user how to handle an external protocol.  This is implemented in XUL and JS in toolkit, but that clearly won't work for Camino.

Comment 1

[Chris Lawson (gone)]

Shawn, would this have caused mailto: links to stop working?

cl

Comment 2

[Samuel Sidler (old account; do not CC)]

I'll answer for him: Yes. mailto: is a protocol and non-http protocols are broken (aiui). (Although I think someone said the ftp: protocol is fine.)

Comment 3

[Shawn Wilsher :sdwilsh]

Well, nsExternalHelperAppService::LoadURI calls nsIHandlerInfo::LaunchWithURI (assuming the prefs are set right), but you should have that...

Comment 4

[Stuart Morgan]

From what I understand so far, I think what we probably want is to have our implementation just show the warning about first-use (which this replaces), and then SetAlwaysAskBeforeHandling(PR_FALSE) and SetPreferredAction(nsIHandlerInfo::useSystemDefault) for the handler so we don't get the dialog again (then LaunchWithURI(uri) to actually deal with it).

Comment 5

[Stuart Morgan]

Attachment: short-term version

I finally had time to look into this more, and what I thought was just finishing up what I had turned into a mess that I wasn't able to quickly disentangle (I'll get into that in a follow-up comment). Since I don't want to leave external protocol handling completely broken for several more weeks if I get busy again, I'd like to just check this in now and leave the bug open for follow-up. This gives us a shell implementation of nsIContentDispatchChooser that does nothing but pass it off to be opened by the system. Basically everything here will be part of the correct solution as well, I just need to get a full implementation of Ask working that does what we want.

Given that the OS handles most protocol security for us, Safari shows no warning, and Firefox is considering having no warning, I'm not concerned about the security of this hack; not ever showing a dialog may be what we decide to do in the long term anyway (this is just not the right way to do it).

Comment 6

[Stuart Morgan]

The issue I'm having with the full solution is that to do anything useful, we need an nsIHandlerService to persist any choices we or the user make about future handling, which means we need to ship nsHandlerService.js

Putting nsHandlerService.js and exthandler.xpt into MacOS/components gives me an error at startup about an include failure for XPCOMUtils.jsm, so I added that to MacOS/modules. Now I get "XPCOMUtils.generateQI is not a function"; given that that's defined in XPCOMUtils.jsm, and that's obviously being found since the error changed, I'm not sure what that means.

Comment 7

[Mark Mentovai]

Comment on attachment 282350 [details] [diff] [review]
short-term version

>+static const char* handlerServiceContractID = "@mozilla.org/uriloader/handler-service;1";

Not used, remove?

>+NS_IMETHODIMP ContentDispatchChooser::Ask(nsIHandlerInfo *aHandler,

This method has no return value!

Comment 8

[Stuart Morgan]

(In reply to comment #7)
> (From update of attachment 282350 [details] [diff] [review])
> >+static const char* handlerServiceContractID = "@mozilla.org/uriloader/handler-service;1";
> 
> Not used, remove?

Sorry, that was from the partially done correct version and I missed while gutting; I'll rip it out.

> >+NS_IMETHODIMP ContentDispatchChooser::Ask(nsIHandlerInfo *aHandler,
> 
> This method has no return value!

Indeed. The semantics are (confusingly to me at first; I expected a method called Ask to give an answer) to ask the user what to do, do it, and push any state about what to do in the future into the handler store directly.

Comment 9

[Stuart Morgan]

Attachment: short-term fix, v2 [landed]

Addresses comments.

Comment 10

[Stuart Morgan]

Comment on attachment 282468 [details] [diff] [review]
short-term fix, v2 [landed]

Landed; leaving open for the real implementation.

Comment 11

[Stuart Morgan]

Attachment: real fix

This takes the approach of showing a warning dialog--not in the sense of a security warning, since there's little value in doing that, but just so that the user won't be surprised that some other application is suddenly launched.

There's another dialog in this code that I'm not actually sure can be shown, but it's there just in case. It's intended for the case where there is no application that can handle the protocol, but for the moment at least we don't get called at all in that case (which is unfortunate, since our dialog for that case is way better than the one core shows).

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> There's another dialog in this code that I'm not actually sure can be shown,
> but it's there just in case. It's intended for the case where there is no
> application that can handle the protocol, but for the moment at least we don't
> get called at all in that case (which is unfortunate, since our dialog for that
> case is way better than the one core shows).
I'm all for helping getting a better dialog in Core if it's needed (after 1.9 obviously).

Comment 13

[Stuart Morgan]

It looks like that's covered by the very unloved bug 259741.

I'm confused though; is it correct behavior that the content chooser implementation is never called just because there's no registered handler on the OS? That seems really wrong, since it prevents a client from, say, allowing a user to pick a web handler for a protocol if they have no local handler.

Comment 14

[Stuart Morgan]

Hm, and somehow Minefield is showing a different dialog there.

Comment 15

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> I'm confused though; is it correct behavior that the content chooser
> implementation is never called just because there's no registered handler on
> the OS? That seems really wrong, since it prevents a client from, say, allowing
> a user to pick a web handler for a protocol if they have no local handler.
Probably - note:  not everything was hooked up with nsIContentDispatchChooser yet.  The goal is to have everything go through it, but that work wasn't completed in time for 1.9 (sucky, I know, but protocol handling got a little better, but this particular case you cited seems to not be fixed).  If that's an easy enough fix, we might be able to get that fixed for 1.9.

Comment 16

[Stuart Morgan]

Is there already a core bug (or bugs) tracking the remaining work, or should I file that as a new bug?

Comment 17

[Shawn Wilsher :sdwilsh]

I am not aware of any bugs, so go ahead a file (worst case, we dupe one to the other).

Comment 18

[Stuart Morgan]

Bug 425971 filed; if that's fixed at some point the "no handler" part of the attached code should Just Work.

Comment 19

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 312492 [details] [diff] [review]
real fix

sr=pink,

we may want to think more about the "NoExternalHandlerText", it seems a little unclear to me.

Comment 20

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 312492 [details] [diff] [review]
real fix

>+/* Protocol Handling */
>+"UnknownContentHandler" = "Unknown Application";

How do we ever get that?

>+"NoExternalHandlerText" = "Camino cannot open “%@” links, and no application that can open “%@” link is installed.";

Maybe "and there are no applications installed that can open “%@” links."?  That gets rid of the passive voice, which I think helps.

Some other observations:

* mailto: and the various usenet protocols don't show warnings (in either Camino or Minefield); why are we privileging them over, say, feed?  They all open external apps, and feed: is far more common than news: these days.  UE-wise, it seems a little odd that some "links that open external apps" cause warnings and others don't.

* Clicking on a feed: link in content bypasses our "make sure it's a conscious choice to use Safari" sheet and causes us to show two different UIs for launching a feed reader.  Can we, in this new nsIContentDispatchChooser world, cause feed: in content to go through our existing UI?  If so (or if we don't yet know it's impossible), can we get a follow-up filed on that?

* help: and afp: seem like they're permanently disabled (I didn't test every protocol that's registered on my Mac; there may be more that are permablocked).  help: used to be dangerous, and I don't really see any reason for exposing it to internet web content given its history, but I'm not sure why afp is blocked.  I assume this is a Core bug, but it's also not compelling enough to me to argue over; I just noticed it during testing.  Is there a blacklist somewhere of these protocols?

* When there is a registered protocol with no application (news on a default Mac OS X install), or if the app is missing (install MT-NW and set it as default, then delete it), Minefield prompts to choose an app, but we claim the protocol is not registered.  Is this also bug 425971?  I initially thought that bug was for truly unregistered protocols (foo:, skype: on my Mac), but now I'm not sure.  Minefield definitely shows two distinct UIs, its equivalent of the "foo is not a registered protocol" sheet for foo/skype, and the "choose an app" window for the usenet ones (both before I had installed a usenet client, and then after I'd deleted MT-NW).

I don't think any of these last four should hold up this bug, so r=ardissone with the string change and either bug comments or follow-up bugs on the rest.

Comment 21

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Test your external protocol links right here!

This is the file I worked up when testing this.

Comment 22

[Stuart Morgan]

- I don't know that the Unknown Application case can ever happen (I would expect not); I just didn't want us to melt if it happened ;)

- I'm definitely not doing anything with mailto:, news:, help:, and afp:, so whatever is happening there is core. I'm not sure where that special-casing lives; we can investigate in a follow-up if we want to change something there.

- We'll have to shift some stuff around, but I don't see any reason we couldn't change feed:, and I think it's a very good idea. Bug 389687 filed.

- I'm not sure how Minefield is distinguishing the two cases where we get the lame protocol error, but I consider both to be part of bug 425971, since I think we should always get called.


Landed with the string change.

Comment 23

[Chris Lawson (gone)]

(In reply to comment #22)
> - We'll have to shift some stuff around, but I don't see any reason we couldn't
> change feed:, and I think it's a very good idea. Bug 389687 filed.

By which I assume you meant bug 426423 ;)

Comment 24

[Shawn Wilsher :sdwilsh]

Note:  the not warning of certain protocols has to do with prefs with the name of "network.protocol-handler.warn-external.[protocol]", where mailto's is "network.protocol-handler.warn-external.mailto".

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #15)
> (In reply to comment #13)
> > I'm confused though; is it correct behavior that the content chooser
> > implementation is never called just because there's no registered handler on
> > the OS? That seems really wrong, since it prevents a client from, say, allowing
> > a user to pick a web handler for a protocol if they have no local handler.
> Probably - note:  not everything was hooked up with nsIContentDispatchChooser
> yet. 

I disagree.  The way it's supposed to work, I think, is that Ask shouldn't be called in the case where there are neither OS-level handlers nor web-handlers registered.

(In reply to comment #20)
> * mailto: and the various usenet protocols don't show warnings (in either
> Camino or Minefield); why are we privileging them over, say, feed?  They all
> open external apps, and feed: is far more common than news: these days. 
> UE-wise, it seems a little odd that some "links that open external apps" cause
> warnings and others don't.

How the prefs are set for the various different apps is in each case an attempt to manually balance security against convenience.  I'd say a pretty good argument could be made to whitelist feed: in the defaults for all toolkit apps; that's probably worth spinning off another bug.

Comment 26

[Stuart Morgan]

My reply to comment 25 is in bug 425971.