Closed Bug 388388 Opened 15 years ago Closed 15 years ago

differentiate between MIME and protocol handler nsIHandlerInfos

Categories

(Core Graveyard :: File Handling, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: myk, Assigned: myk)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

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.
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?
Attachment #272896 - Flags: superreview?
Attachment #272896 - Flags: review?(cbiesinger)
Attachment #272896 - Flags: superreview? → superreview?(dmose)
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 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?
> 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.

(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.
Attachment #272896 - Attachment is obsolete: true
Attachment #273225 - Flags: superreview?(dmose)
Attachment #273225 - Flags: review?(cbiesinger)
Attachment #272896 - Flags: superreview?(dmose)
Attachment #272896 - Flags: review?(cbiesinger)
Blocks: 385065
Here's an update that resolves the conflict with the patch for bug 388701 that was checked in recently.
Attachment #273225 - Attachment is obsolete: true
Attachment #273454 - Flags: superreview?(cbiesinger)
Attachment #273454 - Flags: review?(dmose)
Attachment #273225 - Flags: superreview?(dmose)
Attachment #273225 - Flags: review?(cbiesinger)
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
Attachment #273454 - Flags: review?(dmose) → review+
Blocks: 389377
(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 on attachment 273454 [details] [diff] [review]
patch v3: resolves conflicts with bug 388701

Please change the type of aClass to HandlerClass instead of int.
Attachment #273454 - Flags: superreview?(cbiesinger) → superreview+
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.
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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 389474
(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...
(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>.
Attachment #273454 - Attachment is obsolete: true
Attachment #273641 - Attachment is obsolete: true
Attachment #273768 - Flags: superreview?(cbiesinger)
Attachment #273768 - Flags: review?(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!
Attachment #273768 - Flags: review?(dmose) → review+
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...
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 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?
Attachment #273768 - Flags: superreview?(cbiesinger) → superreview+
False alarm; clobbering some key directories made the crash go away.  I'm running the mochitests on Windows now.
Attached patch patch v6: review comment fixes (obsolete) — Splinter Review
(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.
Attachment #273768 - Attachment is obsolete: true
> (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.
Attachment #273814 - Attachment is obsolete: true
Attachment #273823 - Flags: superreview?(cbiesinger)
Attachment #273823 - Flags: review?(dmose)
Comment on attachment 273823 [details] [diff] [review]
patch v7: removes mMIMEType

Looks good; r=dmose.
Attachment #273823 - Flags: review?(dmose) → review+
Attachment #273823 - Flags: superreview?(cbiesinger) → superreview+
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
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Could this bug be the cause of extension crashes? For instance ctr-tab@design-noir.de crashes on startup, and userchrome.js: http://www.haslo.ch/zeniko/software/userchrome.js.xpi
The latter crashes on second startup after the install.
Also bug 388833 was in the regression range of the crash, together with this one.
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?
Sorry for the late reply but the crashes are indeed gone now.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.