392957 - Protocol Handling dialog throws exception when web service selected

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[cmtalbert]

When attempting to use a web service to send a mailto link, we get the following exception thrown when clicking on the OK button:
Error: Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIHandlerInfo.launchWithURI]
Source file: chrome://mozapps/content/handling/dialog.js
Line: 205

== Steps To Reproduce ==
1. Go to http://people.mozilla.com/~ctalbert/test-mailto-hotmail.html
2. Click "Allow" on the "Add Protocol Handler Bar"
3. Click the "Test This" link
4. The protocol dialog comes up, and Hotmail is in the list.
5. Select Hotmail and click OK

== Actual results ==
Exception thrown

== Expected Results ==
It should open a compose window in Hotmail (after authenticating) to the [e-mail] address.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

What needs to happen, I think, is that much of the code in nsExternalProtocolHandler::AsyncOpen (as well as code that is called from there) needs to move to nsMIMEInfoImpl::LaunchWithWebHandler.  This will probably require making nsIHandlerInfo::LaunchWithURI to accept some more arguments.  It might or might not make sense waiting for LaunchWith* to move to nsIHandlerApp first.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v1 (work in progress)

This patch moves the code as suggested in the comments, and also changes from using a channel redirect to using the nsIURILoader.openURI.  I still need to do a bunch of cleanup before this is ready for review.

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 278822 [details] [diff] [review]
patch, v1 (work in progress)

