388388 - differentiate between MIME and protocol handler nsIHandlerInfos

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, enhancement)

Description

[Myk Melez [:myk] [@mykmelez]]

It should be possible to determine which nsIHandlerInfo objects represent MIME handlers and which represent protocol handlers.  Per discussion in md.platform <http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/8f748d3ba840f685>, we could do this by adding an interface like nsIProtocolInfo to which only protocol handlers can be QIed and then make only MIME handlers QIable to nsIMIMEInfo.

On a related note, it should be possible to obtain the scheme from a protocol handler object.  We could do this either by adding a "scheme" property to nsIProtocolInfo or by adding a "type" property to nsIHandlerInfo which is set to the scheme for protocol handlers and to the MIME type for MIME handlers.

If we take the latter approach, we might want to keep nsIMIMEInfo::MIMEType around for backwards-compatibility, although the two properties should share storage.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: minimal fix that provides this functionality

Here's a patch that implements the minimal fix for this bug.  It adds a "type" attribute to nsIHandlerInfo which contains either the MIME type or the scheme depending on whether the object represents a MIME handler or a protocol handler.

The patch then disables QI to nsIMIMEInfo for objects representing protocol handlers.  It deprecates but does not remove the nsIMIMEInfo::MIMEType attribute to reduce the complexity of the patch and minimize the risk of unforeseen regressions.

nsIHandlerService gets updated to also be able to store protocol handlers, so you have to apply the patch for bug 387930 if you're testing this on the trunk; no need to apply the patch if you're testing on the branch, though, as it's already been checked in to the branch.

Note that I haven't added more tests for nsIHandlerService with a protocol handler, however, since there's currently no way to retrieve an arbitrary nsIHandlerInfo for a protocol handler from JS.  Perhaps bug 385065 will expose nsExternalHelperAppService::GetProtocolHandlerInfo through the nsIExternalProtocolService, solving this problem?

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Err, when I said that perhaps bug 385065 will expose
nsExternalHelperAppService::GetProtocolHandlerInfo through the
nsIExternalProtocolService, I meant that perhaps bug 388701 will do so.

Comment 3

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

Comment on attachment 272896 [details] [diff] [review]
patch v1: minimal fix that provides this functionality

you need to update the IID of nsIHandlerInfo

+// we include an unused parameter in the list.

how about instead of doing that, you make an
enum HandlerType {
  eMIMEInfo,
  eProtocolInfo
};
and use that to differentiate between protocols and mime infos?

could you include the handlerservice changes in bug 387930's patch instead of the one here?

Comment 4

[Myk Melez [:myk] [@mykmelez]]

> you need to update the IID of nsIHandlerInfo

Oops, ok, will do.


> +// we include an unused parameter in the list.
> 
> how about instead of doing that, you make an
> enum HandlerType {
>   eMIMEInfo,
>   eProtocolInfo
> };
> and use that to differentiate between protocols and mime infos?

Hmm, I'll look into that.


> could you include the handlerservice changes in bug 387930's patch instead of
> the one here?

That would be complicated, since the handler service changes in this bug rely on the additional functionality we're adding here to handler info objects.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: addresses review comments

