226071 - xremote: openURL doesn't work well when multiple apps with different capabilities are present

Status: RESOLVED FIXED

(Core Graveyard :: X-remote, defect)

Description

[Darin Fisher]

openURL doesn't work well when multiple apps with different capabilities are
present.  for example, it is currently impossible to use xremote to cause a link
click in thunderbird to load the HREF in firebird.  there are a couple parts to
this problem:

  (1) XRemoteClient only talks to the first "mozilla" window it finds.  however,
      the first window may not be able to handle the given URL.

  (2) XRemoteService assumes it can handle any URL.  this means that a new 
      window might be opened or raised before it is discovered that the given 
      URL cannot be handled internally and would need to be deferred to an 
      external protocol handler.  this is undesirable because the application 
      would appear to have partially responded to the xremote command.

solutions to these problems:

  (1) XRemoteClient should enumerate the list of "mozilla" windows until it 
      finds one that accepts the given URL.  it should only give up after it has 
      exhausted the list.

  (2) XRemoteServer should reject URLs that cannot be handled internally 
      up-front.  no special status code is needed, though it might be nice to
      create one.  it should be enough to just report a general failure and let
      the XRemoteClient query the next "mozilla" window.

now, the bit about determining which URLs can be handled internally and which
should be handled externally gets interesting.  at the moment there is a hack in
nsWebShell::OnLinkClickSync, which attempts to implement filtering along these
lines.  it needs to be factored out into something other than an #ifdef
obviously, and i think it probably makes sense to use preferences.

overall there are really 3 places in the codebase where we need to care about
external protocols:

  (1) nsDocShell::InternalLoad needs to be able to load a first-party URL using
      the system registry if no internal protocol handler exists.  (note: it
      only makes sense to invoke external protocol handlers for first-party
      loads because first-party loads are by definition loads directly initiated 
      by the user.)

  (2) mozTXTtoHTML needs to know if an external protocol handler exists for
      use during linkification of text/plain documents.

  (3) and as i outlined above, it would be beneficial for XRemoteService to
      also know about external protocol handlers.

i think the way we currently load external protocols as a fallback in
nsIOService is not ideal.  it makes sense for nsIOService::NewURI to always
return a nsIURI (even if no registered nsIProtocolHandler exists), but it seems
overkill to go the route of implementing a nsIChannel for external loads because
there is no way to really implement nsIChannel.  all we really need is to invoke
LoadURI in nsDocShell::InternalLoad when the load is first-party and the URI
scheme is of a type that should be and can be handled by an external protocol
handler.

so, what i've done is to rework nsIExternalProtocolHandler so that there are now
the following three methods:

  boolean shouldLoadExternally(in string scheme);
  boolean canLoadExternally(in string scheme);
  void loadURI(in nsIURI uri);

the first returns true if the URI scheme should be loaded using an external
protocol handler (there is a pref that can be set to override the behavior of
this method: network.protocol-handler.external.{scheme}).  this applies only in
the case where the URI appears as a first-party load.  the second function
returns true if an external protocol handler exists.  this is equivalent to
today's ExternalProtocolHandlerExists method.  and the third function is
equivalent to today's LoadUrl method.  i just fixed up the name ;)

also, this interface makes nsIExternalProtocolService unnecessary.

Comment 1

[Darin Fisher]

Attachment: v1.0 patch

Comment 2

[Lorenzo Colitti]

Is this ready for testing? Currently I'm using the solution described in bug
216252 comment 31 to open links in firebird from thunderbird, but it's a hack.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hrmm... That boolean is not that useful, iirc....  Some things to check:

1)  What is its value when we are loading a chrome document (eg navigator.xul)?
2)  What is its value when we are processing a <meta> refresh?
3)  When is it false, in general (I see some SHistory code that sets it false for
    some reason, for example...)

Nice to have undocumented vars like this on interfaces.  ;)

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

> That boolean is not that useful, iirc

which boolean are you talking about?

