Blake Kaplan (:mrbkap) (inactive) Reporter
	Description • 9 years ago

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.

	Brad Lassey [:blassey] (use needinfo?) Assignee
	Comment 4 • 9 years ago

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

	Blake Kaplan (:mrbkap) (inactive) Reporter
	Comment 5 • 9 years ago

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.

	Blake Kaplan (:mrbkap) (inactive) Reporter
	Comment 7 • 9 years ago

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?

	Pulsebot
	Comment 9 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6ec673b3b76

	Carsten Book [:Tomcat]
	Comment 10 • 9 years ago

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

	Pulsebot
	Comment 11 • 9 years ago

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

	Brad Lassey [:blassey] (use needinfo?) Assignee
	Comment 12 • 9 years ago

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

	Blake Kaplan (:mrbkap) (inactive) Reporter
	Comment 13 • 9 years ago

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.

	Pulsebot
	Comment 14 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f40e9b62223

	Carsten Book [:Tomcat]
	Comment 15 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6f40e9b62223

	Brian Fernandes
	Comment 17 • 9 years ago

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.

	arni2033
	Comment 18 • 9 years ago

(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.