(In reply to comment #3)
> (From update of attachment 272896 [details] [diff] [review])
> you need to update the IID of nsIHandlerInfo

Ok, updated in this patch.


> +// we include an unused parameter in the list.
> 
> how about instead of doing that, you make an
> enum HandlerType {
>   eMIMEInfo,
>   eProtocolInfo
> };
> and use that to differentiate between protocols and mime infos?

dmose seems headed towards separate classes (nsMIMEInfo, nsProtocolInfo) in the long run, but in the short run, enum seems less hacky than an aUnused parameter, so this patch uses an enum instead.

Note that I use the name HandlerClass instead of HandlerType to avoid confusion with the "type" property, and I haven't modified all instantiators of nsMIMEInfoImpl to use the new constructor to keep this patch small and minimize the risk of regression.


> could you include the handlerservice changes in bug 387930's patch instead of
> the one here?

I moved as much of that code into bug 387930's patch as I could.  That patch is now checked in, and this patch just puts the finishing touches on that work.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: resolves conflicts with bug 388701

Here's an update that resolves the conflict with the patch for bug 388701 that was checked in recently.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 273454 [details] [diff] [review]
patch v3: resolves conflicts with bug 388701

>      * The MIME type of this MIMEInfo.
>      * 
>      * @return String representing the MIME type.
>+     * 
>+     * @deprecated  use nsIHandlerInfo::type instead.

Can you file a separate |helpwanted| bug to go actually kill the original usage rather than just leaving it as deprecated?

Looks great otherwise.

r=dmose

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #7)
> >+     * @deprecated  use nsIHandlerInfo::type instead.
> 
> Can you file a separate |helpwanted| bug to go actually kill the original usage
> rather than just leaving it as deprecated?

Yup, filed as bug 389377.

Comment 9

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

Comment on attachment 273454 [details] [diff] [review]
patch v3: resolves conflicts with bug 388701

Please change the type of aClass to HandlerClass instead of int.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: int aClass -> HandlerClass aClass

This is the version of the patch I'll check in.  I just changes the type of aClass from int to HandlerClass, per biesi's review comment.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Patch checked in to the trunk:

Checking in netwerk/mime/public/nsIMIMEInfo.idl;
/cvsroot/mozilla/netwerk/mime/public/nsIMIMEInfo.idl,v  <--  nsIMIMEInfo.idl
new revision: 1.29; previous revision: 1.28
done
Checking in uriloader/exthandler/nsHandlerService.js;
/cvsroot/mozilla/uriloader/exthandler/nsHandlerService.js,v  <--  nsHandlerService.js
new revision: 1.6; previous revision: 1.5
done
Checking in uriloader/exthandler/nsMIMEInfoImpl.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsMIMEInfoImpl.cpp,v  <--  nsMIMEInfoImpl.cpp
new revision: 1.59; previous revision: 1.58
done
Checking in uriloader/exthandler/nsMIMEInfoImpl.h;
/cvsroot/mozilla/uriloader/exthandler/nsMIMEInfoImpl.h,v  <--  nsMIMEInfoImpl.h
new revision: 1.30; previous revision: 1.29
done
Checking in uriloader/exthandler/beos/nsMIMEInfoBeOS.h;
/cvsroot/mozilla/uriloader/exthandler/beos/nsMIMEInfoBeOS.h,v  <--  nsMIMEInfoBeOS.h
new revision: 1.4; previous revision: 1.3
done
Checking in uriloader/exthandler/beos/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/beos/nsOSHelperAppService.cpp,v  <--  nsOSHelperAppService.cpp
new revision: 1.19; previous revision: 1.18
done
Checking in uriloader/exthandler/mac/nsMIMEInfoMac.h;
/cvsroot/mozilla/uriloader/exthandler/mac/nsMIMEInfoMac.h,v  <--  nsMIMEInfoMac.h
new revision: 1.5; previous revision: 1.4
done
Checking in uriloader/exthandler/mac/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/mac/nsOSHelperAppService.cpp,v  <--  nsOSHelperAppService.cpp
new revision: 1.49; previous revision: 1.48
done
Checking in uriloader/exthandler/os2/nsMIMEInfoOS2.h;
/cvsroot/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.h,v  <--  nsMIMEInfoOS2.h
new revision: 1.5; previous revision: 1.4
done
Checking in uriloader/exthandler/tests/unit/test_handlerService.js;
/cvsroot/mozilla/uriloader/exthandler/tests/unit/test_handlerService.js,v  <--  test_handlerService.js
new revision: 1.6; previous revision: 1.5
done
Checking in uriloader/exthandler/unix/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/unix/nsOSHelperAppService.cpp,v  <--  nsOSHelperAppService.cpp
new revision: 1.64; previous revision: 1.63
done
Checking in uriloader/exthandler/win/nsMIMEInfoWin.h;
/cvsroot/mozilla/uriloader/exthandler/win/nsMIMEInfoWin.h,v  <--  nsMIMEInfoWin.h
new revision: 1.5; previous revision: 1.4
done
Checking in uriloader/exthandler/win/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/win/nsOSHelperAppService.cpp,v  <--  nsOSHelperAppService.cpp
new revision: 1.69; previous revision: 1.68
done

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

There was a thinko in nsMIMEInfoWin.h where the constructor called nsMIMEInfoImpl's constructor instead of nsMIMEInfoBase's constructor.  Fixing this cured tree redness, but left us with not-yet-understood orange where tests in netwerk were failing in the external helper app service with QI assertion (and maybe other problems).  Since we weren't able to getting sufficient traction on that after working on it for a while, we had to back out.  Re-opening.

Comment 13

[[e-mail]]

(In reply to comment #12)
>tests in netwerk were failing in the external helper app service with QI assertion
nsExternalHelperAppService::GetTypeFromExtension calls nsOSHelperAppService::GetMIMEInfoFromOS with an empty MIME type. If there is no MIME type in the registry for the extension (.js in my case), the latter returns an nsIMIMEInfo with no MIME type which then fails the NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIMIMEInfo, !mMIMEType.IsEmpty()) test...

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v5: don't assume the existence of mMIMEType; use mClass instead

(In reply to comment #13)
> nsExternalHelperAppService::GetTypeFromExtension calls
> nsOSHelperAppService::GetMIMEInfoFromOS with an empty MIME type. If there is
> no MIME type in the registry for the extension (.js in my case), the latter
> returns an nsIMIMEInfo with no MIME type which then fails the
> NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIMIMEInfo, !mMIMEType.IsEmpty()) test...

Hmm, in that case here's a patch that doesn't assume the existence of mMIMEType for MIME handlers.  Instead, it sets a new mClass member variable to an instance's handler class upon construction and then uses that variable to determine whether or not the instance implements the nsIMIMEInfo interface.

I compiled and tested this patch on Mac, and the mochitests all passed.  I compiled on Windows but couldn't test because of a problem in MozillaBuild (or perhaps just my installation of it) that causes xpcshell to hang when invoked from the MozillaBuild terminal <http://groups.google.com/group/mozilla.dev.builds/browse_thread/thread/1cabb967172cb2a7>.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 273768 [details] [diff] [review]
patch v5: don't assume the existence of mMIMEType; use mClass instead

>+    NS_INTERFACE_MAP_ENTRY(nsIHandlerInfo)
>+    // This is only an nsIMIMEInfo if it's a MIME handler with a MIME type.
>+    NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIMIMEInfo, mClass == eMIMEInfo)