Comment 5

[Darin Fisher]

lorenzo: yeah, the patch is ready for testing.  the patch does not include the
corresponding pref changes to thunderbird.  you'd want to set
network.protocol-handler.external.http in your thunderbird preferences.

boris: yeah, those are good questions.

>1)  What is its value when we are loading a chrome document (eg navigator.xul)?

doesnt matter does it?  chrome URLs are never going to be marked external.  one
issue is probably URLs like jar:http://, since you'd want those to follow the
rules for http:// and not jar://.  maybe we should add some special case
handling for jar URLs.  maybe this means the API should take a full URL instead
of only a URL scheme.  hmm..

>2)  What is its value when we are processing a <meta> refresh?

looks like first-party is always true while processing a meta refresh.  that's
wrong.  it should inherit the value from its predecessor.  

>3)  When is it false, in general (I see some SHistory code that sets it false
>for some reason, for example...)

well, consider inline frames.  first-party is set to false while loading frames,
which is desirable in this case.

so for the most part, i think the first-party flag tells us what we want to
know.  it isn't perfect, but i think it will work out well in most cases... and
in other cases, like the meta refresh case, i think we can with a little effort
make it work.  of course, if you have a better suggestion, i'd be more than
happy to consider it! ;-)

Comment 6

[Boris Zbarsky [:bzbarsky]]

biesi, the "first party" boolean.

> doesnt matter does it?

Tbird had chrome:// marked external initially... Also, the mailnews start page
is an http:// page; does the load of that have aFirstParty true or false?

> first-party is set to false while loading frames, which is desirable in this
> case.

It is?  Where is that code?

One problem I see is that there is no scriptable way to do a "non-first-party"
load...  This seems like the right approach, but we need to evaluate all
loadURI/InternalLoad callers and see what they are doing...

Comment 7

[Darin Fisher]

>> doesnt matter does it?
>
>Tbird had chrome:// marked external initially... Also, the mailnews start page
>is an http:// page; does the load of that have aFirstParty true or false?

it doesn't make any sense to mark a chrome:// URL as external.  today that would
prevent anything from working.  the mailnews start page appears to load just
fine when HTTP is marked to be loaded externally.


>> first-party is set to false while loading frames, which is desirable in this
>> case.
>
>It is?  Where is that code?

content/base/src/nsFrameLoader.cpp (see the LoadFrame method)


>One problem I see is that there is no scriptable way to do a "non-first-party"
>load...  

hmm... true.  though we could fix that if we really wanted to (for privileged
scripts anyways).


>This seems like the right approach, but we need to evaluate all
>loadURI/InternalLoad callers and see what they are doing...

agreed.

Comment 8

[Darin Fisher]

Attachment: v2.0 patch

i thought about this patch a bunch today, and eventually i came to the
conclusion that we can get things working well with far fewer changes.	i still
believe there are some good changes in the v1.0 patch, but some of those
changes concern me and they are not necessary to solve this bug.

i've come up with something much simpler and far less risky.  i'm hopeful that
this is something we can do for 1.6.  this patch makes very minimal changes to
the codebase.  what i've done is to add this method to
nsIExternalProtocolService:

  boolean isExposedProtocol(in string aScheme);

this method can be used to query whether or not a protocol handler should be
"exposed" ... meaning whether or not the protocol handler should be invoked via
xremote (or equivalent) and should be processed by the app in response to a
link click.  this probably isn't the best verb, but i think it works. 
suggestions welcome.  preferences are used to control which protocols are
exposed.  there is a blanket pref that can be used to force all protocols to be
exposed or no protocols to be exposed.	otherwise, per scheme prefs can be set
to tweak the result of IsExposedProtocol.

i have backed out all my changes to necko.

i replaced the #ifdef MOZ_THUNDERBIRD in nsWebShell::OnLinkClickSync with a
call to nsIExternalProtocolService::IsExposedProtocol.	it calls
nsIExternalProtocolHandler::LoadUrl if the protocol is not exposed.  this
handles link clicks in exactly the same way that they are handled today in
thunderbird.

