Closed Bug 1297686 Opened 8 years ago Closed 6 years ago

When multiple desktop files support the same protocol scheme, only one of them is listed

Categories

(Firefox :: File Handling, defect, P3)

Product:
Firefox
Component:
File Handling
Version:
51 Branch
Platform:
All
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: bugs, Assigned: jhorak)

Details

(Whiteboard: tpi:+)

Attachments

(2 files, 1 obsolete file)

list-all-scheme-handlers.patch
18.38 KB, patch
jst
: feedback+
bzbarsky
: feedback+
Details | Diff | Splinter Review
Bug 1297686 - List GIO handlers in protocol handler list in handlers dialog and in preferences,
59 bytes, text/x-review-board-request
Paolo
: review+
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160823072522

Steps to reproduce:

Install multiple programs handling a single URI scheme, for example, Gajim and Pidgin from hg.
Click on an xmpp: link, for example xmpp:test@muc.linkmauve.fr?join


Actual results:

Only a single program is listed in the “Open with…” dialog, here Gajim, but it seems non-deterministic.


Expected results:

Both programs should be listed, and the user should be able to select the one they prefer.
OS: Unspecified → Linux
Hardware: Unspecified → All
In my /usr/share/applications/mimeinfo.cache I have “x-scheme-handler/xmpp=gajim-remote.desktop;purple-url-handler.desktop;” to list all of the desktop files supporting this URI scheme, as generated by update-desktop-database.

From Python when I query this database I correctly get both desktop files:
>>> from gi.repository import Gio
>>> print(Gio.app_info_get_all_for_type("x-scheme-handler/xmpp"))'
[<Gio.DesktopAppInfo object at 0x7f69191a2dc8 (GDesktopAppInfo at 0x840820)>, <Gio.DesktopAppInfo object at 0x7f69191a2e10 (GDesktopAppInfo at 0x8408f0)>]
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0

I have tested this issue on Ubuntu 16.04 x64 and Windows 10 x64 with the latest Firefox release (48.0.2) and the latest Nightly (51.0a1-20160828030602) and managed to reproduce it.
After installing Gajim and Pidgin when clicking on xmpp:test@muc.linkmauve.fr?join, only a single program is listed in the “Open with…” dialog.
Status: UNCONFIRMED → NEW
Component: Untriaged → Networking
Ever confirmed: true
Product: Firefox → Core
This doesn't seem like a networking bug.
Component: Networking → Untriaged
Component: Untriaged → Widget
Pretty sure this is in the code under uriloader, but I guess we don't have a component for that. Moving this into widget->gdk, seems like the next best classification.
Component: Widget → Widget: Gtk
Priority: -- → P3
Whiteboard: tpi:+
Yes, Core Graveyard: File Handling would be the correct component, but not much point moving it there.
Component: Widget: Gtk → File Handling
Product: Core → Firefox
Implementation of possible application handlers by using GAppInfo [1].
What I'm really curious about is implementation of nsMIMEInfoUnix::GetPossibleApplicationHandlers in the patch. This could probably be done better (somewhere else). Could you please check it an help me out there? 

[1] https://www.freedesktop.org/software/gstreamer-sdk/data/docs/2012.5/gio/GAppInfo.html
Assignee: nobody → jhorak
Attachment #8805097 - Flags: feedback?(cbiesinger)
Comment on attachment 8805097 [details] [diff] [review]
list-all-scheme-handlers.patch

Sorry, I'm not really involved with Mozilla anymore. Please find someone else -- I'm  not sure who handles this code these days.
Attachment #8805097 - Flags: feedback?(cbiesinger)
Comment on attachment 8805097 [details] [diff] [review]
list-all-scheme-handlers.patch

Trying other person from: https://wiki.mozilla.org/Modules/All#docshell Please have a look, or try to point me to the right reviewer.
Attachment #8805097 - Flags: feedback?(jst)
Comment on attachment 8805097 [details] [diff] [review]
list-all-scheme-handlers.patch

Adding Boris, but we may need someone with Linux API knowledge and/or a lot of free time :-)

This looks great by the way, thanks for providing a patch.
Attachment #8805097 - Flags: feedback?(bzbarsky)
Comment on attachment 8805097 [details] [diff] [review]
list-all-scheme-handlers.patch