The above comment is no longer correct.

>+    void SetType(const nsACString & aType) { mType = aType; }

This method doesn't appear to actually be used.  Unless you foresee needing it soon, maybe remove it?

We should also test the flash game regression that this is said to have caused before landing.

r=dmose with that stuff addressed

Nice work nailing this down, guys!

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Doing 'make -C netwerk/test/httpserver' and then 'make -C netwerk/test/httpserver check' with this patch is crashing during the destruction of nsMIMEInfoWin as the mDefaultApplication pointer is attempting to release the raw pointer, which is bogus (0x00000007).  Investigating...

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

So nsMIMEInfoWin uses NS_IMPL_ISUPPORTS_INHERITED1 and inherits from nsMIMEInfoBase which now uses table-driven QI.  I suspect this is where the problem lies.

Comment 18

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

Comment on attachment 273768 [details] [diff] [review]
patch v5: don't assume the existence of mMIMEType; use mClass instead

Can you remove mMIMEType now, replacing its uses with mType?

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

False alarm; clobbering some key directories made the crash go away.  I'm running the mochitests on Windows now.

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v6: review comment fixes

(In reply to comment #15)
> (From update of attachment 273768 [details] [diff] [review])
> >+    NS_INTERFACE_MAP_ENTRY(nsIHandlerInfo)
> >+    // This is only an nsIMIMEInfo if it's a MIME handler with a MIME type.
> >+    NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIMIMEInfo, mClass == eMIMEInfo)
> 
> The above comment is no longer correct.

Ah, right.  I've corrected it.


> >+    void SetType(const nsACString & aType) { mType = aType; }
> 
> This method doesn't appear to actually be used.  Unless you foresee needing it
> soon, maybe remove it?

Good point.  I've removed it.


> We should also test the flash game regression that this is said to have caused
> before landing.

I tested it, and it doesn't crash, but then it didn't crash for me with the patch that caused crashes for others, so I'm not sure how useful my results are.