the Xremote changes are virtually identical to the v1.0 patch.	the only change
is in XRemoteService, where it now calls IsExposedProtocol instead of the other
foo to determine whether or not to reject the openURL command.

finally, i needed to tweak mailWindow.js to make this work well in seamonkey
mail when a built-in protocol is configured to not be exposed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I'd prefer the other approach in the long run, I think....

Comment 10

[Darin Fisher]

one more point:  the v1.0 patch breaks support for external protocols being
loaded via a HTTP redirect.  that was actually being used by some embedding apps
in the past (ok, AOL/Gecko... but still!!).  i don't think we should regress
this feature just yet.  simpler is better at this point so i prefer the v2.0 patch.

Comment 11

[Darin Fisher]

bz: i agree that there are parts of the v1 patch that i like... and i would like
to find a way to incorporate them in the future.  but, with the uncertainty
there, i think we would need to punt the v1 changes to 1.7 :(  i think we can do
this incrementally.

Comment 12

[Boris Zbarsky [:bzbarsky]]

For 1.6, sure.

Http redirects are not first-party, are they?  Docshell doesn't even really know
they happen.  <meta> refresh is a different issue, but should probably be
treated as http redirects are....

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135875 [details] [diff] [review]
v2.0 patch

>Index: docshell/base/nsWebShell.cpp

> #include "nsCExternalHandlerService.h"

We need a nsDocShellCID.h.... ;)

>+  PRBool earlyReturn = PR_FALSE;

Why do we need this?  Why not just return from the one spot where you set it to
true?

>Index: xpfe/components/xremote/src/XRemoteService.cpp

>+#include <nsCExternalHandlerService.h>
>+#include <nsIExternalProtocolService.h>

I guess that's supposed to pick up the system Mozilla includes?  Or what?

I have to run.	I may not get to the rest of this tonight.  If so, I won't get
to it tomorrow either.	:(

Comment 14

[Darin Fisher]

>> #include "nsCExternalHandlerService.h"
>
>We need a nsDocShellCID.h.... ;)

yeah, that would be nice.


>>+  PRBool earlyReturn = PR_FALSE;
>
>Why do we need this?  Why not just return from the one spot where you set it to
>true?

code/sighs/.. conditional early return from an inner scope with comptrs equals
bloat.  maybe my solution is overkill :-/


>>Index: xpfe/components/xremote/src/XRemoteService.cpp
>
>>+#include <nsCExternalHandlerService.h>
>>+#include <nsIExternalProtocolService.h>
>
>I guess that's supposed to pick up the system Mozilla includes?  Or what?

yeah, i was just following convention in that file.  the angle brackets are
unusual for mozilla code.


>I have to run.	I may not get to the rest of this tonight.  If so, I won't get
>to it tomorrow either.	:(

well, i'll keep my fingers crossed.. i would really like to get this in before
the beta freeze if possible :)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135875 [details] [diff] [review]
v2.0 patch


>Index: xpfe/components/xremote/src/XRemoteService.cpp
>+XRemoteService::ShouldNotOpenURL(const nsCString &aURL)

I'd flip the booleans and call this MayOpenURL.

>+    if (NS_SUCCEEDED(rv))
>+      *aResult = val;
>+    else

Curly braces around the if clause there, please (since you have them around the
else).

r=bzbarsky with those, I guess -- the rest of the code looks reasonable and I'm
still hoping we will do the other approach for 1.7...

Comment 16

[Brian Ryner (not reading)]

Comment on attachment 135875 [details] [diff] [review]
v2.0 patch

