299067 - Add nsIClassInfo to nsISimpleURI

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Stuart Parmenter]

I keep hitting in my Thunderbird builds with Lightning:

###!!! ASSERTION: aObject must implement nsIClassInfo: 'Not Reached', file
c:/builds/mozilla/objdir-thunderbird/xpcom/io/../../../xpcom/io/nsFastLoadFile.cpp,
line 2144

Where aObject is a nsSimpleURI.  I'm not sure what we're doing that is causing
this, but dbaron says that anything that implements nsISerializable has to also
implement nsIClassInfo.  I basically just copied the code from nsStandardURL for
this.

Comment 1

[Stuart Parmenter]

Attachment: Add nsIClassInfo to nsSimpleURI

This shouldn't break anything.. it is a very minor patch, but I'd like to get
it in 1.8 if possible.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 187554 [details] [diff] [review]
Add nsIClassInfo to nsSimpleURI

>+NS_IMETHODIMP 
>+nsSimpleURI::GetInterfaces(PRUint32 *count, nsIID * **array)
>+{
>+    *count = 0;
>+    *array = nsnull;
>+    return NS_OK;
>+}

Err..

>+NS_IMETHODIMP 
>+nsSimpleURI::GetContractID(char * *aContractID)
>+{
>+    *aContractID = nsnull;
>+    return NS_OK;
>+}

It has a contractid, at
http://lxr.mozilla.org/seamonkey/source/netwerk/build/nsNetCID.h#116 ?

Comment 3

[Stuart Parmenter]

We can certainly implement those things.  nsStandardURL doesn't, which is
perhaps a seperate bug, but I'm not sure that they are needed to fix this..

Comment 4

[Stuart Parmenter]

Attachment: With GetInterfaces and GetContractID implemented

just for you, vlad...

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 187559 [details] [diff] [review]
With GetInterfaces and GetContractID implemented

>+nsSimpleURI::GetInterfaces(PRUint32 *count, nsIID * **array)

If you allocate all 4 IID objects up front, you can simplify your early return
code a lot.

>+    const nsIID& isIID = NS_GET_IID(nsISupports);
>+    iids[0] = NS_STATIC_CAST(nsIID *, nsMemory::Clone(&isIID, sizeof(nsIID)));
>+    if (!iids[0]) {
>+        nsMemory::Free(iids);
>+        return NS_ERROR_OUT_OF_MEMORY;
>+    }

nsIClassInfo.idl says you can skip nsISupports.

Comment 6

[Stuart Parmenter]

Attachment: Clean up the early returns

Yeah, not sure what I was thinking there...

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

You still need to null check the three allocations, but you can do that all at
the end...

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #4)
> Created an attachment (id=187559) [edit]
> With GetInterfaces and GetContractID implemented
> 
> just for you, vlad...

Yay :)  Eradicating QI from JS code, one component at a time...

Comment 9

[Stuart Parmenter]

Attachment: It is right this time, honest.

Comment 10

[Darin Fisher]

nsStandardURL.cpp does not implement all of nsIClassInfo.  it only implements
GetClassID (and GetClassIDNoAlloc) because that's all that fastload needs.  why
are you implementing the rest of nsIClassInfo?

In fact, I'd argue that implementing GetInterfaces is a _BAD_ idea.  why? 
because necko supports pluggable protocol handlers via a frozen contract.  no
where in the contract for nsIProtocolHandler::newURI does it say that the
resulting nsIURI must support nsIClassInfo to automatically reflect certain
interface methods to script.  if you make this change (and assuming you do the
same with nsStandardURL) and people start using it, then existing third-party
nsIProtocolHandlers will lose.  that doesn't seem like a good thing to me.

