384374 - refactor nsIMIMEInfo & nsExternalHelperAppService to support protocol handling

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

This has turned out to be big enough that I'm spinning it off into a separate bug.  This is the stuff that I've been discussing with biesi on and off recently, should have a patch soon.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: helper app rdf refactoring patch, v1

This patch does the minimal set of things to make the nsExternalHelperAppService deal with local and remote protocol handlers in mimeTypes.rdf.  That is:

a) A base nsIHandlerInfo interface is factored out of nsIMIMEInfo which contains things common to both MIME and protocol handlers.
b) preferredApplication along with associated description, executable file, and uriTemplate attributes are factored off into nsIHandlerApp and children
c) support for protocol scheme handler info is added into the re-factored RDF code
d) nsWebProtocolRedirect is renamed to nsProtocolRedirect in preparation for the day when it handles both local and remote protocols; we also now get the relevant configuration info from RDF via nsIHandlerInfo rather than from a preference.
e) a number of methods in nsExternalHelperAppService whose names start with Get but are not actually getters are renamed in order to reduce confusion.

Things I need to do before checking in: 

1) fix up platform-specific code for platforms other than Mac once biesi comments on the existing patch
2) file a bug on the "wrong document channel" warning I've been seeing lately

After this lands, subsequent patches (either here or in another bug) will 

A) replace nsIHandlerApp with an array of apps and tweak the RDF to match
B) rationalize the handler app launching strategy across all handlers

Comment 2

[Shawn Wilsher :sdwilsh]

You have nsIHandlerApp, nsILocalHandlerApp, and nsIWebHandlerApp, but the latter two do not inherit from nsIHandlerApp.  Why is this?

Also, you've got places with tabs in this patch :)

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #2)
> You have nsIHandlerApp, nsILocalHandlerApp, and nsIWebHandlerApp, but the
> latter two do not inherit from nsIHandlerApp.  Why is this?

It makes the C++ inheritance messy.  It turned out to be not trivial to fix this (though it might be possible), and since the IDL inheritance doesn't really give any concrete benefit, I just punted.  I'll add a comment to the IDL explaining.

> Also, you've got places with tabs in this patch :)

Will fix.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: helper app rdf refactoring patch, v2

Fixes the stuff sdwilsh pointed out, tweaks the windows-specific code to build and work, and adds a bit of interface commentary.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: helper app rdf refactoring patch, v3

Updated to remove a spurious change, and fix a mis-cased include file that kept Linux from building.

Comment 6

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

minor comments: (real review tomorrow)

+++ netwerk/mime/public/nsIMIMEInfo.idl	20 Jun 2007 01:42:27 -0000
+ * nsIHandlerInfo gives access to the information about how a given protocol
+ * scheme or MIME-type is handled.
  */

While you're touching this comment, wanna move it to right before the
[scriptable, ...] line? That way, tools like doxygen can tell what it belongs
to.

+     * Returns a nsIFile that points to the application the user has said
+     * they want associated with this content type. This is not always
+     * guaranteed to be set!!
+     */
+    attribute nsIHandlerApp preferredApplicationHandler;

It's not actually an nsIFile...

Comment 7

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

Thanks for also updating the SeaMokey code.

+++ embedding/components/ui/progressDlg/nsProgressDialog.js	20 Jun 2007 01:39:47 -0000
+            if ( this.MIMEInfo && 
+                 this.MIMEInfo.preferredApplicationHandler.executable ) {
+                var appName = 
+                  this.MIMEInfo.preferredApplicationHandler.executable.leafName;
 
Couldn't preferredApplicationHandler be null here? (note also that the IDL
says it can be null)

+++ netwerk/mime/public/nsIMIMEInfo.idl	20 Jun 2007 01:42:27 -0000
+ * nsIMIMEInfo extends nsIHandleInfo with a bunch of information specific to

typo here (nsIHandle_r_Info)

+     * Indicates whether a default application handler exists,
+     * i.e. whether launchWithFile with action = useSystemDefault is possible
+     * and applicationDescription will contain usable information.
+     */
+    readonly attribute boolean hasDefaultHandler;

The comment was previously wrong too, but can you nonetheless replace
applicationDescription with defaultDescription here?


Perhaps equals should also move to nsIHandlerInfo?


Does it really make sense to have launchWithFile on nsIHandlerInfo? It doesn't
seem useful for protocol handlers.

+++ toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in	20 Jun 2007 01:43:55 -0000
+        otherHandler.setAttribute("path",
+        						  this.getPath(this.chosenApp.executable));

Tabs :)

+		var localHandlerApp = 
+          Components.classes["@mozilla.org/uriloader/local-handler-app;1"].
+          createInstance(Components.interfaces.nsILocalHandlerApp);