>Index: calendar/sunbird/app/profile/all.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/sunbird/app/profile/all.js,v
>retrieving revision 1.3
>diff -p -u -1 -0 -r1.3 all.js
>--- calendar/sunbird/app/profile/all.js	6 Nov 2003 19:35:22 -0000	1.3
>+++ calendar/sunbird/app/profile/all.js	19 Nov 2003 02:42:43 -0000
>@@ -628,16 +628,21 @@ pref("network.autodial-helper.enabled", 
> // and we're not already running).
> pref("advanced.system.supportDDEExec", true);
> pref("browser.xul.error_pages.enabled", false);
> 
> pref("signon.rememberSignons",              true);
> pref("signon.expireMasterPassword",         false);
> 
> pref("network.protocol-handler.external.mailto", true); // for mail
> pref("network.protocol-handler.external.news" , true); // for news 
> 
>+// By default, all protocol handlers are hidden.  This means
>+// that composer will not respond to X-remote openURL commands

s/composer/calendar/

Comment 17

[Darin Fisher]

Comment on attachment 135875 [details] [diff] [review]
v2.0 patch

would really like to get this in for 1.6 beta since it hopefully fixes the
thunderbird link click problem on Linux/Unix platforms.

Comment 18

[Lorenzo Colitti]

This patch wouldn't compile for me:

XRemoteService.cpp:50: nsCExternalHandlerService.h: No such file or directory
XRemoteService.cpp:51: nsIExternalProtocolService.h: No such file or directory
<etc>

Adding "exthandler" to the REQUIRES line in the Makefile in
mozilla/xpfe/components/xremote/src solved the problem.

This could just be my tree though.

Comment 19

[Christopher Blizzard (:blizzard)]

When rewriting the X part of this code, bryner and I had talked about adding
some cached information on the window so that you didn't have to ask Mozilla
about various mime types.  Querying can be slow, at least with xremote-based
startup the X communication represents a lot of that startup time.

So, in order to help that out, is there a way to enumerate the protocols that
are supported in Mozilla?  Maybe in a way we could expose as part of the protocol?

Comment 20

[Darin Fisher]

lorenzo: yeah, i have that change in my tree too... i just forgot to include it
in the patch.

blizzard: i agree that it would be nice to improve the xremote protocol at some
point.  you could enumerate the pref branch (using nsIPrefBranch::getChildList)
and use that to construct a list of exposed protocols.

Comment 21

[Darin Fisher]

Attachment: v2.1 patch

revised patch per comments from bz, bryner, and blizzard (over irc).

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 136008 [details] [diff] [review]
v2.1 patch

I'd be uncomfortable taking this after beta. We have about a week 'till beta so
I say better sooner rather than later. a=asa (on behalf of drivers) for checkin
to Mozilla 1.6 beta.

Comment 23

[Darin Fisher]

patch checked in.  thanks to bryner for landing the mail/ changes for me.

Comment 24

[Darin Fisher]

i filed bug 226374 as a follow-up with regards to incorporating some of the
goodness from the v1 patch that didn't make it into the v2 patch.  that bug is
just about further changes to necko, docshell, and exthandler.  further xremote
enhancements are another subject altogether ;-)

Comment 25

[André Dahlqvist]

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

Comment 26

[Mickey Stein]

Is there some new additions to the .mozconfig files (or to user.js or anything
of that sort) to enable the feature of having tbird URL's popup in firebird?
I've built both tb/fb a couple times since this checkin without having any sign
of anything happening when I click on a URL in an email. 

Thanks. 

Comment 27

[[e-mail]]

Comment on attachment 136008 [details] [diff] [review]
v2.1 patch

>-#ifdef MOZ_THUNDERBIRD
>-  // XXX ugly thunderbird hack to force all url clicks to go to the system default app
>-  // I promise this will be removed once we figure out a better way. 
Nice ;-)

Comment 28

[Darin Fisher]

Mickey,