(In reply to comment #18)
> (From update of attachment 273768 [details] [diff] [review])
> Can you remove mMIMEType now, replacing its uses with mType?

We could, but that would make this patch a lot larger and potentially riskier.  Given the problems we've already run into, and the complexity of this code, I think we're probably better doing that in a separate patch.

Comment 21

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v7: removes mMIMEType

> (In reply to comment #18)
> > (From update of attachment 273768 [details] [diff] [review] [details])
> > Can you remove mMIMEType now, replacing its uses with mType?
> 
> We could, but that would make this patch a lot larger and potentially riskier. 
> Given the problems we've already run into, and the complexity of this code, I
> think we're probably better doing that in a separate patch.

Erm, sorry, I misunderstood what you were saying.  In fact we can now remove mMIMEType, replacing its uses with mType.  Here's a patch that does that.

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 273823 [details] [diff] [review]
patch v7: removes mMIMEType

Looks good; r=dmose.

Comment 23

[Myk Melez [:myk] [@mykmelez]]

Thanks dmose and biesi for your help and reviews.  Fix checked into the trunk again:

Checking in netwerk/mime/public/nsIMIMEInfo.idl;
/cvsroot/mozilla/netwerk/mime/public/nsIMIMEInfo.idl,v  <--  nsIMIMEInfo.idl
new revision: 1.31; previous revision: 1.30
done
Checking in uriloader/exthandler/nsHandlerService.js;
/cvsroot/mozilla/uriloader/exthandler/nsHandlerService.js,v  <--  nsHandlerService.js
new revision: 1.8; previous revision: 1.7
done
Checking in uriloader/exthandler/nsMIMEInfoImpl.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsMIMEInfoImpl.cpp,v  <--  nsMIMEInfoImpl.cpp
new revision: 1.61; previous revision: 1.60
done
Checking in uriloader/exthandler/nsMIMEInfoImpl.h;
/cvsroot/mozilla/uriloader/exthandler/nsMIMEInfoImpl.h,v  <--  nsMIMEInfoImpl.h
new revision: 1.32; previous revision: 1.31
done
Checking in uriloader/exthandler/beos/nsMIMEInfoBeOS.h;
/cvsroot/mozilla/uriloader/exthandler/beos/nsMIMEInfoBeOS.h,v  <--  nsMIMEInfoBeOS.h
new revision: 1.6; previous revision: 1.5
done
Checking in uriloader/exthandler/beos/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/beos/nsOSHelperAppService.cpp,v  <--  nsOSHelperAppService.cpp
new revision: 1.21; previous revision: 1.20
done
Checking in uriloader/exthandler/mac/nsMIMEInfoMac.h;
/cvsroot/mozilla/uriloader/exthandler/mac/nsMIMEInfoMac.h,v  <--  nsMIMEInfoMac.h
new revision: 1.7; previous revision: 1.6
done
Checking in uriloader/exthandler/mac/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/mac/nsOSHelperAppService.cpp,v  <--  nsOSHelperAppService.cpp
new revision: 1.51; previous revision: 1.50
done
Checking in uriloader/exthandler/os2/nsMIMEInfoOS2.h;
/cvsroot/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.h,v  <--  nsMIMEInfoOS2.h
new revision: 1.7; previous revision: 1.6
done
Checking in uriloader/exthandler/tests/unit/test_handlerService.js;
/cvsroot/mozilla/uriloader/exthandler/tests/unit/test_handlerService.js,v  <--  test_handlerService.js
new revision: 1.8; previous revision: 1.7
done
Checking in uriloader/exthandler/unix/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/unix/nsOSHelperAppService.cpp,v  <--  nsOSHelperAppService.cpp
new revision: 1.66; previous revision: 1.65
done
Checking in uriloader/exthandler/win/nsMIMEInfoWin.h;
/cvsroot/mozilla/uriloader/exthandler/win/nsMIMEInfoWin.h,v  <--  nsMIMEInfoWin.h
new revision: 1.8; previous revision: 1.7
done
Checking in uriloader/exthandler/win/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/win/nsOSHelperAppService.cpp,v  <--  nsOSHelperAppService.cpp
new revision: 1.71; previous revision: 1.70
done

Comment 24

[Ria Klaassen (not reading all bugmail)]

Could this bug be the cause of extension crashes? For instance [e-mail] crashes on startup, and userchrome.js: http://www.haslo.ch/zeniko/software/userchrome.js.xpi
The latter crashes on second startup after the install.

Comment 25

[Ria Klaassen (not reading all bugmail)]

Also bug 388833 was in the regression range of the crash, together with this one.

Comment 26

[Ria Klaassen (not reading all bugmail)]

It crashes also on entering http://www.codedread.com/svg-support.php

Comment 27

[Myk Melez [:myk] [@mykmelez]]

Ria: we've clobbered, and the crashes should be gone.  Are you still seeing those crashes with the latest tinderbox builds?  Or, if you're building from scratch, can you clobber and rebuild and then test again?

Comment 28

[Ria Klaassen (not reading all bugmail)]

Sorry for the late reply but the crashes are indeed gone now.