web-based protocol handlers don't work in certain situations

VERIFIED FIXED in mozilla1.9beta1

Status

Core Graveyard
File Handling
P1
normal
VERIFIED FIXED
10 years ago
a year ago

People

(Reporter: dmose, Assigned: dmose)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

Trunk
mozilla1.9beta1
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [proto] [has patch, r, sr; landing as soon as i re-run tests])

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

10 years ago
In bug 392957, the web based protocol handling code is moving from nsExtProtocolChannel to nsMIMEInfoImpl::LaunchWithWebHandler (some day it will eventually end up on nsWebHandlerApp).  In the process, nsIHandlerInfo.launchWithURI is getting an optional window context parameter that will be used to parent the dialog, and will also be used as the window to load the web-based handler in.  This leaves two problems:

* there is no way for a caller to specify that a web-based handler should be loaded in a new tab / window (as per preferences).  An example caller that wants this is the "Send Link" item in the context menu that drops down from a link.

* many callers still use the deprecated nsIExternalProtocolService.loadUrl which doesn't allow a window context to be passed in.

So we need to modify the API to support creating a new tab / window on the fly, and audit all callers to make sure that they're using it appropriately.
Flags: blocking1.9?
(Assignee)

Comment 1

10 years ago
When fixing and verifying this bug, we also need to ensure that the parent document doesn't get stomped if somebody puts a URI that is handled by a web-handler into an <object> or <img> tag.
(Assignee)

Updated

10 years ago
Whiteboard: [proto]
(Assignee)

Updated

10 years ago
Assignee: nobody → dmose
Priority: -- → P1
(Assignee)

Updated

10 years ago
Blocks: 382019
(Assignee)

Comment 2

10 years ago
Created attachment 282730 [details] [diff] [review]
precursor patch, v2

This is a precursor patch to clean things up in preparation for the actual fix; it will leave the code in a more maintainable / understandable state.  It's a revised version of the patch that originally began in bug 393122; it allows nsLocalHandlerApp to be over-ridden on per-OS basis, and moves the bulk of the work done in nsIHandlerInfo.launchWithURI to now happen in nsIHandlerApp.launchWithURI.  It also fixes bug 382019, and comes with some brand new tests.
Attachment #282730 - Flags: superreview?(bzbarsky)
Attachment #282730 - Flags: review?(cbiesinger)
(Assignee)

Comment 3