This patch alone does not make everything work.  You still need to make your
system launch Firebird as its default browser, and that unfortunately is not yet
trivial to do.  You have a variety of options, but basically you need to create
a shell script like this:

  #!/bin/sh
  export MOZILLA_FIVE_HOME=/path/to/MozillaFirebird
  export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$MOZILLA_FIVE_HOME
  url=$1
  [ -z $url ] && url=about:blank
  $MOZILLA_FIVE_HOME/mozilla-xremote-client openURL\($url\) && exit 0
  exec $MOZILLA_FIVE_HOME/MozillaFirebird $url

and then you need to set this shell script to be executed as the default URL
handler under GNOME2.  or you can set preferences in thunderbird to execute this
shell script for certain URL types, like http://.

the preference to set (in user.js) is:

user_pref("network.protocol-handler.app.http", "/path/to/that_shell_script");

you'd want to do this for http, https, ftp, and whatever other URL types you
want to have opened by firebird.  you only need to configure these prefs in your
thunderbird profile.

you should be able to use a similar trick to make URLs like news:// load in
thunderbird when clicked on in the browser.

we have plans to make this part easier... ;-)

Comment 29

[Darin Fisher]

Attachment: mozfb_open.sh -- sample shell script for launching firebird

Comment 30

[Scott MacGregor]

thanks Darin. I'm going to turn your notes into a little document users can use
as a resource until we reach the next step.

Comment 31

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Yes, thanks Darin. same problem in Standalone Composer of course...

Comment 32

[Scott MacGregor]

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

Comment 33

[mozilla3eran]

The scripts in comment 28, comment 29 and
http://www.mozilla.org/projects/thunderbird/linuxurls.html
are all missing doublequotes around occurances of $url. Recall that bash
performs some expansions after parameter expansion.

Comment 34

[Mickey Stein]

Darin, thanks for #28. Both are working well now. I'd taken that as a starting
point awhile back and come up with this one for fb:
#!/bin/bash
export MOZILLA_FIVE_HOME=/usr/local/MozillaFirebird
export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$MOZILLA_FIVE_HOME
url=$1
[ -z $url ] && url=about:blank
## Run Firebird remotely if there's an existing process running...
if [ -x $MOZILLA_FIVE_HOME/MozillaFirebird ] ; then
    rv=`ps -ef | grep '/usr/local/MozillaFirebird' | grep -v grep | wc -l`
    if [ $rv  == "0" ] ; then
        $MOZILLA_FIVE_HOME/mozilla-xremote-client openURL\($url\) && exit 0
        $MOZILLA_FIVE_HOME/MozillaFirebird $url &
    else
        $MOZILLA_FIVE_HOME/mozilla-xremote-client openURL\($url,new-tab\) && exi
t 0
        $MOZILLA_FIVE_HOME/MozillaFirebird -remote "openURL($url, new-tab)" &
    fi
fi

and this one for mailto on tb:

#!/bin/bash
export MOZILLA_FIVE_HOME=/usr/local/thunderbird
export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$MOZILLA_FIVE_HOME
echo
echo $MOZILLA_FIVE_HOME
echo
addr=$1
[ -z $addr ]

rv=1;
#if [ -x $MOZILLA_FIVE_HOME/thunderbird ]
#then
rv=`ps -ef | grep '/usr/local/thunderbird' | grep -v grep | wc -l`

if [ $rv == 2 ]
then
    $MOZILLA_FIVE_HOME/mozilla-xremote-client mailto\($addr\) && exit 0
    exec $MOZILLA_FIVE_HOME/thunderbird -remote "($addr)" &
fi

if [ $rv == 0 ]
then
    $MOZILLA_FIVE_HOME/mozilla-xremote-client mailto\($addr\) && exit 0
    exec $MOZILLA_FIVE_HOME/thunderbird -compose "mailto:$addr" &
fi

Comment 35

[Kevin Teuscher]

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

Comment 36

[Kevin Teuscher]

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

Comment 37

[Kevin Teuscher]

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

Comment 38

[Kevin Teuscher]

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

Comment 39

[Boris Zbarsky [:bzbarsky]]

I just realized that the code we added here never sets the aDocShell and aRequest out param values.  :(