Sorry for the lack of response on this before.  :(

This generally looks reasonable.  There's a bunch of details that need careful checking, but as far as overall structure goes I just have one question: is there a reason to not just have nsIGIOMimeApp inherit from nsIHandlerApp instead of having a separate object that forwards things along?  That would avoid having to worry about complications like "what happens if someone resets .mimeApp to null?" and whatnot that are not really handled well by the code right now.  If there's a reason, it's worth documenting.

Looks like prior work on this stuff was reviewed by roc and written by you, so in terms of who should actually review this stuff... I guess I can.  I'd just like to get the answer to the question above sorted out first.
Attachment #8805097 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 8805097 [details] [diff] [review]
list-all-scheme-handlers.patch

This seems perfectly reasonable to me as well.
Attachment #8805097 - Flags: feedback?(jst) → feedback+
Sorry for late answer, I've been busy with some other stuff.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #10)
> Comment on attachment 8805097 [details] [diff] [review]
> list-all-scheme-handlers.patch
> is there a reason to not just have nsIGIOMimeApp inherit from
> nsIHandlerApp instead of having a separate object that forwards things
> along?  
Hm, looking at the code, I need some service to obtain list of application (list of nsIGIOMimeApp) suitable for specified uri scheme (I use nsGIOService::GetAppsForURIScheme for it). 

On the other hand it could be done in nsMIMEInfoUnix::GetPossibleApplicationHandlers similar way it is done for example in Android implementation, similar to static nsMIMEInfoAndroid::GetMimeInfoForMimeType [1] if we wrap the code in some #ifdef MOZ_ENABLE_GIO. What do you think?

[1] http://searchfox.org/mozilla-central/rev/0aed9484bd3e97206fd1949ee4a4992ef300a81f/uriloader/exthandler/android/nsMIMEInfoAndroid.cpp#48
Flags: needinfo?(bzbarsky)
The getting a list thing is not the problem; the way it works in the patch is fine, imo.

The question I had was why the list that we get is a list of nsIGIOHandlerApp instances, each of which points to a nsIGIOMimeApp.  Why not just have it be a list of nsIGIOMimeApp instances and have nsIGIOMimeApp inherit from nsIHandlerApp (and add the new "command" member on nsIGIOMimeApp).

That is, why did we need the extra nsIGIOHandlerApp indirection?
Flags: needinfo?(bzbarsky)
Oh, I see. If the nsIGIOMimeApp inherits from nsIHandlerApp we shouldn't need nsIGIOHandlerApp. Can I keep nsMIMEInfoUnix::GetPossibleApplicationHandlers as it is in the patch?
At first glance, that seems fine, yes.
There's one thing though. In nsHandlerService.js and dialog.js we would have to check if the object is instance of nsIGIOMimeApp instead of nsIGIOHandlerApp. Is that okay?
You mean in the new code being added?  As far as I can tell, that's fine, yes.  Why wouldn't it be?
Attachment #8836689 - Attachment is obsolete: true
Comment on attachment 8836689 [details]
Bug 1297686 - List GIO handlers in protocol handler list in handlers dialog and in preferences,

Struggling with mozreview part a bit. Sorry for the fuzz.
Attachment #8836689 - Attachment is obsolete: true
Attachment #8836689 - Flags: review?(bzbarsky)
Attachment #8836700 - Attachment is obsolete: true
More description of changes in mozreview. 
However I'm getting following warnings during build:

0:05.23 In file included from /home/jhorak/mozilla/mozilla-central/objdir/dist/include/nsIGIOService.h:14:0,
 0:05.23                  from /home/jhorak/mozilla/mozilla-central/toolkit/system/gnome/nsGIOService.h:9,
 0:05.23                  from /home/jhorak/mozilla/mozilla-central/toolkit/system/gnome/nsGIOService.cpp:6:
 0:05.23 /home/jhorak/mozilla/mozilla-central/objdir/dist/include/nsIMIMEInfo.h:464:14: warning: ‘virtual nsresult nsIHandlerApp::GetName(nsAString_internal&)’ was hidden [-Woverloaded-virtual]
 0:05.23    NS_IMETHOD GetName(nsAString & aName) = 0;
 0:05.23               ^~~~~~~
 0:05.23 In file included from /home/jhorak/mozilla/mozilla-central/toolkit/system/gnome/nsGIOService.h:9:0,
 0:05.23                  from /home/jhorak/mozilla/mozilla-central/toolkit/system/gnome/nsGIOService.cpp:6:
 0:05.23 /home/jhorak/mozilla/mozilla-central/objdir/dist/include/nsIGIOService.h:52:14: warning:   by ‘virtual nsresult nsIGIOMimeApp::GetName(nsACString_internal&)’ [-Woverloaded-virtual]
 0:05.23    NS_IMETHOD GetName(nsACString & aName) = 0;
 0:05.23               ^~~~~~~

I'm thinking about removing name attribute from nsIGIOMimeApp and using nsIHandlerApp name attr instead where required. What do you think?
That would be OK, yes, if the two names have the same semantics.

Looking at the callers of nsGIOMimeApp::GetName, all of them end up converting the name to UTF-16 anyway, so would actually be better off with the nsAString version anyway.
Oh, and I apologize for the lag here.  I will try to get to this today, but I'm not sure whether it will happen yet...
Comment on attachment 8836689 [details]
Bug 1297686 - List GIO handlers in protocol handler list in handlers dialog and in preferences,

https://reviewboard.mozilla.org/r/112054/#review114608

r=me with the nits below addressed.

::: toolkit/system/gnome/nsGIOService.cpp:56
(Diff revision 3)
>    aName.Assign(g_app_info_get_name(mApp));
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +nsGIOMimeApp::GetName(nsAString & aName)

& next to nsAString.

::: toolkit/system/gnome/nsGIOService.cpp:60
(Diff revision 3)
>  NS_IMETHODIMP
> +nsGIOMimeApp::GetName(nsAString & aName)
> +{
> +  nsCString name;
> +  GetName(name);
> +  aName.Assign(NS_ConvertUTF8toUTF16(name));

NS_CopyUTF8toUTF16(name, aName);
    
but I assume this code will change anyway per the GetName discussion.

::: toolkit/system/gnome/nsGIOService.cpp:65
(Diff revision 3)
> +  aName.Assign(NS_ConvertUTF8toUTF16(name));
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsGIOMimeApp::SetName(const nsAString & aName)

& next to nsAString.

::: toolkit/system/gnome/nsGIOService.cpp:109
(Diff revision 3)
>  
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +nsGIOMimeApp::GetDetailedDescription(nsAString & aDetailedDescription)

& next to nsAString.

::: toolkit/system/gnome/nsGIOService.cpp:115
(Diff revision 3)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMETHODIMP
> +nsGIOMimeApp::SetDetailedDescription(const nsAString & aDetailedDescription)

& next to nsAString.

::: toolkit/system/gnome/nsGIOService.cpp:126
(Diff revision 3)
> +nsGIOMimeApp::Equals(nsIHandlerApp* aHandlerApp, bool* _retval)
> +{
> +  if (!aHandlerApp)
> +    return NS_ERROR_FAILURE;
> +
> +  nsCString thisName;

Make this an nsString too, for simplicity.

::: toolkit/system/gnome/nsGIOService.cpp:344
(Diff revision 3)
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +nsGIOService::GetAppsForURIScheme(const nsACString& aURIScheme,
> +                                 nsIMutableArray **aResult)

** next to nsIMutableArray, please.

::: toolkit/system/gnome/nsGIOService.cpp:344
(Diff revision 3)
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +nsGIOService::GetAppsForURIScheme(const nsACString& aURIScheme,
> +                                 nsIMutableArray **aResult)

Please fix the indent here.

::: toolkit/system/gnome/nsGIOService.cpp:351
(Diff revision 3)
> +
> +  nsresult rv = NS_OK;
> +  nsCOMPtr<nsIMutableArray> handlersArray =
> +    do_CreateInstance(NS_ARRAY_CONTRACTID);
> +
> +  nsCString contentType("x-scheme-handler/");

Might as well maks this an nsAutoCString and skip the extra allocations.

::: toolkit/system/gnome/nsGIOService.cpp:354
(Diff revision 3)
> +    do_CreateInstance(NS_ARRAY_CONTRACTID);
> +
> +  nsCString contentType("x-scheme-handler/");
> +  contentType.Append(aURIScheme);
> +
> +  GList* appInfoList = g_app_info_get_all_for_type(contentType.get());

Do we need to free this list once we're done with it?   If not, how does it get cleaned up?  The g_app_info_get_all_for_type documentation is ... not helpful.

::: toolkit/system/gnome/nsGIOService.cpp:357
(Diff revision 3)
> +  contentType.Append(aURIScheme);
> +
> +  GList* appInfoList = g_app_info_get_all_for_type(contentType.get());
> +  if (appInfoList) {
> +    GList* l = appInfoList;
> +    while (l != NULL) {

Just `while (l)` is probably better.  And maybe with a variable name that doesn't look like 1.

::: toolkit/system/gnome/nsGIOService.cpp:359
(Diff revision 3)
> +  GList* appInfoList = g_app_info_get_all_for_type(contentType.get());
> +  if (appInfoList) {
> +    GList* l = appInfoList;
> +    while (l != NULL) {
> +      nsCOMPtr<nsIGIOMimeApp> mimeApp = new nsGIOMimeApp(G_APP_INFO(l->data));
> +      if (mimeApp) {

Can't be null; `new` uses an infallible (crash on failure) allocator.

::: toolkit/system/gnome/nsGIOService.cpp:367
(Diff revision 3)
> +      l = l->next;
> +    }
> +  } else {
> +    rv = NS_ERROR_FAILURE;
> +  }
> +  NS_ADDREF(*aResult = handlersArray);

You probably shouldn't do that if you plan to return NS_ERROR_FAILURE.  So in other words, just early-return NS_ERROR_FAILURE if !appInfoList, and then outdent everything.

::: uriloader/exthandler/nsHandlerService.js:854
(Diff revision 3)
>        this._removeTarget(aHandlerAppID, NC_PATH);
>        this._removeTarget(aHandlerAppID, NC_URI_TEMPLATE);
> +      this._removeTarget(aHandlerAppID, NC_APP_NAME);
> +      this._removeTarget(aHandlerAppID, NC_APP_COMMANDLINE);
> +    }
> +    else if(aHandlerApp instanceof Ci.nsIGIOMimeApp){

Space after "if", please.  Yes, I know the previous two branches of the "else if" have the same problem.  :(

::: uriloader/exthandler/nsHandlerService.js:1074
(Diff revision 3)
>      }
>      else if(aHandlerApp instanceof Ci.nsIDBusHandlerApp){
>        aHandlerApp.QueryInterface(Ci.nsIDBusHandlerApp);
>        handlerAppID += "dbus:" + aHandlerApp.service + " " + aHandlerApp.method + " " + aHandlerApp.uriTemplate;
> +    }
> +    else if(aHandlerApp instanceof Ci.nsIGIOMimeApp){

Again, space after "if".

::: uriloader/exthandler/unix/nsMIMEInfoUnix.cpp:138
(Diff revision 3)
>    NS_ADDREF(*aPossibleAppHandlers);
>    return NS_OK;
>  }
> +#elif defined(MOZ_ENABLE_GIO)
> +NS_IMETHODIMP
> +nsMIMEInfoUnix::GetPossibleApplicationHandlers(nsIMutableArray ** aPossibleAppHandlers)

** next to nsIMutableArray.

::: xpcom/system/nsIGIOService.idl:12
(Diff revision 3)
> +#include "nsIMIMEInfo.idl"
>  
>  interface nsIUTF8StringEnumerator;
>  interface nsIURI;
> +interface nsIMutableArray;
> +interface nsIHandlerApp;

No need; it's defined in the nsIMIMEInfo.idl you're including.

::: xpcom/system/nsIGIOService.idl:18
(Diff revision 3)
>  
>  /* nsIGIOMimeApp holds information about an application that is looked up
>     with nsIGIOService::GetAppForMimeType. */
>  // 66009894-9877-405b-9321-bf30420e34e6 prev uuid
>  
> -[scriptable, uuid(ca6bad0c-8a48-48ac-82c7-27bb8f510fbe)] 
> +[scriptable, uuid(c9ed29e3-cabb-4769-9b49-cb38b0713178)]

No need to change interface ids anymore, for what it's worth.
Attachment #8836689 - Flags: review?(bzbarsky) → review+
Comment on attachment 8836689 [details]
Bug 1297686 - List GIO handlers in protocol handler list in handlers dialog and in preferences,

https://reviewboard.mozilla.org/r/112054/#review114608

> Do we need to free this list once we're done with it?   If not, how does it get cleaned up?  The g_app_info_get_all_for_type documentation is ... not helpful.

Yes, it needs to be freed by g_list_free. List items ownership is transfered to the nsGIOMimeApp.

> Just `while (l)` is probably better.  And maybe with a variable name that doesn't look like 1.

Yes, sorry about that, renamed to appInfo.
Comment on attachment 8836689 [details]
Bug 1297686 - List GIO handlers in protocol handler list in handlers dialog and in preferences,

https://reviewboard.mozilla.org/r/112054/#review116524

r=me.  Thank you!
I kicked off a try run.  It's at https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e53ab84ef76f75f13dc21c213057864bd127f22

This shows a number of failures:

1)  Failures in browser/components/feeds/test/test_registerHandler.html 
2)  Crashes in nsExternalHelperAppService::ExternalProtocolHandlerExists when
    running dom/base/test/test_bug606729.html; looks like a null-deref.  Probably
    because it calls GetPossibleApplicationHandlers and then uses the return value
    without checking whether it got anything.  Seems fishy.
Flags: needinfo?(jhorak)
Sorry about the try run. I used to have it when the mq workflow was in use. For some reason I don't have enough rights to run in now.

When no handler app is found the g_app_info_get_all_for_type returns NULL and not an empty list. The documentation is rather vague there [1], current source of glib2 [2]. In that case the previous patch 
the nsMIMEInfoUnix::GetPossibleApplicationHandlers returns out of memory. This failed the tests because possibleAppHandlerArray is frequently used as mutable array in handlerService.

I've fixed that by expecting NULL is actually no app info found.

[1] https://developer.gnome.org/gio/stable/GAppInfo.html#g-app-info-get-all-for-type
[2] https://github.com/GNOME/glib/blob/0d28ee458f1847b3c7f3ed810b28b9a988ece40a/gio/gdesktopappinfo.c#L3955
Flags: needinfo?(jhorak)
Comment on attachment 8836689 [details]
Bug 1297686 - List GIO handlers in protocol handler list in handlers dialog and in preferences,

https://reviewboard.mozilla.org/r/112054/#review120882

::: toolkit/system/gnome/nsGIOService.cpp:344
(Diff revisions 4 - 5)
>  
>    nsAutoCString contentType("x-scheme-handler/");
>    contentType.Append(aURIScheme);
>  
>    GList* appInfoList = g_app_info_get_all_for_type(contentType.get());
> +  // The g_app_info_get_all_for_type returns NULL when no appinfo is found

s/The //

::: toolkit/system/gnome/nsGIOService.cpp:346
(Diff revisions 4 - 5)
>    contentType.Append(aURIScheme);
>  
>    GList* appInfoList = g_app_info_get_all_for_type(contentType.get());
> +  // The g_app_info_get_all_for_type returns NULL when no appinfo is found
> +  // or error occurs (contentType is NULL). We are fine with empty app list
> +  // and we're sure that contenType is not NULL, so we won't return failure.

"contentType"
No need to apologize.  This is what try is for.  ;)

I did another try push with the update patch.  It's at https://treeherder.mozilla.org/#/jobs?repo=try&revision=78057c4260b97bd0eaa42f7765631cb10ce2f416

There's one lint failure in the JS changes:

  TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/mozapps/handling/content/dialog.js:137:12 |
  Closing curly brace does not appear on the same line as the subsequent block. (brace-style)

which I think is saying you want the "else if" on the same line as the '} closing the if body.

There are also failures in the xpcshell uriloader/exthandler/tests/unit/test_handlerService.js test.

The about:newtab csp failures are unlikely to be related to this patch; chances are they have to do with the base revision the mozreview push was on top of....
Flags: needinfo?(jhorak)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #35)
> There are also failures in the xpcshell
> uriloader/exthandler/tests/unit/test_handlerService.js test.
Hm, this one is a bit game changer. Test expects that possibleApplicationHandlers for http are empty:
http://searchfox.org/mozilla-central/source/uriloader/exthandler/tests/unit/test_handlerService.js#151
Not sure why is that. 

I would have to change the patch the way the nsGIOService::GetAppsForURIScheme is called from files where possibleApplicationHandlers are requested (handling/content/dialog.js, nsHandlerService.js and maybe some more I guess) and not from GetPossibleApplicationHandlers.
Flags: needinfo?(jhorak)
Looks like dmose wrote this stuff once upon a time.  Dan, I know it's been a while, but do you recall why this test is asserting no handlers for "http" in this case?
Flags: needinfo?(dmose)
That does seem strange.  Sadly, after staring at the code a bunch, it's still not coming back to me (sorry for not commenting better!).  Perhaps dolske remembers?
Flags: needinfo?(dmose) → needinfo?(dolske)
I don't remember either. I think Paolo has touched this code more recently this decade, perhaps he can be of help.
Flags: needinfo?(dolske) → needinfo?(paolo.mozmail)
Uh, yes, I've looked at this code very recently.

The first thing is that we're actually replacing the RDF storage with JSON. This is already implemented and we'll likely switch and migrate data before the next merge. The implementation and regression tests for both back-ends should be updated, unless you wait for bug 1287658 to land first, in which case you can add the functionality only to the JSON back-end.

With regard to the question about possibleApplicationHandlers, there is definitely code that relies on it being empty by default. I don't know if this is only test code, though.

The issue is clearly that the same object is mixing operating-system provided handler information and manually configured handler information. We have to de-duplicate them when loading or when saving the configuration. We need to add specific tests that this happens correctly for possibleApplicationHandlers. There is also code that expects the first possible application handler to be the preferred handler.

I will probably need to spend more time looking at the changes made in this bug, but I have to work on the JSON back-end first, so this may delay thing a little bit.
Flags: needinfo?(paolo.mozmail)
Paolo, I'm going to leave the needinfo on you so this doesn't get lost...
Flags: needinfo?(paolo.mozmail)
The migration to JSON just landed, and it includes several new tests in "common_test_handlerService.js" that can be used to make sure the new implementation is compliant to the current API.
Flags: needinfo?(paolo.mozmail)
Hi there, any news on this bug?  It’s still preventing XMPP clients from being used properly on Linux desktops.
Flags: needinfo?(jhorak)
Paolo, at this point is what's needed changes to the patch to use the new JSON backend?
Flags: needinfo?(paolo.mozmail)
Trying to apply the patch to the latest trunk, application chooser seems to work, but preferences are broken. Will try to fix this.
Flags: needinfo?(jhorak)
(In reply to Boris Zbarsky [:bz] from comment #44)
> Paolo, at this point is what's needed changes to the patch to use the new
> JSON backend?

Yes, the serialization of nsIGIOMimeApp should be added to the JSON back-end, while there is no need to implement the RDF serialization. There are also new regression tests that should be updated.
Flags: needinfo?(paolo.mozmail)
I've tried to do the best to port it to the nsHandlerService-json and in-content-new. Not sure if bz is the right reviewer or paolo.
Definitely not me, for those bits; I know nothing about them...
Comment on attachment 8836689 [details]
Bug 1297686 - List GIO handlers in protocol handler list in handlers dialog and in preferences,

https://reviewboard.mozilla.org/r/112054/#review172656

I can review this, but it will have to wait for later next week.

You need to add a regression tests for the JSON back-end persistence to the existing test file.

::: uriloader/exthandler/nsHandlerService-json.js:353
(Diff revision 7)
> -        handlerInfo.possibleApplicationHandlers.appendElement(handlerApp);
> +        let handlerAlreadyIn = false;
> +        // Don't add handler if already in possibleHandlers
> +        let enumerator = possibleApplicationHandlers.enumerate();
> +        while (enumerator.hasMoreElements()) {
> +          let handlerFromPossibleHandlers = enumerator.getNext().QueryInterface(Ci.nsIHandlerApp);
> +          if (handlerFromPossibleHandlers && handlerApp.equals(handlerFromPossibleHandlers)) {
> +            handlerAlreadyIn = true;
> +            break;
> +          }
> +        }
> +        if (!handlerAlreadyIn) {
> +          possibleApplicationHandlers.appendElement(handlerApp);
> +        }

I'm not sure this is necessary, if I remember correctly the consumers already accept the presence of duplicate entries in possibleApplicationHandlers.

If it turns out to be necessary, we need a test that is able to exercise this code.
Comment on attachment 8836689 [details]
Bug 1297686 - List GIO handlers in protocol handler list in handlers dialog and in preferences,

https://reviewboard.mozilla.org/r/112054/#review173966

::: uriloader/exthandler/nsHandlerService-json.js:353
(Diff revision 7)
> -        handlerInfo.possibleApplicationHandlers.appendElement(handlerApp);
> +        let handlerAlreadyIn = false;
> +        // Don't add handler if already in possibleHandlers
> +        let enumerator = possibleApplicationHandlers.enumerate();
> +        while (enumerator.hasMoreElements()) {
> +          let handlerFromPossibleHandlers = enumerator.getNext().QueryInterface(Ci.nsIHandlerApp);
> +          if (handlerFromPossibleHandlers && handlerApp.equals(handlerFromPossibleHandlers)) {
> +            handlerAlreadyIn = true;
> +            break;
> +          }
> +        }
> +        if (!handlerAlreadyIn) {
> +          possibleApplicationHandlers.appendElement(handlerApp);
> +        }

Yes, this is required. What happens if that's missing is that handlerInfo.possibleApplicationHandlers in Linux now always returns array with nsGIOMimeApp instances which are used for the scheme.

These possibleApplicationHandlers are stored from previous execution in the json file so adding stored handlerApp without checking whenever possibleApplicationHandlers already has it leads to duplicates.

We would have to avoid storing nsGIOMimeApps to json, but that would break remembering the choice from before I guess.
(In reply to Jan Horak from comment #51)
> handlerInfo.possibleApplicationHandlers in Linux now always returns array
> with nsGIOMimeApp instances which are used for the scheme.

Eh, I'm afraid this will break regression tests on some machines because the initial contents of possibleApplicationHandlers is not always empty. We may have to clear the list manually like we do for MIME type handlers:

https://dxr.mozilla.org/mozilla-central/rev/b95b1638db48fc3d450b95b98da6bcd2f9326d2f/uriloader/exthandler/tests/HandlerServiceTestUtils.jsm#98-105

However, given that other consumers may assume that possibleApplicationHandlers is always empty for protocol handlers, we'll have to verify whether changing this breaks anything in the user interface.

The original idea for MIME type handlers was that possibleLocalHandlers would contain a list of everything available, and possibleApplicationHandlers would start empty and then be populated with the shortlist of applications that were chosen at least once by the user. Unfortunately, this code is quite old and this assumption wasn't always kept over time, for example in the Android implementation, and we already had to special-case it in the regression tests.

Some confusion also accumulated over time over the distinction between MIME type handlers and protocol handlers. It seems to me that your patch applies the application search to both cases, but we should really only use it for protocol handlers.

Additionally, I have concerns about the performance of looking up the list of protocol handlers, since we run this code on the main thread. This would happen even if we end up just using the default handler. We already had bugs on file for similar issues, and they are fundamentally caused by the current architecture, but I think we shouldn't make the situation worse.

To solve this, we could either choose the route of the possibleLocalHandlers system, extending it to accept other handler classes, or add a separate method to populate possibleApplicationHandlers only in the few instances where it is needed.
Comment on attachment 8836689 [details]
Bug 1297686 - List GIO handlers in protocol handler list in handlers dialog and in preferences,

I'm sorry I didn't have time to look into this in detail before, but given the concerns from comment 52 I think we should put some more thought into this patch before landing it.
Attachment #8836689 - Flags: feedback-
(In reply to :Paolo Amadini from comment #52)
> (In reply to Jan Horak from comment #51)
> > handlerInfo.possibleApplicationHandlers in Linux now always returns array
> > with nsGIOMimeApp instances which are used for the scheme.
> 
> Eh, I'm afraid this will break regression tests on some machines because the
> initial contents of possibleApplicationHandlers is not always empty. 
> We may have to clear the list manually like we do for MIME type handlers:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> b95b1638db48fc3d450b95b98da6bcd2f9326d2f/uriloader/exthandler/tests/
> HandlerServiceTestUtils.jsm#98-105
Yes, some tests are broken due to this.

> However, given that other consumers may assume that
> possibleApplicationHandlers is always empty for protocol handlers, we'll
> have to verify whether changing this breaks anything in the user interface.
> 
> The original idea for MIME type handlers was that possibleLocalHandlers
> would contain a list of everything available, and
> possibleApplicationHandlers would start empty and then be populated with the
> shortlist of applications that were chosen at least once by the user.
This could comply with the very end comment.

> Unfortunately, this code is quite old and this assumption wasn't always kept
> over time, for example in the Android implementation, and we already had to
> special-case it in the regression tests.
> 
> Some confusion also accumulated over time over the distinction between MIME
> type handlers and protocol handlers. It seems to me that your patch applies
> the application search to both cases, but we should really only use it for
> protocol handlers.
You mean that nsGIOMimeApp now implements nsIHandlerApp while inherits from nsIGIOMimeApp?

> 
> Additionally, I have concerns about the performance of looking up the list
> of protocol handlers, since we run this code on the main thread. This would
> happen even if we end up just using the default handler. We already had bugs
> on file for similar issues, and they are fundamentally caused by the current
> architecture, but I think we shouldn't make the situation worse.
Well the g_app_info_get_all_for_type does not have async version ATM, so Gtk seems to not to see it as a problem so far. For the dialog I wouldn't see is as problem, for the preferences, hm, might be, because we're loading handlers for all schemes. How can we deal with it?

> To solve this, we could either choose the route of the possibleLocalHandlers
> system, extending it to accept other handler classes, or add a separate
> method to populate possibleApplicationHandlers only in the few instances
> where it is needed.

Okay, the list of the handlers for scheme dialog fills there by populateList [1]:

Should I just append nsIGIOMimeApp there to the list while not touching possibleApplicationHandlers at all (by adding the items without creating duplicates)?
And then add selected handler to the possibleApplicationHandlers in onAccept?

And similarly in rebuildActionsMenu [2] append nsIGIOMimeApp to the menu and in _storeAction check if the selected item is in possibleApplicationHandler and if it is not append to it? 
[1] http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/toolkit/mozapps/handling/content/dialog.js#105
[2] http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/browser/components/preferences/in-content-new/main.js#1671
(In reply to Jan Horak from comment #54)
> > Some confusion also accumulated over time over the distinction between MIME
> > type handlers and protocol handlers. It seems to me that your patch applies
> > the application search to both cases, but we should really only use it for
> > protocol handlers.
> You mean that nsGIOMimeApp now implements nsIHandlerApp while inherits from
> nsIGIOMimeApp?

Not sure what you're asking, but what I mean is that you call GetAppsForURIScheme with the mSchemeOrType argument, which can be a scheme (mailto) or MIME type (text/plain). Confusingly, the same class handles both.

> Okay, the list of the handlers for scheme dialog fills there by populateList
> [1]:
> 
> Should I just append nsIGIOMimeApp there to the list while not touching
> possibleApplicationHandlers at all (by adding the items without creating
> duplicates)?

That sounds good to me.

> And then add selected handler to the possibleApplicationHandlers in onAccept?

The preferredApplicationHandler is already stored in such a way that it is included in possibleApplicationHandlers when populating the handler information the next time, so this might be obtained for free.

> And similarly in rebuildActionsMenu [2] append nsIGIOMimeApp to the menu and
> in _storeAction check if the selected item is in possibleApplicationHandler
> and if it is not append to it? 

For the preferences, I suggest using the appPicker dialog route, which currently loads the list from possibleLocalHandlers:

http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/browser/components/preferences/in-content-new/main.js#2049-2077

This is currently enabled on Windows only, but you could enable it also on Linux for protocol handlers.
For some reason I'm unable to set Paolo as reviewer (not accepting reviews now). Mike, feel free to reassign the review to someone else. Thanks.
Attachment #8836689 - Flags: review?(mh+mozilla) → review?(karlt)
Attachment #8836689 - Flags: review?(karlt)
Given Paolo already knows what is happening here, it is more efficient to wait until he is back.

The reviewboard API is unhelpful, just clearing the reviewer when submitting a change to :paolo.
The bugzilla API at least says "is not currently accepting 'review' requests."
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(paolo.mozmail)
Attachment #8836689 - Flags: review?(paolo.mozmail)
The patch looks quite good! Thanks for the code simplification and the detailed overview in the commit message.

I'll try to do a review later this week.
Comment on attachment 8836689 [details]
Bug 1297686 - List GIO handlers in protocol handler list in handlers dialog and in preferences,

https://reviewboard.mozilla.org/r/112054/#review188146

I'm not sure if these files are excluded from the style checks we do with ESLint, but you can verify using "./mach lint -wo".

To verify that serialization works, you should add a test to test_handlerService_json.js that would be very similar to this one:

https://dxr.mozilla.org/mozilla-central/rev/2cd3752963fc8f24f7c202687eab55e83222f608/uriloader/exthandler/tests/unit/common_test_handlerService.js#278-320

::: browser/components/preferences/in-content/main.js:1656
(Diff revision 8)
>  
>      if (aHandlerApp instanceof Ci.nsIWebContentHandlerInfo)
>        return aHandlerApp.uri;
>  
> +    if (aHandlerApp instanceof Ci.nsIGIOMimeApp)
> +      return aHandlerApp.name;

This likely has to check "aHandlerApp.command" instead, see below.

::: browser/components/preferences/in-content/main.js:1820
(Diff revision 8)
> +          let label = handler.name;
> +          label = this._prefsBundle.getFormattedString("useApp", [label]);

Simplify to one line.

::: toolkit/mozapps/handling/content/dialog.js:136
(Diff revision 8)
> +      }
> +      else if (app instanceof Ci.nsIGIOMimeApp) {

nit: brace on the same line

::: toolkit/mozapps/handling/content/dialog.js:161
(Diff revision 8)
> +      let gIOSvc = Cc["@mozilla.org/gio-service;1"].
> +                   getService(Ci.nsIGIOService);

indentation nit:

let gIOSvc = Cc["@mozilla.org/gio-service;1"]
               .getService(Ci.nsIGIOService);

::: toolkit/system/gnome/nsGIOService.cpp:111
(Diff revision 8)
> +NS_IMETHODIMP
> +nsGIOMimeApp::Equals(nsIHandlerApp* aHandlerApp, bool* _retval)
> +{
> +  if (!aHandlerApp)
> +    return NS_ERROR_FAILURE;
> +
> +  nsString thisName;
> +  nsString theirName;
> +
> +  GetName(thisName);
> +  aHandlerApp->GetName(theirName);
> +
> +  *_retval = thisName.Equals(theirName);
> +  return NS_OK;
> +}

It seems to me that the key used to find the application in the system elsewhere is the command, so we should probably compare it rather than the name. This is similar to nsILocalHandlerApp comparing the executable, for example.

::: toolkit/system/gnome/nsGIOService.cpp:117
(Diff revision 8)
> +  nsString thisName;
> +  nsString theirName;

nit: my general understanding is that for short strings it's better to use nsAutoString, which I see used in other similar cases in the code. There are various instances that can be changed in this patch.

::: toolkit/system/gnome/nsGIOService.cpp:351
(Diff revision 8)
> +      nsString name;
> +      mimeApp->GetName(name);

This looks unused?

::: uriloader/exthandler/nsHandlerService-json.js:453
(Diff revision 8)
> +    } else if ("command" in handlerObj) {
> +      if (Cc["@mozilla.org/gio-service;1"]) {
> +        let gIOSvc = Cc["@mozilla.org/gio-service;1"].
> +                     getService(Ci.nsIGIOService);
> +        // We use application name and commandline to recreate nsIHandlerApp
> +        // instance by using gio service.
> +        if (!handlerObj.name || !handlerObj.command)
> +          return null;
> +        handlerApp = gIOSvc.createAppFromCommand(handlerObj.command, handlerObj.name)
> +                     .QueryInterface(Ci.nsIHandlerApp);
> +      }
>      } else {

This can be corrected and simplified to:

    } else if ("command" in handlerObj &&
               "@mozilla.org/gio-service;1" in Cc) {
      handlerApp = Cc["@mozilla.org/gio-service;1"]
                     .getService(Ci.nsIGIOService)
                     .createAppFromCommand(handlerObj.command,
                                           handlerObj.name);
    }

(Note that the name will be overwritten at the end of the method anyways.)

I'm not sure of whether the final .QueryInterface(Ci.nsIHandlerApp) is necessary or not, as the return type of the method is nsIGIOMimeApp. If it's necessary, you can add it back at the end of the sequence, aligned with createAppFromCommand.
Attachment #8836689 - Flags: review?(paolo.mozmail)
Comment on attachment 8836689 [details]
Bug 1297686 - List GIO handlers in protocol handler list in handlers dialog and in preferences,

https://reviewboard.mozilla.org/r/112054/#review188146

> It seems to me that the key used to find the application in the system elsewhere is the command, so we should probably compare it rather than the name. This is similar to nsILocalHandlerApp comparing the executable, for example.

The nsIHandlerApp does not have command/executable, should I try to cast to nsILocalHandler, like http://searchfox.org/mozilla-central/source/uriloader/exthandler/nsLocalHandlerApp.cpp#55 ? I guess the default system handler is in fact nsILocalHandler instance, or am I wrong? Should I also add support to nsILocalHandler::Equals for nsIGIOMimerApp?
Can you simply convert the other operand to nsIGIOMimeApp and check the command? If the other operand is not an nsIGIOMimeApp then it's considered different.
(In reply to :Paolo Amadini from comment #63)
> Can you simply convert the other operand to nsIGIOMimeApp and check the
> command? If the other operand is not an nsIGIOMimeApp then it's considered
> different.
Well I'd like to compare nsGIOMimeHandler to nsILocalHandler, because system default application handler (which is added first to the list) is also in the list of gio application handlers. So current implementation won't duplicate entries in the list.

The other thing I've hit yesterday is for example that nsGIOHandler command  is "transmission-gtk %U %u" while nsILocalHandler command (default system handler) is "transmission-gtk %U", so handler applications in the list are somehow duplicated since strings are not exactly equal.

Or are duplicate entries in the list fine (does not look good to me)?
Flags: needinfo?(paolo.mozmail)
(In reply to Jan Horak from comment #64)
> Well I'd like to compare nsGIOMimeHandler to nsILocalHandler, because system
> default application handler (which is added first to the list) is also in
> the list of gio application handlers. So current implementation won't
> duplicate entries in the list.
> 
> The other thing I've hit yesterday is for example that nsGIOHandler command 
> is "transmission-gtk %U %u" while nsILocalHandler command (default system
> handler) is "transmission-gtk %U", so handler applications in the list are
> somehow duplicated since strings are not exactly equal.

Ah, that makes sense. For now, it looks like the easiest option is to compare the name provided by the system instead of the command. In the end, this code is just finding new potential handlers, so we don't regress the ability to access the existing default system handlers, and we can live with potentially missing one extra handler. If this causes issues in the future, we can revisit.

For completeness, when the other instance is a GIO handler I would still compare the commands and consider the two instances equal if the commands are the same, even if the name is different.

In the front-end places where we call "equals", we should note in a comment that we have to call the method on the GIO handler instance, as calling the one on the other handler wouldn't detect the equality. In the future, we should likely refactor the code so that nsIHandlerApp instances simply provide an equality key field that can be used by the base class, but this simpler solution is fine for now.
Flags: needinfo?(paolo.mozmail)
First of all, thanks that you're looking into this.

I've hit another issue with stored gio handlers. In case user uninstall previously chosen application which he made default handler before, the error page is shown because the handler is not found [1]. This is because alwaysAsk == false, so it never tries to ask user for handler. This is rather annoying and can be fixed only by going to the preferences, deleting handlers.json or reinstalling the application again.

I've modified the patch to fix this (showing app chooser dialog when NS_ERROR_FILE_NOT_FOUND error happens) and also added new nsGIOService::FindAppFromCommandline method which returns null when the handler is not found (it differs from nsGIOService::CreateAppFromCommand only by not calling g_app_info_create_from_commandline).

But this makes testing difficult. I can store non existing gio handler to the storage, but loading it should not return nsGIOMimeApp instance but nullptr. I could add some bool nsGIOMimeApp::HandlerExists() to check the handler app is available and create an instance of nsGIOMimeApp anyway, but that would require to test that at various places I believe.

[1] http://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/uriloader/exthandler/nsExternalHelperAppService.cpp#1008
(In reply to Jan Horak from comment #66)
> I've modified the patch to fix this (showing app chooser dialog when
> NS_ERROR_FILE_NOT_FOUND error happens)

This is a nice feature, probably helpful in general. Can you move this part to a separate bug, so it's easier to track possible regressions? A test for it would be appreciated but not required, given that the current state of tests in the module may make it cumbersome to write.

I'll probably be able to take a look at the other changes this week.
Comment on attachment 8836689 [details]
Bug 1297686 - List GIO handlers in protocol handler list in handlers dialog and in preferences,

https://reviewboard.mozilla.org/r/112054/#review194002

The approach in this patch has the limit that an application should be listed in the system for it to be valid, even though the executable could still exist. While this may be fine, it has the testing issues you found.

So another approach could be to continue to use CreateAppFromCommand, then separate the executable name from the arguments (we may have functions to do this already) and check if the executable exists. This could be done by a method on nsIGIOHandlerApp. This way, for testing you could create a temporary file like we do for nsILocalHandlerApp and just add very similar tests for the cases where the application exists and where it doesn't.

::: toolkit/system/gnome/nsGIOService.cpp:119
(Diff revisions 8 - 9)
>      return NS_ERROR_FAILURE;
>  
> -  nsString thisName;
> -  nsString theirName;
>  
> +  // Get nsIFile executable filename to compare with nsGIOMimeApp command

As we discussed, in this case you are comparing the readable application description rather than the actual command, so the comment should be updated accordingly.

::: toolkit/system/gnome/nsGIOService.cpp:120
(Diff revisions 8 - 9)
>  
> -  nsString thisName;
> -  nsString theirName;
>  
> +  // Get nsIFile executable filename to compare with nsGIOMimeApp command
> +  nsCOMPtr <nsILocalHandlerApp> localHandlerApp = do_QueryInterface(aHandlerApp);

nit: no space after nsCOMPtr

::: toolkit/system/gnome/nsGIOService.cpp:524
(Diff revisions 8 - 9)
> + * returns error.
> + * @param aCommandline command with parameters used to start the application
> + * @return NS_OK when application is found, NS_ERROR_NOT_AVAILABLE otherwise
> + */
> +NS_IMETHODIMP
> +nsGIOService::FindAppFromCommandline(nsACString const& aCommandline,

Either rename this to FindAppFromCommand, or rename the function below to CreateAppFromCommandline.

::: toolkit/system/gnome/nsGIOService.cpp:525
(Diff revisions 8 - 9)
> + * @param aCommandline command with parameters used to start the application
> + * @return NS_OK when application is found, NS_ERROR_NOT_AVAILABLE otherwise
> + */
> +NS_IMETHODIMP
> +nsGIOService::FindAppFromCommandline(nsACString const& aCommandline,
> +                                       nsIGIOMimeApp** aAppInfo)

nit: indent two spaces less

::: toolkit/system/gnome/nsGIOService.cpp:575
(Diff revisions 8 - 9)
>    GAppInfo *app_info = nullptr, *app_info_from_list = nullptr;
>    GList *apps = g_app_info_get_all();
>    GList *apps_p = apps;
>  
>    // Try to find relevant and existing GAppInfo in all installed application
>    // We do this by comparing each GAppInfo's executable with out own
>    while (apps_p) {
>      app_info_from_list = (GAppInfo*) apps_p->data;
>      if (!app_info) {
>        // If the executable is not absolute, get it's full path
>        char *executable = g_find_program_in_path(g_app_info_get_executable(app_info_from_list));
>  
>        if (executable && strcmp(executable, PromiseFlatCString(cmd).get()) == 0) {
>          g_object_ref (app_info_from_list);
>          app_info = app_info_from_list;
>        }
>        g_free(executable);
>      }
>  
>      g_object_unref(app_info_from_list);
>      apps_p = apps_p->next;
>    }
>    g_list_free(apps);

You should try to reuse FindAppFromCommandline here, or better, since the only caller is the code that sets the default browser, repurpose this function to always create a new app and have the code that sets the default browser call this as a fallback if FindAppFromCommandline returns null.

Setting the default browser would also have to be tested after this change.

::: uriloader/exthandler/nsHandlerService-json.js:221
(Diff revisions 8 - 9)
>  
>        for (let handlerNumber of Object.keys(schemes[scheme])) {
>          let handlerApp = this.handlerAppFromSerializable(schemes[scheme][handlerNumber]);
>          // If there is already a handler registered with the same template
>          // URL, the newly added one will be ignored when saving.
> +        if (handlerApp)

This particular call will never return null because it's initialized from a template URL. You may add a comment to that effect, but the check is not needed.

::: uriloader/exthandler/nsHandlerService-json.js:373
(Diff revisions 8 - 9)
> +      // It's possible that the handlerApp has been uninstalled and is no longer
> +      // available, in this case the handlerApp is null.
> +      if (!handlerApp)
> +        continue;
> +
>        if (isFirstItem) {
>          isFirstItem = false;
>          handlerInfo.preferredApplicationHandler = handlerApp;
>        }
> -      if (handlerApp) {
> -        handlerInfo.possibleApplicationHandlers.appendElement(handlerApp);
> +      handlerInfo.possibleApplicationHandlers.appendElement(handlerApp);
> -      }
> +    }
> -    }

The existing code was correct in how it reset preferredApplicationHandler when the function returned null.

::: uriloader/exthandler/nsHandlerService-json.js:462
(Diff revision 9)
> +    } else if ("command" in handlerObj &&
> +               "@mozilla.org/gio-service;1" in Cc) {
> +      handlerApp = Cc["@mozilla.org/gio-service;1"]
> +                     .getService(Ci.nsIGIOService)
> +                     .findAppFromCommandline(handlerObj.command);
> +      if (!handlerApp)

nit: braces around single-line statements in this file

::: uriloader/exthandler/unix/nsGNOMERegistry.cpp:85
(Diff revision 9)
>  nsGNOMERegistry::GetFromType(const nsACString& aMIMEType)
>  {
>    RefPtr<nsMIMEInfoUnix> mimeInfo = new nsMIMEInfoUnix(aMIMEType);
>    NS_ENSURE_TRUE(mimeInfo, nullptr);
>  
> -  nsAutoCString name;
> +  nsString name;

nsAutoString
Attachment #8836689 - Flags: review?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #69)
> ::: toolkit/system/gnome/nsGIOService.cpp:524
> Either rename this to FindAppFromCommand, or rename the function below to
> CreateAppFromCommandline.
> 
> ::: toolkit/system/gnome/nsGIOService.cpp:525
> nit: indent two spaces less
> 
> ::: toolkit/system/gnome/nsGIOService.cpp:575
> You should try to reuse FindAppFromCommandline here, or better, since the
> only caller is the code that sets the default browser, repurpose this
> function to always create a new app and have the code that sets the default
> browser call this as a fallback if FindAppFromCommandline returns null.
> 
> Setting the default browser would also have to be tested after this change.

Of course, this part of the review doesn't have to be addressed if you follow the alternative route I suggested at the start of the review.
(In reply to :Paolo Amadini from comment #68)
> (In reply to Jan Horak from comment #66)
> > I've modified the patch to fix this (showing app chooser dialog when
> > NS_ERROR_FILE_NOT_FOUND error happens)
> 
> This is a nice feature, probably helpful in general. Can you move this part
> to a separate bug, so it's easier to track possible regressions? A test for
> it would be appreciated but not required, given that the current state of
> tests in the module may make it cumbersome to write.

Moved to bug 1408010.
Comment on attachment 8836689 [details]
Bug 1297686 - List GIO handlers in protocol handler list in handlers dialog and in preferences,

https://reviewboard.mozilla.org/r/112054/#review194002

> This particular call will never return null because it's initialized from a template URL. You may add a comment to that effect, but the check is not needed.

Hm, the handlerAppFromSerializable could return null for the not found gio application. Or do I miss something?
(In reply to Jan Horak from comment #72)
> Hm, the handlerAppFromSerializable could return null for the not found gio
> application. Or do I miss something?

This code is related to loading presets for web applications handling certain protocols like "mailto:". Each localized build provides these to Firefox using special "about:config" preferences. The object used as input to handlerAppFromSerializable is constructed a few lines above, and always represents a web application handler rather than a local application handler. Hope this clarifies!
Comment on attachment 8836689 [details]
Bug 1297686 - List GIO handlers in protocol handler list in handlers dialog and in preferences,

https://reviewboard.mozilla.org/r/112054/#review196286


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/system/gnome/nsGIOService.cpp:35
(Diff revision 10)
> + */
> +static nsresult
> +GetCommandFromCommandline(nsACString const& aCommandWithArguments, nsACString& aCommand) {
> +  GError *error = nullptr;
> +  gchar **argv;
> +  if (!g_shell_parse_argv(aCommandWithArguments.BeginReading(), NULL, &argv, &error) ||

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

  if (!g_shell_parse_argv(aCommandWithArguments.BeginReading(), NULL, &argv, &error) ||
                                                                ^
                                                                nullptr
Looking at failing test test_handlerService_rdf.js -  do I need to implement loading of nsGIOMimeApp to the handlerService.js?
Flags: needinfo?(paolo.mozmail)
(In reply to Jan Horak [:jhorak] from comment #76)
> Looking at failing test test_handlerService_rdf.js -  do I need to implement
> loading of nsGIOMimeApp to the handlerService.js?

No, just moving the test to test_handlerService_json.js should suffice.
Flags: needinfo?(paolo.mozmail)
Comment on attachment 8836689 [details]
Bug 1297686 - List GIO handlers in protocol handler list in handlers dialog and in preferences,

https://reviewboard.mozilla.org/r/112054/#review197194

::: browser/components/preferences/in-content/main.js:1812
(Diff revision 10)
> +        }
> +        // Check if the handler is already in possibleHandlers
> +        let appAlreadyInHandlers = false;
> +        for (let i = possibleHandlers.length - 1; i >= 0; --i) {
> +          let app = possibleHandlers.queryElementAt(i, Ci.nsIHandlerApp);
> +          if (app.equals(handler)) {

This should be handler.equals(app) to consider nsILocalHandlerApp correctly, right? Add a comment to explain.

::: browser/components/preferences/in-content/main.js:1823
(Diff revision 10)
> +          let menuItem = document.createElement("menuitem");
> +          menuItem.setAttribute("action", Ci.nsIHandlerInfo.useHelperApp);
> +          let label = this._prefsBundle.getFormattedString("useApp", [handler.name]);
> +          menuItem.setAttribute("label", label);
> +          menuItem.setAttribute("tooltiptext", label);
> +          // TODO add application icon

We try to avoid TODO lines if possible. If this is something to implement later, file a bug and reference it here.

::: toolkit/mozapps/handling/content/dialog.js:174
(Diff revision 10)
> +        }
> +        // Check if the handler is already in possibleHandlers
> +        let appAlreadyInHandlers = false;
> +        for (let i = possibleHandlers.length - 1; i >= 0; --i) {
> +          let app = possibleHandlers.queryElementAt(i, Ci.nsIHandlerApp);
> +          if (app.equals(handler)) {

Same here as above.

::: toolkit/system/gnome/nsGIOService.cpp:36
(Diff revision 10)
> +static nsresult
> +GetCommandFromCommandline(nsACString const& aCommandWithArguments, nsACString& aCommand) {
> +  GError *error = nullptr;
> +  gchar **argv;
> +  if (!g_shell_parse_argv(aCommandWithArguments.BeginReading(), NULL, &argv, &error) ||
> +      !argv[0]) {

nit: I guess you should still call g_strfreev even if argv[0] is null?

::: toolkit/system/gnome/nsGIOService.cpp:564
(Diff revision 10)
>        // If the executable is not absolute, get it's full path
> -      char *executable = g_find_program_in_path(g_app_info_get_executable(app_info_from_list));
> +      const char *executable = g_app_info_get_commandline(app_info_from_list);
>  
> -      if (executable && strcmp(executable, PromiseFlatCString(cmd).get()) == 0) {
> +      if (executable &&
> +          strcmp(executable, PromiseFlatCString(aCommandline).get()) == 0) {

Why this change? This code doesn't seem to do what the comment says anymore.

::: toolkit/system/gnome/nsGIOService.cpp:590
(Diff revision 10)
> -  if (!app_info) {
> -    app_info = g_app_info_create_from_commandline(PromiseFlatCString(cmd).get(),
> +  *aAppInfo = nullptr;
> +  return NS_ERROR_NOT_AVAILABLE;
> +}
> +
> +/**
> + * Create application info for specified commandand application name.

Worth noting in the function description that command line arguments are ignored and %u is always added.
Attachment #8836689 - Flags: review?(paolo.mozmail)
Comment on attachment 8836689 [details]
Bug 1297686 - List GIO handlers in protocol handler list in handlers dialog and in preferences,

https://reviewboard.mozilla.org/r/112054/#review200058

Sorry for the delay, I've tested this and it seems to work quite well! There is only one small thing to fix, and there's a high chance that the next revision will be the final one :-)

::: toolkit/system/gnome/nsGIOService.cpp:609
(Diff revision 11)
> +                                   nsIGIOMimeApp**   appInfo)
> +{
> +  GError *error = nullptr;
> +  *appInfo = nullptr;
> +
> +  // Using G_APP_INFO_CREATE_SUPPORTS_URIS caling g_app_info_create_from_commandline

typo: calling

::: uriloader/exthandler/nsHandlerService-json.js:455
(Diff revision 11)
> +      handlerApp = Cc["@mozilla.org/gio-service;1"]
> +                     .getService(Ci.nsIGIOService)
> +                     .createAppFromCommand(handlerObj.command, handlerObj.name);
> +      if (!handlerApp) {
> +        return null;
> +      }

Given the latest changes, this should now be a try-catch block that returns null if an error is raised.

You should update the test to check that this code path works correctly. Just add webHandlerApp as the second of the possible handlers for the MIME type, update the check done before the dummy file has been removed to ensure both applications are visible, and add a new test after the removal checking that only the web application remains.
Attachment #8836689 - Flags: review?(paolo.mozmail)
Comment on attachment 8836689 [details]
Bug 1297686 - List GIO handlers in protocol handler list in handlers dialog and in preferences,

https://reviewboard.mozilla.org/r/112054/#review203328

Thank you! I really appreciate all the effort and the expertise you contributed, this turned out to be a pretty complex patch, also touching some old code that hasn't been updated in a while.
Attachment #8836689 - Flags: review?(paolo.mozmail) → review+
Keywords: checkin-needed
I rebased this on the latest mozilla-central, and it seems that the tryserver builds failed:

toolkit/system/gnome/nsGIOService.cpp:403:50: error: no matching function for call to 'nsIMutableArray::AppendElement(nsCOMPtr<nsIGIOMimeApp>&, bool)'
Flags: needinfo?(jhorak)
Keywords: checkin-needed
Oh, it looks like the fix for bug 1313150 has landed, so my patch bitrot. Sorry for the trouble. Attaching fixed patch which only removes the second parameter from AppendElement call.
Flags: needinfo?(jhorak)
I lost track that this patch was waiting to be checked in. I guess we need a rebased tryserver build now:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b3ef01aeb9a23294504181b725caa78b3c353a5
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9660ff09c221
List GIO handlers in the protocol handler list, in the handlers dialog, and in preferences. r=paolo
Trivial fix, this failed on other platforms due to the rename of "do_print" to "info":

https://treeherder.mozilla.org/#/jobs?repo=try&revision=96bee23877e3ba068cb8ed0bb5303215f169ceb6
Flags: needinfo?(jhorak)
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e35fd376b8b
List GIO handlers in the protocol handler list, in the handlers dialog, and in preferences. r=paolo
https://hg.mozilla.org/mozilla-central/rev/9e35fd376b8b
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: