Bug 940206 makes Navigator.registerProtocolHandler work in e10s, but clicking on links for registered protocols doesn't work. This appears to be because we don't implement nsIHandlerService in child processes. We need a (probably simple) implementation in the child for at least the web handlers. I think that should be enough as nsExternalProtocolHandler is already somewhat e10s-aware, but we'll definitely need to test that.

Comment on attachment 8675164 [details] [diff] [review] handlerService.patch Review of attachment 8675164 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/mime/nsIMIMEInfo.idl @@ +262,5 @@ > + > + > +%{C++ > + typedef nsIHandlerApp* _nsIHandlerApp; > +%} this isn't needed, deleted locally

Comment on attachment 8675164 [details] [diff] [review] handlerService.patch Review of attachment 8675164 [details] [diff] [review]: ----------------------------------------------------------------- I think some of this can be done more simply and there's a bunch of nits to fix as well. ::: dom/ipc/ContentParent.cpp @@ +3889,5 @@ > + actor->AddRef(); > + return actor; > +} > + > +bool ContentParent::DeallocPHandlerServiceParent(PHandlerServiceParent* aHandlerServiceParent) Nit: Return type on its own line. ::: uriloader/exthandler/HandlerServiceChild.h @@ +5,5 @@ > +#include "mozilla/dom/PHandlerInfoChild.h" > +#include "mozilla/dom/PHandlerAppChild.h" > + > +class HandlerServiceChild : public mozilla::dom::PHandlerServiceChild > + , public nsISupports I don't think this needs to implement nsISupports. You should be able to just stick an NS_INLINE_DECL_REFCOUNTING in and remove the ISUPPORTS{0} stuff. Also, might as well mark these classes as 'final'. @@ +16,5 @@ > + private: > + virtual ~HandlerServiceChild() {} > +}; > + > +class HandlerAppChild : public mozilla::dom::PHandlerAppChild { Uber-nit: { on its own line. ::: uriloader/exthandler/HandlerServiceParent.cpp @@ +8,5 @@ > +using mozilla::dom::HandlerInfo; > + > +NS_IMPL_ISUPPORTS0(HandlerServiceParent); > + > +class ProxyHandlerInfo : public nsIHandlerInfo { Maybe put this in an anonymous namespace so the class name doesn't leak out? @@ +248,5 @@ > +} > + > +bool HandlerAppParent::RecvGetName(nsString* rName) { > + nsString name; > + mApp->GetName(name); mApp->GetName(*rName) @@ +287,5 @@ > + > +bool HandlerInfoParent::RecvGetAlwaysAskBeforeHandling(bool* rAsk) > +{ > + bool ask; > + mHandlerInfo->GetAlwaysAskBeforeHandling(&ask); No need for the local. ::: uriloader/exthandler/HandlerServiceParent.h @@ +8,5 @@ > + > +class nsIHandlerApp; > + > +class HandlerServiceParent : public mozilla::dom::PHandlerServiceParent > +, public nsISupports This also probably doesn't need to be nsISupports and should be final. @@ +13,5 @@ > +{ > + public: > + HandlerServiceParent(); > + NS_DECL_ISUPPORTS > + virtual PHandlerInfoParent* AllocPHandlerInfoParent() override; Weird indentation. ::: uriloader/exthandler/PHandlerApp.ipdl @@ +9,5 @@ > + > +sync protocol PHandlerApp { > + manager PHandlerInfo; > + > + parent: Uber-nit: please move this to the 0th column. ::: uriloader/exthandler/PHandlerInfo.ipdl @@ +7,5 @@ > + > +namespace mozilla { > +namespace dom { > + > +sync protocol PHandlerInfo { Why is this a protocol and not a struct with the relevant data? It seems like we could avoid both dealing with the lifetime of these things as well as avoiding a few extra sync messages. ::: uriloader/exthandler/nsContentHandlerService.cpp @@ +93,5 @@ > +NS_IMPL_ISUPPORTS(RemoteHandlerApp, nsIHandlerApp) > + > + > + > +static inline nsIHandlerApp* PHandlerAppChildTonsIHandlerApp(PHandlerAppChild* appChild) { This doesn't seem to add much value. @@ +130,5 @@ > + HandlerInfo info = nsIHandlerInfoToHandlerInfo(aHandlerInfo); > + PHandlerInfoChild *pinfo = mHandlerServiceChild->SendPHandlerInfoConstructor(); > + mHandlerServiceChild->SendFillHandlerInfo(info, nsCString(aOverrideType), pinfo, &pinfo); > + CopyHanderInfoTonsIHandlerInfo(pinfo, aHandlerInfo); > + return NS_ERROR_NOT_IMPLEMENTED; Should this be NS_OK? Who's responsible for deleting pinfo? @@ +155,5 @@ > + return a.Compare(b.Data(), false, a.Length()) < 0; > + } > +}; > + > +std::map<nsCString, nsCString, StringCompare> extToTypeMap; IIRC, global non-POD objects are bad for startup time because their constructors are run pretty early. This should be a member on nsContentHandlerService. This should also use |nsClassHashtable<nsCStringHashKey, nsCString>| as its type for consistency with the rest of Gecko. ::: uriloader/exthandler/nsContentHandlerService.h @@ +4,5 @@ > + {0xc4b6fb7c, 0xbfb1, 0x49dc, {0xa6, 0x5f, 0x03, 0x57, 0x96, 0x52, 0x4b, 0x53}} > + > +namespace mozilla { > + namespace dom { > + class PHandlerServiceChild; Nit: namespace declarations don't add levels of indentation, so this should be: namespace mozilla { namespace dom { class ...; } } @@ +8,5 @@ > + class PHandlerServiceChild; > + } > +} > + > +class nsContentHandlerService : public nsIHandlerService Any reason not to move this into namespace dom and call it ContentHandlerService? @@ +19,5 @@ > + nsresult Init(); > + > +private: > + virtual ~nsContentHandlerService(); > + mozilla::dom::PHandlerServiceChild* mHandlerServiceChild; This should be a RefPtr<HandlerServiceChild> to get rid of the manual refcounting.

Comment on attachment 8682385 [details] [diff] [review] handlerService.patch Review of attachment 8682385 [details] [diff] [review]: ----------------------------------------------------------------- This is really close to being ready. I saw one big thing to fix (sharing some duplicated code) and a couple more nits. ::: uriloader/exthandler/ContentHandlerService.cpp @@ +28,5 @@ > + mHandlerServiceChild = static_cast<HandlerServiceChild*>(cpc->SendPHandlerServiceConstructor()); > + return NS_OK; > +} > + > +static inline HandlerInfo nsIHandlerInfoToHandlerInfo(nsIHandlerInfo* aInfo) This should be shared with the version in HandlerServiceParent.cpp. For ease you could make this a static function on either the child or parent's class. While we're here, you might as well return the HandlerInfo through an outparam (by pointer) since that's the style we use in the rest of Gecko. I think the way that this is written should be fine because of RVO eliding one of the temporaries, but we might as well be consistent. @@ +147,5 @@ > + > + > +NS_IMETHODIMP ContentHandlerService::GetTypeFromExtension(const nsACString & aFileExtension, nsACString & _retval) > +{ > + nsCString* cachedType = NULL; nullptr ::: uriloader/exthandler/HandlerServiceChild.h @@ +6,5 @@ > +class HandlerServiceChild final : public mozilla::dom::PHandlerServiceChild > +{ > + public: > + NS_INLINE_DECL_REFCOUNTING(HandlerServiceChild) > + HandlerServiceChild() {}; Nits: Indentation and no need for the semicolon. ::: uriloader/exthandler/HandlerServiceParent.cpp @@ +43,5 @@ > +} > + > +NS_IMETHODIMP ProxyHandlerInfo::SetPreferredApplicationHandler(nsIHandlerApp *aPreferredApplicationHandler) > +{ > + nsCOMPtr<nsIHandlerApp> app(aPreferredApplicationHandler); No need for this local. @@ +243,5 @@ > + nsCOMPtr<nsIHandlerInfo> info(WrapHandlerInfo(aHandlerInfo)); > + nsCOMPtr<nsIHandlerService> handlerSvc = do_GetService(NS_HANDLERSERVICE_CONTRACTID); > + handlerSvc->Exists(info, exists); > + nsCString type; > + info->GetType(type); Why are these two lines here?

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=16838764&repo=mozilla-inbound

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/453b41d5aef7

this isn't the greenest try run I've ever seen, but all of the oranges seem to be known randoms. https://treeherder.mozilla.org/#/jobs?repo=try&revision=00cc16676137

Comment on attachment 8686041 [details] [diff] [review] handlerService.patch Review of attachment 8686041 [details] [diff] [review]: ----------------------------------------------------------------- A few nits seem to have crept in. I also noticed a few things that can be cleaner. ::: dom/ipc/ContentChild.cpp @@ +1897,5 @@ > > +PHandlerServiceChild* > +ContentChild::AllocPHandlerServiceChild() > +{ > + HandlerServiceChild* actor = new HandlerServiceChild(); Nit: Extra space after =. ::: dom/ipc/ContentParent.cpp @@ +3889,5 @@ > > +PHandlerServiceParent* > +ContentParent::AllocPHandlerServiceParent() > +{ > + HandlerServiceParent* actor = new HandlerServiceParent(); Nit: extra space after =. ::: uriloader/exthandler/HandlerServiceParent.cpp @@ +9,5 @@ > +using mozilla::dom::RemoteHandlerApp; > + > +namespace { > + > + class ProxyHandlerInfo : public nsIHandlerInfo { This block shouldn't be indented. Also might as well make this class final. @@ +15,5 @@ > + explicit ProxyHandlerInfo(const HandlerInfo& aHandlerInfo); > + NS_DECL_ISUPPORTS; > + NS_DECL_NSIHANDLERINFO; > + protected: > + virtual ~ProxyHandlerInfo() {} For all of the classes that are final, the destructor doesn't have to be virtual anymore either (since the class can only be deleted via a pointer to its concrete most-derived type). @@ +25,5 @@ > + NS_IMPL_ISUPPORTS(ProxyHandlerInfo, nsIHandlerInfo) > + > + ProxyHandlerInfo::ProxyHandlerInfo(const HandlerInfo& aHandlerInfo) : mHandlerInfo(aHandlerInfo), mPossibleApps(do_CreateInstance(NS_ARRAY_CONTRACTID)) > + { > + nsTArray<HandlerApp> apps(aHandlerInfo.possibleApplicationHandlers()); Even if you don't want to use the C++11 syntax, it's probably worth avoiding copying this nsTArray by using a const reference. @@ +28,5 @@ > + { > + nsTArray<HandlerApp> apps(aHandlerInfo.possibleApplicationHandlers()); > + for (unsigned int i = 0; i < apps.Length(); i++) { > + HandlerApp happ = apps[i]; > + nsCOMPtr<nsIHandlerApp> app = new RemoteHandlerApp(happ); No need for the single-use temporary. Just do |new RemoteHandlerApp(apps[i])|. Or, this could be: for (auto& happ : aHandlerInfo.possibleApplicationHandlers()) { ... } for full new-C++ style points.

I see bug #1176646 was marked a duplicate of this one, am I right in assuming that bug #1095484 is a duplicate as well, covering the same use case? I ask because in the latest nightly - 46.0a1 (2015-12-29), I still don't see bug #1095484 fixed; it still behaves as described in bug #1095484, comment #10. I'm assuming the code has made it to the nightly builds already, though unsure.

(In reply to Brian Fernandes from comment #17) > bug #1095484 is a duplicate as well Duplicate of what? Bug 1176646 was repeatedly marked as duplicate of irrelevant bugs, and then I finally found the right one. So 1095484 is a different issue.