Tabs again

-#ifdef MOZ_RDF
+#ifdef MOZ_RDF 

Please don't add trailing whitespace


+++ uriloader/exthandler/nsExternalHelperAppService.cpp	20 Jun 2007 01:44:10 -0000
+                                      NS_STATIC_CAST(nsIHandlerInfo *,
+                                                     aMIMEInfo));

Do you need the explicit cast here?

+  // applications which we might need later.  For now, just use nsMIMEInfoImpl
+  // instead of implementating a separate nsIHandlerInfo object.

So you can actually QI protocol handlerinfos to nsIMIMEInfo? That doesn't seem
ideal, but ok. Would be good to fix this eventually...

+  nsresult rv = CallQueryInterface(mimeInfo, aHandlerInfo); 

No need to use QI for this. Just do:
  NS_ADDREF(*aHandlerInfo = mimeInfo);

+  return FillProtoInfoForSchemeFromDS(aScheme, *aHandlerInfo);

You potentially return a failure code here, but set the out parameter. (but
that this is just an internal helper function... so that's ok. just noting
this.)

+  const nsACString& aContentType, nsIMIMEInfo * aMIMEInfo )

While you're touching this line, please remove the space before the )

+++ uriloader/exthandler/nsHandlerAppImpl.cpp	20 Jun 2007 01:44:12 -0000
+  mName.Assign(aName);
+    aName = mName;

Is there a reason why one of these uses Assign while the other has operator=?

+}
\ No newline at end of file

Please fix :)

+++ uriloader/exthandler/nsHandlerAppImpl.h	20 Jun 2007 01:44:12 -0000
+  nsHandlerAppBase(const PRUnichar *aName) NS_HIDDEN { mName.Assign (aName); };
+  nsHandlerAppBase(const nsAString & aName) NS_HIDDEN { mName.Assign(aName); };

I'd have added another space before the { instead of adding one before the (

+  // overriding to keep old caching behavior
+  NS_IMETHOD GetName(nsAString & aName);

You mean so that you can get a useful name even if none was set
explicitly?

+++ uriloader/exthandler/mac/nsMIMEInfoMac.cpp	20 Jun 2007 01:44:14 -0000
+    rv = localHandlerApp->GetExecutable(&application);

Please don't add memory leaks :)

Comment 8

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

Comment on attachment 269168 [details] [diff] [review]
helper app rdf refactoring patch, v3

does camino need changes too?

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

All comments addressed; a few specific responses:

(In reply to comment #7)
> Thanks for also updating the SeaMokey code.

You're welcome.

> Couldn't preferredApplicationHandler be null here? (note also that the IDL
> says it can be null)

Ah, good catch; thanks!

> Perhaps equals should also move to nsIHandlerInfo?

Indeed; Shawn pointed out a case where this would be required today.  However, that entrains at least one other change, and we don't need it _quite_ yet, so I'd like to do it in one of the upcoming patches.

> Does it really make sense to have launchWithFile on nsIHandlerInfo? It doesn't
> seem useful for protocol handlers.

Current thinking is to change this to launchWithURI in an upcoming patch.

> Do you need the explicit cast here?

Good catch; I don't!

> +  // applications which we might need later.  For now, just use nsMIMEInfoImpl
> +  // instead of implementating a separate nsIHandlerInfo object.
> 
> So you can actually QI protocol handlerinfos to nsIMIMEInfo? That doesn't seem
> ideal, but ok. Would be good to fix this eventually...

Yes, agreed.  I'll file a bug on this at checkin.
 
> +  return FillProtoInfoForSchemeFromDS(aScheme, *aHandlerInfo);
> 
> You potentially return a failure code here, but set the out parameter. (but
> that this is just an internal helper function... so that's ok. just noting
> this.)

My understanding has been that, even in regular XPCOM methods, if you don't return a failure code, there is no particular guarantee made about what, if anything, an out param contains. 
 
> +++ uriloader/exthandler/nsHandlerAppImpl.cpp   20 Jun 2007 01:44:12 -0000
> +  mName.Assign(aName);
> +    aName = mName;
> 
> Is there a reason why one of these uses Assign while the other has operator=?

One piece of the code was moved from elsewhere, the other one I wrote myself.  I've changed them to both use Assign.
 
> +  // overriding to keep old caching behavior
> +  NS_IMETHOD GetName(nsAString & aName);
> 
> You mean so that you can get a useful name even if none was set
> explicitly?

Correct.  I've modified the comment to clarify.
 