>+     * @param aWindowContext  Callbacks provided by the window where the URI
>+     *                        is to be loaded.  Note that if no callbacks are
>+     *                        provided, this will be handed off to the system
>+     *                        default browser (YYY bug #) rather than opened
>+     *                        in the copy of Gecko running in this process.
>+     *       
>      * @throw NS_ERROR_INVALID_ARG if action is not valid for this function.
>      * Other exceptions may be thrown.
>      */
>-    void launchWithURI(in nsIURI aURI);
>+    void launchWithURI(in nsIURI aURI, 
>+                       in nsIInterfaceRequestor aWindowContext);
Should aWindowContext be [optional] then?  Based on the comment, it seems like it should be.

>+    this._windowContext = window.arguments[8];
nit: oh, you are breaking up my pretty alignment of = :(
Perhaps a shorter variable name if you don't want to mess up blame.

>+    if (this._windowContext) {
>+      this._windowContext.QueryInterface(Ci.nsIInterfaceRequestor);
>+    }
nit: no braces around one line if's for toolkit JS.

>   /**
>    * Used to load a URI via an external application. Might prompt the user for
>    * permission to load the external application.
>    *
>    * @param aURI           The URI to load
>-   * @param aWindowContext The parent window to open the dialog with.
>+   *
>+   * @param aWindowContext The parent window to open the dialog with, notify
>+   *                       progress on, etc.  Note that if no callbacks are
>+   *                        provided, this will be handed off to the system
>+   *                        default browser (YYY bug #) rather than opened
>+   *                        in the copy of Gecko running in this process.
>    */
So, Mano has suggested in the past when comments are this long to do it a bit differently:
 * @param aWindowContext
 *        The parent window....
(or, line up the description on a new line with the name of the argument)

</my-$0.02>

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v2

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 279153 [details] [diff] [review]
patch, v2

This fixes the most common case.  There are still some other cases where web handlers won't yet work.  Bugs have been spun off, and are referenced in comments in the patch.

Comment 6

[Boris Zbarsky [:bzbarsky]]

The idl should probably document what sort of "window context" is expected, exactly.  Probably just documenting that it's the sort of thing you'd pass to URILoader, right?

I'm not sure about the changes to no longer send redirect notifications, etc. I'll wait for biesi to review all that stuff before trying to sort out the current behavior and how this patch modifies it.

Comment 7

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

Comment on attachment 279153 [details] [diff] [review]
patch, v2

docshell/base/nsWebShell.cpp
+          return extProtService->LoadURI(aURI, 
+                                         static_cast<nsIInterfaceRequestor *>(this)); 

I'd use an implicit cast here

netwerk/mime/public/nsIMIMEInfo.idl\
+     * @param aURI            The URI to launch this application with
      *
+     * @param aWindowContext 
+     *        The window to parent the dialog against, and, if a web handler

I think it'd look nicer to use the same style for both parameters (ie. put the description on the next line for aURI as well)

uriloader/exthandler/nsIExternalProtocolService.idl	

same comment about comment style (for loadURI)

+  return uriLoader->OpenURI(newChannel, PR_TRUE, aWindowContext);

I believe this should be PR_FALSE; only link clicks should pass PR_TRUE, if I remember this setup correctly. bz, can you confirm?


uriloader/exthandler/nsExternalProtocolHandler.cpp
   OpenURL();
   return NS_ERROR_NO_CONTENT; // force caller to abort.

I wonder if ignoring the return value from OpenURL is a good thing...

Comment 8

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

bz, please take a look at the question for you in the previous comment

Comment 9

[Boris Zbarsky [:bzbarsky]]

At the moment, only link clicks and form submissions pass true for aIsContentPreferred.  For this code, we would ideally pass in the same value as got passed to the URILoader at the start of this load, if we came through the URILoader, and false otherwise.

This is really mostly a concern for non-browser apps, since I believe browser apps tend to ignore that boolean...

Comment 10

[Boris Zbarsky [:bzbarsky]]

So just to make sure I understand:

If I have <iframe src="http://foo/mycontent"> and it returns a type we handle with a web handler we will hit nsExtProtocolChannel::AsyncOpen, call into the URILoader, and have it work because the loadgroup for that channel is the one for the iframe's docshell.

In on the other hand I have an <img> or <object>, the loadgroup will be for the containing document.... so won't this clobber that document with the web app?  Or am I missing something?

I assume we have some tests for this stuff somewhere?

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #6)
> The idl should probably document what sort of "window context" is expected,
> exactly.  Probably just documenting that it's the sort of thing you'd pass to
> URILoader, right?

Good point; I'll fix.

(In reply to comment #10)
> So just to make sure I understand:
> 
> If I have <iframe src="http://foo/mycontent"> and it returns a type we handle

W.r.t. an iframe "returning a type", are you referring to the the mime-type of the content in the iframe?  Note that for the moment, we're only supporting web-based handlers for protocols, not MIME content-types.  However, we do want this code to be used for MIME-types as well, eventually.

> with a web handler we will hit nsExtProtocolChannel::AsyncOpen, call into the
> URILoader, and have it work because the loadgroup for that channel is the one
> for the iframe's docshell.
>
> If on the other hand I have an <img> or <object>, the loadgroup will be for the
> containing document.... so won't this clobber that document with the web app? 
> Or am I missing something?

In either the MIME-type case which I think you're describing above, or the analogous protocol-handler case, you're correct.  This is similar or identical to the work that has been split off into bug 394483, and I think that's probably the right place to address this issue.

> I assume we have some tests for this stuff somewhere?

Not yet, those could come with bug 394483 too, I think.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v3

Review comments addressed.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Ah, yes.   I meant protocol, not type.  And of course at the moment content policy will block most such loads (not in iframes, but most others) because we don't want things popping up in response to the non-internal protocols.  That will need to be fixed at some point...

> This is similar or identical to the work that has been split off into bug
> 394483

Maybe....  The concern I have is clobbering the parent document from something that was never meant to be loaded in that document.  I guess the content policy thing will save us for now, but it's something to watch out for.

I'll try to review this ASAP, but I'm not sure I'll get to it before the M8 freeze.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #13)

> Maybe....  The concern I have is clobbering the parent document from something
> that was never meant to be loaded in that document.  

I understand; I really do think that bug is the right place to address that concern.

> I'll try to review this ASAP, but I'm not sure I'll get to it before the M8
> freeze.

Since this is a blocker for M8, would you be ok with me switching it to a different sr?
 

Comment 15

[Boris Zbarsky [:bzbarsky]]

As long as biesi's given it a thorough once-over, sure.

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 279180 [details] [diff] [review]
patch, v3

sr=me (dmose asked me to look since bz's out on vacation)

a few nits over IRC, but seems good overall.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v4

sr nits addressed; carrying forward r+ and sr+.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Requesting blocking mozilla-1.9 for auto-approval.  Web-based protocol handling, a P1 for Firefox 3, won't work without this.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 279364 [details] [diff] [review]
patch, v4

Risk here is fairly low, as the only thing that could really be broken here is hand off of unknown protocol URIs to apps.  This blocks Firefox 3 feature completeness for M8.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Fix checked in on the trunk:

Checking in docshell/base/nsWebShell.cpp;
/cvsroot/mozilla/docshell/base/nsWebShell.cpp,v  <--  nsWebShell.cpp
new revision: 1.696; previous revision: 1.695
done
Checking in netwerk/mime/public/nsIMIMEInfo.idl;
/cvsroot/mozilla/netwerk/mime/public/nsIMIMEInfo.idl,v  <--  nsIMIMEInfo.idl
new revision: 1.36; previous revision: 1.35
done
Checking in toolkit/mozapps/handling/content/dialog.js;
/cvsroot/mozilla/toolkit/mozapps/handling/content/dialog.js,v  <--  dialog.js
new revision: 1.7; previous revision: 1.6
done
Checking in toolkit/mozapps/handling/src/nsContentDispatchChooser.js;
/cvsroot/mozilla/toolkit/mozapps/handling/src/nsContentDispatchChooser.js,v  <--  nsContentDispatchChooser.js
new revision: 1.3; previous revision: 1.2
done
Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <--  nsExternalHelperAppService.cpp
new revision: 1.339; previous revision: 1.338
done
Checking in uriloader/exthandler/nsExternalProtocolHandler.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalProtocolHandler.cpp,v  <--  nsExternalProtocolHandler.cpp
new revision: 1.46; previous revision: 1.45
done
Checking in uriloader/exthandler/nsIExternalProtocolService.idl;
/cvsroot/mozilla/uriloader/exthandler/nsIExternalProtocolService.idl,v  <--  nsIExternalProtocolService.idl
new revision: 1.13; previous revision: 1.12
done
Checking in uriloader/exthandler/nsMIMEInfoImpl.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsMIMEInfoImpl.cpp,v  <--  nsMIMEInfoImpl.cpp
new revision: 1.67; previous revision: 1.66
done
Checking in uriloader/exthandler/nsMIMEInfoImpl.h;
/cvsroot/mozilla/uriloader/exthandler/nsMIMEInfoImpl.h,v  <--  nsMIMEInfoImpl.h
new revision: 1.37; previous revision: 1.36
done
Checking in uriloader/exthandler/mac/nsMIMEInfoMac.cpp;
/cvsroot/mozilla/uriloader/exthandler/mac/nsMIMEInfoMac.cpp,v  <--  nsMIMEInfoMac.cpp
new revision: 1.11; previous revision: 1.10
done
Checking in uriloader/exthandler/mac/nsMIMEInfoMac.h;
/cvsroot/mozilla/uriloader/exthandler/mac/nsMIMEInfoMac.h,v  <--  nsMIMEInfoMac.h
new revision: 1.11; previous revision: 1.10
done
Checking in uriloader/exthandler/os2/nsMIMEInfoOS2.cpp;
/cvsroot/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp,v  <--  nsMIMEInfoOS2.cpp
new revision: 1.9; previous revision: 1.8
done
Checking in uriloader/exthandler/os2/nsMIMEInfoOS2.h;
/cvsroot/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.h,v  <--  nsMIMEInfoOS2.h
new revision: 1.10; previous revision: 1.9
done