10 years ago
(Note that much of the precursor patch is not new code; it's just moving existing code around).
(Assignee)

Comment 4

10 years ago
I don't _think_ this patch is going to break OS2, but it might.
Status: NEW → ASSIGNED
Blocks: 397690
I might be a little laggy getting to this review.  Sorry; just pretty much swamped with things right now.
Comment on attachment 282730 [details] [diff] [review]
precursor patch, v2

sorry for not actually getting to this yesterday, real life intruded

netwerk/mime/public/nsIMIMEInfo.idl
+[scriptable, uuid(f65554fb-858c-4700-988e-41f6832c2f2b)]
 interface nsIHandlerInfo : nsISupports {

shouldn't you change the nsIHandlerApp IID instead?

uriloader/exthandler/nsExternalHelperAppService.cpp	
+    platform_local_handler_app_t *handlerApp(
+      new platform_local_handler_app_t(EmptyString(), aApplication));

I think this would look nicer if written as:
+    platform_local_handler_app_t *handlerApp =
+      new platform_local_handler_app_t(EmptyString(), aApplication);

uriloader/exthandler/nsLocalHandlerApp.h
+   * Launches this application with a single argument (typically either
+   * a file path or a URI spec).  This is meant as a helper method for
+   * implementations of LaunchWithURI/LaunchDefaultWithFile.

well, this class doesn't have a LaunchDefaultWithFile... 

+#ifdef XP_MAC

er... that looks wrong, given that XP_MAC is for mac classic...

uriloader/exthandler/tests/browser/browser_handlerApps.js
+  // if we get this far without an exception, we've passed
+  ok(true, "webHandler launchWithURI test");

what would really be nice is if you verified that the URL was actually loaded (with an onload handler perhaps?)

+    exe.initWithPath("C:\\WINDOWS\\SYSTEM32\\HOSTNAME.EXE");

can you get WinD from the directory service instead? Windows doesn't have to be installed in c:\windows
Attachment #282730 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 7

10 years ago
Created attachment 283445 [details] [diff] [review]
precursor patch, v3

This has all the changes you requested except for renaming to PlatformHandlerApp_t, which I will do before checking in.  The code itself is pretty much the same; re-requesting review for the test stuff, which has changed fairly substantially: from a browserchrome test to a mochitest.
Attachment #282730 - Attachment is obsolete: true
Attachment #283445 - Flags: review?(cbiesinger)
Attachment #282730 - Flags: superreview?(bzbarsky)
Comment on attachment 283445 [details] [diff] [review]
precursor patch, v3

uriloader/exthandler/tests/mochitest/handlerApp.xhtml

use spaces, not tabs :)

+	var location = String(window.location).split('?');

just use location.href instead of the String() thing

in fact, just use location.search (that includes the question mark, but that shouldn't be a problem)


uriloader/exthandler/tests/mochitest/handlerApps.js
+    uri, newWindow.QueryInterface(Components.interfaces.nsIInterfaceRequestor). 
+                   getInterface(Components.interfaces.nsIWebNavigation).
+                   QueryInterface(Components.interfaces.nsIDocShell));

please just use a temporary variable for the window context

+  var osString = Components.classes["@mozilla.org/xre/app-info;1"].
+                 getService(Components.interfaces.nsIXULRuntime).OS;
...
+  var dirSvc = Components.classes["@mozilla.org/file/directory_service;1"]
+                 .getService(Components.interfaces.nsIDirectoryServiceProvider);

please put the dot in a consistent place. (I like to put it on the second line under the dot in Components.classes)

(also for the other xpcom objects in this function)

+    exe.append("iCal.app"); // lingers after the tests finish, but oh well

you can't use the unix codepat? :(

+  var ioService = Components.classes["@mozilla.org/network/io-service;1"].
+    getService(Components.interfaces.nsIIOService);

you already have an ioService variable...

+  // dispose of the running iCal process  

so it doesn't actually linger after the test finishes?

(developers running this test on their machine might get a surprise when their iCal gets killed...)


+// document.addEventListener("load", myOnLoad, false);

what's with that line?

uriloader/exthandler/tests/mochitest/test_handlerApps.xhtml

+RHEET!

valid HTML would require this to be in a <p> or something similar :) (also in handlerApp.xhtml)

+<script class="testbody" type="text/javascript">
+<![CDATA[
+
+
+
+
+]]>
+</script>

that doesn't seem necessary
Attachment #283445 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 9