> +++ uriloader/exthandler/mac/nsMIMEInfoMac.cpp  20 Jun 2007 01:44:14 -0000
> +    rv = localHandlerApp->GetExecutable(&application);
> 
> Please don't add memory leaks :)

Whoops!  Fixed by changing the raw pointer to an nsCOMPtr.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #8)
> (From update of attachment 269168 [details] [diff] [review])
> does camino need changes too?

Possibly; I'll post to m.d.platform to give them and other embedders fair warning.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: helper app rdf refactoring patch, v4

Comment 12

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

>My understanding has been that, even in regular XPCOM methods, if you don't
>return a failure code, there is no particular guarantee made about what, if
>anything, an out param contains. 

Right, so if the caller follows that it won't free the object you put into the out param which means it'll be leaked.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 269459 [details] [diff] [review]
helper app rdf refactoring patch, v4

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp
>@@ -782,87 +784,110 @@ nsresult nsExternalHelperAppService::Fil
>+nsresult nsExternalHelperAppService::FillContentHandlerProperties(
>+  const char * aContentType, const char * aTypeNodePrefix,
>+  nsIRDFService * aRDFService, nsIHandlerInfo * aHandlerInfo)
> {
...
>   if (externalAppNodeResource)
>   {
>-    FillLiteralValueFromTarget(externalAppNodeResource, kNC_PrettyName, &stringValue);
>-    if (stringValue)
>-      aMIMEInfo->SetApplicationDescription(nsDependentString(stringValue));
>- 
>+    const PRUnichar * appName;
>+    FillLiteralValueFromTarget(externalAppNodeResource, kNC_PrettyName,
>+                               &appName);

Isn't appName leaked here? It looks like it would be. See http://lxr.mozilla.org/mozilla/source/rdf/base/src/nsRDFService.cpp#553

Same for the other calls to FillLiteralValueFromTarget.

>+nsresult nsExternalHelperAppService::FillProtoInfoForSchemeFromDS(
>+    const nsACString& aType, nsIHandlerInfo * aHandlerInfo)
...
>+  // Build uri for the handlertype resource.
>+  nsCAutoString typeNodeName(NC_SCHEME_NODE_PREFIX);
>+  nsCAutoString type(aType);
>+  ToLowerCase(type);
>+  typeNodeName.Append(type);

Better written as

nsCAutoString typeNodeName;
ToLowerCase(aType, typeNodeName);
typeNodeName.Insert(NS_LITERAL_STRING(NC_SCHEME_NODE_PREFIX), 0);

>Index: uriloader/exthandler/nsHandlerAppImpl.h

>+class nsLocalHandlerApp : public nsHandlerAppBase, public nsILocalHandlerApp
...
>+  nsLocalHandlerApp() : nsHandlerAppBase() {};
...
>+  virtual ~nsLocalHandlerApp() {};

No need to explicitly write these empty dtors and ctors. The compiler will generate them anyway. Same with the other classes in this file.

>+  nsLocalHandlerApp(const PRUnichar *aName, nsIFile *aExecutable) 
>+    : nsHandlerAppBase(aName) {
>+      mExecutable = aExecutable;
>+    }
>+
>+  nsLocalHandlerApp(const nsAString & aName, nsIFile *aExecutable) 
>+    : nsHandlerAppBase(aName) { 
>+      mExecutable = aExecutable;
>+    }

Initialize mExecutable in the colon-list instead, like so:

  nsLocalHandlerApp(const nsAString & aName, nsIFile *aExecutable) 
    : nsHandlerAppBase(aName),
      mExecutable(aExecutable)
    { }

Same thing for mUriTemplate in the next class.

>Index: uriloader/exthandler/win/nsMIMEInfoWin.cpp
> nsMIMEInfoWin::GetProperty(const nsAString& aName, nsIVariant* *_retval)
> {
>   nsresult rv = NS_ERROR_FAILURE;
>   if (mDefaultApplication && aName.EqualsLiteral(PROPERTY_DEFAULT_APP_ICON_URL))
>     rv = GetIconURLVariant(mDefaultApplication, _retval);
>-  else if (mPreferredApplication && aName.EqualsLiteral(PROPERTY_CUSTOM_APP_ICON_URL))
>-    rv = GetIconURLVariant(mPreferredApplication, _retval);
>+  else if (mPreferredApplication && 
>+           aName.EqualsLiteral(PROPERTY_CUSTOM_APP_ICON_URL)) {
>+      nsCOMPtr<nsILocalHandlerApp> localHandler = 
>+        do_QueryInterface(mPreferredApplication, &rv);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    
>+      nsCOMPtr<nsIFile> executable;
>+      rv = localHandler->GetExecutable(getter_AddRefs(executable));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      rv = GetIconURLVariant(executable, _retval);
>+  }
>   return rv;
> }

You don't seem to be following the indentation pattern as the rest of the function. (4 instead of 2). I think this would also be easier to read if you put {} around all if-statements. 

Oh,, I hate functions that end with |return rv|. Could you change the if-statements to return early instead and end the function with a |return NS_ERROR_FAILURE|

sr=me with that.

Comment 14

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

the default dtor is nonvirtual, so it is necessary to explicitly write the empty destructor here.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

How does having an explicit empty dtor change anything? The same non-trivial code will be instantiated anyway.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, you said non-virtual, not non-trivial :) Ok, makes sense

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #12)
> Right, so if the caller follows that it won't free the object you put into the
> out param which means it'll be leaked.

Whoops; sure enough!  Fixed.

(In reply to comment #13)
> >+    const PRUnichar * appName;
> >+    FillLiteralValueFromTarget(externalAppNodeResource, kNC_PrettyName,
> >+                               &appName);
> 
> Isn't appName leaked here? It looks like it would be. See
> http://lxr.mozilla.org/mozilla/source/rdf/base/src/nsRDFService.cpp#553

No, FillLiteralValueFromTarget doesn't call GetValue; it calls GetValueConst, which is [shared].

> No need to explicitly write these empty dtors and ctors. The compiler will
> generate them anyway. Same with the other classes in this file.

OK, as per your discussion with biesi, I blew away all the empty ctors but left the empty (virtual) dtors.

> You don't seem to be following the indentation pattern as the rest of the
> function. (4 instead of 2). 

Doh; betrayed by my editor!  Fixed.

> I think this would also be easier to read if you put {} around all 
> if-statements. 

Done.
 
> Oh,, I hate functions that end with |return rv|. Could you change the
> if-statements to return early instead and end the function with a |return
> NS_ERROR_FAILURE|

OK, I've made that change here.  What in particular makes you dislike that style?

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It's too easy to accidentally blow away the contents of rv so I dislike keeping it around for too long basically. What can happen is if you insert code between something that sets rv and the final |return rv| that code can either blow away an error code, or accidentally return an error-code even though that error was handled. Like

rv = doSuccessful();
...
rv = doFailing();
if (NS_FAILED(rv)) {
  recover();
}
...
return rv;

where the code between the '...' is the new code.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: helper app rdf refactoring patch, v5

This addresses the various sr comments from the last round, except that I had to put the empty ctors back, as gcc 4.0.1 on Tiger was simply failing to build rather than generating defaults.

It also make the nsExtProtocolChannel into more of a real channel in the case where we're handing off to a web handler: the main change is that we add and remove ourselves from the loadgroup in this case and use LOAD_REPLACE in order to avoid docloader assertions.

Comment 20

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

oh right, you also had non-default constructors. you only get a default constructor if you don't define any others.

Comment 21

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

Comment on attachment 270654 [details] [diff] [review]
helper app rdf refactoring patch, v5

- I think AsyncFinish is a bad name for a function that is not actually asynchronous... I'd just call it Finish

- When opening the new channel succeeds, you should not call the stream listener functions for the old channel. 

(the HTTP code doesn't either; it nulls out mListener/mContext before those functions would be called)

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: refactor the helper app rdf, v6

Renamed that method to Finish, and we now only call the listeners in the failure case.

Comment 23

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

Comment on attachment 270778 [details] [diff] [review]
refactor the helper app rdf, v6

but you still have:
  mOriginalChannel->Finish(NS_BINDING_REDIRECTED);

which is a failure code, and you shouldn't call the listener methods here.


and don't checkin the client.mk part of this patch ;)

Comment 24

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

Comment on attachment 270778 [details] [diff] [review]
refactor the helper app rdf, v6

r+sr=biesi if you make Finish also check for NS_BINDING_REDIRECTED in addition to the NS_FAILED

Comment 25

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

or instead of

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: refactor the helper app rdf, v7 (checked in)

This is the final trunk-based patch that got checked in.

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

> Things I need to do before checking in: 
> 
> 1) fix up platform-specific code for platforms other than Mac once biesi
> comments on the existing patch

Done.

> 2) file a bug on the "wrong document channel" warning I've been seeing lately

This turned out out to be an assertion, not a warning, so it got fixed before checkin.
 
> After this lands, subsequent patches (either here or in another bug) will 
> 
> A) replace nsIHandlerApp with an array of apps and tweak the RDF to match

This is bug 385740.

> B) rationalize the handler app launching strategy across all handlers

This is bug 386078.

Comment 28

[Jeff Walden [:Waldo]]

Testing is/was handled in separate bugs, since theoretically this doesn't add anything new, as I understand it.