if you want to make URLs easier to use from JS, then we need a new API wrapping
nsIURI because the implementation of nsIURI is not entirely something we
control.  we do for certain protocols, but not all in general.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #10)
> nsStandardURL.cpp does not implement all of nsIClassInfo.  it only implements
> GetClassID (and GetClassIDNoAlloc) because that's all that fastload needs.  why
> are you implementing the rest of nsIClassInfo?
> 
> In fact, I'd argue that implementing GetInterfaces is a _BAD_ idea.  why? 
> because necko supports pluggable protocol handlers via a frozen contract.  no
> where in the contract for nsIProtocolHandler::newURI does it say that the
> resulting nsIURI must support nsIClassInfo to automatically reflect certain
> interface methods to script.  if you make this change (and assuming you do the
> same with nsStandardURL) and people start using it, then existing third-party
> nsIProtocolHandlers will lose.  that doesn't seem like a good thing to me.
> 
> if you want to make URLs easier to use from JS, then we need a new API wrapping
> nsIURI because the implementation of nsIURI is not entirely something we
> control.  we do for certain protocols, but not all in general.

Fair enough, that explanation makes sense.

There may be a case here for fixing fastload to not require people to
implement-but-not-implement classinfo, and instead just add GetClassID to
nsISerializable as needed.  Not sure how frozen nsISerializable is; it doesn't
seem to be according to the idl, but that hasn't always been a good guide... but
that's a separate bug entirely.

Comment 12

[Darin Fisher]

Comment on attachment 187554 [details] [diff] [review]
Add nsIClassInfo to nsSimpleURI

Let's go with this patch instead.  r=darin

Comment 13

[Stuart Parmenter]

OK, we'll go with the first patch then.  I'd like to get this in for 1.8b3 as it
is causing some issues in Lightning.

Comment 14

[Benjamin Smedberg]

Comment on attachment 187554 [details] [diff] [review]
Add nsIClassInfo to nsSimpleURI

Please land quickly

Comment 15

[Mike Shaver (:shaver emeritus)]

Wait a sec.  We want to violate the contract of nsIClassInfo and give it a
partial implementation, because if we add a new capability to nsStandardURL
people might start to rely on unfrozen behaviour?  Our environment is full of
things that people can't use because they're unfrozen; we can't stop improving
our platform because others might not keep up, and because people might not
write perfectly general code if they don't want it.

What happens when we add an nsIPropertyBag QI to nsIChannel?  Can't people then
write to that new API, and not work with other implementations that don't
implement it, etc.?  I really don't think I agree with the reasoning here.

Comment 16

[Stuart Parmenter]

checked in the first patch.  Feel free to continue arguing about the last patch
if you want.  I tend to agree with shaver and vlad that we should implement the
whole thing, but as the first patch fixes the problem that I'm hitting, I don't
care as much anymore.

Comment 17

[Darin Fisher]

> Wait a sec.  We want to violate the contract of nsIClassInfo and give it a
> partial implementation, because if we add a new capability to nsStandardURL
> people might start to rely on unfrozen behaviour?  Our environment is full of
> things that people can't use because they're unfrozen; we can't stop improving
> our platform because others might not keep up, and because people might not
> write perfectly general code if they don't want it.

I think you are confusing things when you talk about dipping into the unfrozen
interface bucket.  I'm talking about messing with frozen interface contracts.

It is possible to write a protocol handler that uses only frozen interfaces.  In
fact, you could make this work all the way back to Mozilla 1.4 (maybe 1.0) if
you so desired.  It would be extremely unwise IMO to break protocol handlers
that were developed by following our rules about adhering to frozen interfaces.
 Afterall, we have told people that they can develop to our frozen interfaces
without fear of being broken by a future version of our product.  Are you saying
that is a bad rule?


> What happens when we add an nsIPropertyBag QI to nsIChannel?  Can't people 
> then write to that new API, and not work with other implementations that don't
> implement it, etc.?  I really don't think I agree with the reasoning here.

nsIPropertyBag is only implemented by HTTP.  It is not uniformly implemented. 
To the best of my knowledge the code that uses nsIPropertyBag fails gracefully
when that interface is not supported.  If not, then it is a bug.  When we add
support for nsIPropertyBag to all of our channels, we will need to take care to
QI and fail gracefully if the interface is not supported.  

nsIClassInfo::getInterfaces is a different beast altogether because the coder
has to take no action to get at the extra methods exposed by an additional
interface.  The onLinkIconAvailable issues with <tabbrowser> in Firefox 1.0 are
not an example to be repeated! ;-)

Comment 18

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

> nsIPropertyBag is only implemented by HTTP.

(It is also implemented by file: and ftp:, fwiw)