10 years ago
(In reply to comment #8)
> 
> +    exe.append("iCal.app"); // lingers after the tests finish, but oh well
> 
> you can't use the unix codepat? :(

No, LaunchWithURI on Mac is for talking to application bundles, not just UNIX executables.

> +  // dispose of the running iCal process  
> 
> so it doesn't actually linger after the test finishes?
> 
> (developers running this test on their machine might get a surprise when their
> iCal gets killed...)

Yeah, that's probably worse than having it linger around on the test machines, since there's only ever going to be a single copy of it running on OS X anyway.  I'll comment that code out and include details about why.
(Assignee)

Comment 10

10 years ago
Created attachment 283458 [details] [diff] [review]
precursor patch, v4

Most of this patch is just moving existing code around.  The only two pieces which are actually new code are the tests and nsWebHandlerApp.launchWithURI, which is a JavaScript version of the old C++ nsMIMEInfoBase::LaunchWithWebHandler.
Attachment #283445 - Attachment is obsolete: true
Attachment #283458 - Flags: superreview?(jonas)
Attachment #283458 - Flags: review+
Comment on attachment 283458 [details] [diff] [review]
precursor patch, v4

>Index: netwerk/mime/public/nsIMIMEInfo.idl
>-[scriptable, uuid(b504f39e-d88a-4435-8e0d-e13f1070f7e7)]
>+[scriptable, uuid(8d298761-0963-4c90-99e2-6ea498825e82)]
>+
> interface nsIHandlerApp : nsISupports {

Please remove the empty line.

>Index: uriloader/exthandler/nsLocalHandlerApp.cpp
>+nsLocalHandlerApp::LaunchWithIProcess(const nsCString& aArg)
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsIProcess> process = do_CreateInstance(NS_PROCESS_CONTRACTID, &rv);
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  if (NS_FAILED(rv = process->Init(mExecutable)))
>+    return rv;
>+
>+  const char *string = aArg.get();
>+
>+  PRUint32 pid;
>+  return process->Run(PR_FALSE, &string, 1, &pid);
>+}

Do you need to deal with %00 or ascii nulls or any such cruft in the uri here?

>Index: uriloader/exthandler/nsLocalHandlerApp.h
>+#ifdef XP_MACOSX
>+# ifndef NSLOCALHANDLERAPPMAC_H_
>+#  include "mac/nsLocalHandlerAppMac.h"
>+typedef nsLocalHandlerAppMac PlatformLocalHandlerApp_t;
>+# endif
>+#else 
>+typedef nsLocalHandlerApp PlatformLocalHandlerApp_t;
>+#endif

I still don't understand why you need the inner ifndef here. If nsLocalHandlerAppMac.h is included first it'll define NSLOCALHANDLERAPPMAC_H_ and then include this file, when nsLocalHandlerAppMac.h is then included again it'll do nothing since the NSLOCALHANDLERAPPMAC_H_ is already defined.

The result of the current solution is that PlatformLocalHandlerApp_t won't get typedefed if nsLocalHandlerAppMac.h is included first, which I'm guessing was not the intention?

sr=me with that fixed or explained.
Attachment #283458 - Flags: superreview?(jonas) → superreview+
(Assignee)

Comment 12

10 years ago
> Do you need to deal with %00 or ascii nulls or any such cruft in the uri here?

On UNIX systems, I think we're OK.  On Mac and Windows systems, I'm less sure.  I'll spin off another bug about this, because the code in this patch is exactly equivalent to the existing code in the tree; this patch won't be introducing any new issues.

> I still don't understand why you need the inner ifndef here. If
> nsLocalHandlerAppMac.h is included first it'll define NSLOCALHANDLERAPPMAC_H_
> and then include this file, when nsLocalHandlerAppMac.h is then included again
> it'll do nothing since the NSLOCALHANDLERAPPMAC_H_ is already defined.

The inner ifndef is for the case where we're compiling nsLocalHandlerAppMac.cpp.  It includes nsLocalHandlerAppMac.h which itself includes nsLocalHandlerApp.h before actually defining nsLocalHandlerAppMac.  Since nsLocalHandlerApp.h tries to do the typedef before nsLocalHandlerAppMac is defined, the compiler falls over.

> The result of the current solution is that PlatformLocalHandlerApp_t won't get
> typedefed if nsLocalHandlerAppMac.h is included first, which I'm guessing was
> not the intention?

This is an irrelevant side effect.  PlatformLocalHandlerApp_t is intended to be used by consumers of "nsLocalHandlerApp.h", whereas "nsLocalHandlerAppMac.h" is a platform-specific implementation detail which shouldn't be used by consumers at all.
(Assignee)

Comment 13

10 years ago
Created attachment 283519 [details] [diff] [review]
precursor patch, v5 (checked in)

Final patch for checkin.
Attachment #283458 - Attachment is obsolete: true
Attachment #283519 - Flags: superreview+
Attachment #283519 - Flags: review+
(Assignee)

Comment 14

10 years ago
Comment on attachment 283519 [details] [diff] [review]
precursor patch, v5 (checked in)

Precursor patch checked in.
Attachment #283519 - Attachment description: precursor patch, v5 → precursor patch, v5 (checked in)
(Assignee)

Comment 15

10 years ago
Urgh, I screwed up and checked this in during threat-level orange without realizing that this bug hadn't yet gotten blocking-1.9+.

That said, I can't imagine shipping 1.9 with major use cases of a P1 feature like this busted, so I believe that leaving it in the tree is the right thing.

If people feel strongly that I should back it out; let me know, and I can do that tomorrow.
+# ifndef NSLOCALHANDLERAPPMAC_H_  
+# include "mac/nsLocalHandlerAppMac.h"
+typedef nsLocalHandlerAppMac PlatformLocalHandlerApp_t;
+# endif

another solution would be to just forward-declare nsLocalHandlerAppMac...
also, you still have a tab in uriloader/exthandler/tests/mochitest/handlerApp.xhtml
Flags: blocking1.9? → blocking1.9+
So, did we back this out?  It's listed as a beta blocker.  Please update.
Poke.  Update?  Landing?
(Assignee)

Comment 20

10 years ago
Created attachment 285186 [details] [diff] [review]
fix web-based handlers

This patch makes launchWithURI attempt to find and/or open a new browser window/tab if no window context is passed into a web handler.  I'm going to split the iframe, object, etc. cases out to another bug.
Attachment #285186 - Flags: superreview?(jonas)
Attachment #285186 - Flags: review?(cbiesinger)
(Assignee)

Updated

10 years ago
Whiteboard: [proto] → [proto] [has patch] [needs r, sr]
Comment on attachment 285186 [details] [diff] [review]
fix web-based handlers

(I'm no browser peer, I didn't review the change in browser/)

+        windowMediator.getMostRecentWindow("navigator:browser").

:( this will not work for embeddors

why can't you demand that people pass in a context?

as for the actual code:
+      var browserDOMWin =
+        windowMediator.getMostRecentWindow("navigator:browser").
+        getInterface(Ci.nsIWebNavigation).
+        QueryInterface(Ci.nsIDocShellTreeItem).
+        rootTreeItem.QueryInterface(Ci.nsIInterfaceRequestor).
+        getInterface(Ci.nsIDOMWindow).
+        QueryInterface(Ci.nsIDOMChromeWindow).
+        browserDOMWindow;

Does this not work:
        windowMediator.getMostRecentWindow("navigator:browser")
                      .QueryInterface(Ci.nsIDOMChromeWindow)
                      .browserDOMWindow;

hm, you're making use of javascript's weird scoping rules here... (declaring browserDOMWindow in a block, using it outside of it)
Attachment #285186 - Flags: review?(cbiesinger) → review+
Comment on attachment 285186 [details] [diff] [review]
fix web-based handlers

>+    try {
>+      // get browser dom window
>+      var browserDOMWin =
>+        windowMediator.getMostRecentWindow("navigator:browser").
>+        getInterface(Ci.nsIWebNavigation).
>+        QueryInterface(Ci.nsIDocShellTreeItem).
>+        rootTreeItem.QueryInterface(Ci.nsIInterfaceRequestor).
>+        getInterface(Ci.nsIDOMWindow).
>+        QueryInterface(Ci.nsIDOMChromeWindow).
>+        browserDOMWindow;
>+    } catch (ex) {
>+      // if we got an exception, there are several possible reasons why:
>+      // a) this gecko embedding doesn't provide an nsIBrowserDOMWindow
>+      //    implementation (i.e. doesn't support browser-style functionality),
>+      //    so we need to kick the URL out to the OS default browser.  This is
>+      //    the subject of bug 394479.
>+      // b) this embedding does provide an nsIBrowserDOMWindow impl, but
>+      //    there doesn't happen to be a browser window open at the moment; one
>+      //    should be opened.  It's not clear whether this situation will really
>+      //    ever occur in real life.  If it does, the only API that I can find
>+      //    that seems reasonably likely to work for most embedders is the
>+      //    command line handler.  
>+      // c) something else went wrong 
>+
>+      // it's not clear how one would differentiate between the three cases
>+      // above, so for now we just propagate the exception upwards.
>+      throw ex;
>+    }

This try/catch doesn't do anything for now, and you'll likely have to break up the above expression when you do want to fix the comment, so just remove the try/catch for now. Do leave the comment though.
Attachment #285186 - Flags: superreview?(jonas) → superreview+
(Assignee)

Comment 23

10 years ago
(In reply to comment #21)
> (From update of attachment 285186 [details] [diff] [review])
> (I'm no browser peer, I didn't review the change in browser/)

I'll get a browser peer to do that.

> +        windowMediator.getMostRecentWindow("navigator:browser").
> 
> :( this will not work for embeddors

Well, non-XUL and non-browser embeddors will have problems, yes.  As per the
comments in the catch clause, we'll have to handle that case in one of a few
different ways.

> why can't you demand that people pass in a context?

In the alwaysAsk case, the caller won't know whether a web-based protocol
handler is in use before calling nsIExternalProtocolService.LoadURI.  If the
caller doesn't have a window/tab to put the web-based handler in (e.g.
non-click cases), it will need to open one, which in the case of a local
handler would need to be closed, creating a weird user experience.

I've thought through a bunch of possible ways to change the API to try and
alleviate this problem, but none of them really work, other than having the
front-end pass in an interface with an "open a front end window" method. 
That would be theoretically pure, but makes for an even more complex API.

We could hoist the context creation up a level from inside nsWebHandlerApp.js
to both exthandler and the content-dispatch-chooser, but that doesn't really
buy us much (e.g. we still have a copy of the code in a non-front-end place).

> Does this not work:
>         windowMediator.getMostRecentWindow("navigator:browser")
>                       .QueryInterface(Ci.nsIDOMChromeWindow)
>                       .browserDOMWindow;

Yes, fixed.

> hm, you're making use of javascript's weird scoping rules here... (declaring
> browserDOMWindow in a block, using it outside of it)

function scoping ftw!  :-D
(Assignee)

Comment 24

10 years ago
Created attachment 285891 [details] [diff] [review]
fix web-based handlers, v2 (checked in)

r and sr comments addressed; also fixed up the nsIMIMEInfo doxygen comments to reflect reality.  Still needs rs or r from a browser peer.
Attachment #285186 - Attachment is obsolete: true
Attachment #285891 - Flags: superreview+
Attachment #285891 - Flags: review+
Comment on attachment 285891 [details] [diff] [review]
fix web-based handlers, v2 (checked in)

r=me on the browser.js change.
Attachment #285891 - Flags: review+
(Assignee)

Updated

10 years ago
Whiteboard: [proto] [has patch] [needs r, sr] → [proto] [has patch, r, sr; landing as soon as i re-run tests]
(Assignee)

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Attachment #285891 - Attachment description: fix web-based handlers, v2 → fix web-based handlers, v2 (checked in)
(Assignee)

Comment 26

10 years ago
Patch checked in; Comment 1 has been spun off into bug 400886.
dmose: Can you suggest a way that QA can verify this bug?
(Assignee)

Comment 28

10 years ago
Set up one of the web-based mailto: handlers on <http://people.mozilla.com/~ctalbert/>.

From the context menu on a web link, select "Send Link...", and makes sure it opens the compose page in the appropriate webmail app.  Note that the To: address may actually be bogus (i.e. contain the word "mailto:"), but that's ok, because those test handlers aren't totally complete yet.
verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102504 Minefield/3.0a9pre using the steps suggested by dmose in Comment 28.
Status: RESOLVED → VERIFIED
Attachment 283519 [details] [diff] added:

Index: toolkit/toolkit-makefiles.sh
 MAKEFILES_uriloader="
   uriloader/Makefile
   uriloader/base/Makefile
   uriloader/exthandler/Makefile
   uriloader/exthandler/tests/Makefile
+  uriloader/exthandler/tests/browser/Makefile
 "

but this directory doesn't exist in CVS. Do the tests need to be landed too, or should this change be reverted ?
Blocks: 449763
Blocks: 749872
Depends on: 933303
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.