263957 - Convert nsProperties to nsTHashtable, implement GetKeys

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I made the mistake of convincing myself to use nsIProperties in my webdav
interfaces, so now I have to fix it.

Patch coming up which:
 - uses nsTHashtable (nsInterfaceHashtable<nsCStringHashKey, nsISupports>)
 - implements GetKeys at all (!!!!)

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: As promised.

(Also an explicit cast to quell a warning; warnings make me sad.)

I was tempted to change NS_ERROR_FAILURE to some better "no such entry" error
code while I was at it, but it's a frozen interface, and I guess someone
_could_ be counting on that gross behaviour.

Comment 2

[Darin Fisher]

Comment on attachment 161820 [details] [diff] [review]
As promised.

>Index: xpcom/ds/nsArray.cpp

>+        if (*aResult == (PRUint32)-1)

PR_UINT32_MAX ?


>Index: xpcom/ds/nsProperties.cpp

>+    if (!nsProperties_HashBase::Get(nsDependentCString(prop),
...
>+    return Put(nsDependentCString(prop), value) ? NS_OK : NS_ERROR_FAILURE;
...
>+    nsDependentCString key(prop);
...
>+    if (!nsProperties_HashBase::Get(key, getter_AddRefs(value)))
...
>+    *result = nsProperties_HashBase::Get(nsDependentCString(prop),

seems to me that nsCharPtrHashKey might be a better choice.



> NS_IMETHODIMP 
> nsProperties::GetKeys(PRUint32 *count, char ***keys)
> {
>-    return NS_ERROR_NOT_IMPLEMENTED;
>+    PRUint32 n = *count = Count();
>+    char ** k = *keys = new char *[n];

shouldn't you use nsMemory::Alloc for this since you are crossing
XPCOM boundaries?

(would be better if this method returned an enumerator type.)


...
>+    if (NS_FAILED(gked.res)) {
>+        // Free 'em all
>+        for (PRUint32 i = 0; i < gked.next; i++)
>+            nsMemory::Free(k[i]);
>+        return gked.res;

doesn't this failure case need to free 'k' ?


>+class nsProperties : public nsIProperties,
...
>+  virtual ~nsProperties() { }

can this dtor be non-virtual?


also, not that it really matters, but nsProperties_BashHash could be
a private base class.

Comment 3

[Darin Fisher]

Attachment: v2 patch

I revised this patch on account of the fact that I want to use GetKeys as part
of the solution to bug 265135.

I also had to restore support for aggregation, and in doing so I had to fix the
NS_ENSURE_PROPER_AGGREGATION macro.  The intent of that macro is to require
callers of CreateInstance to pass an IID other than nsISupports when passing a
non-null outer instance.  Afterall, the inner object can never be the
nsISupports identity object, that's the job of the outer object!

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 162789 [details] [diff] [review]
v2 patch

Man, this open-source thing rules. sr=shaver, and thanks.

Comment 5

[Benjamin Smedberg]

Comment on attachment 162789 [details] [diff] [review]
v2 patch

> I also had to restore support for aggregation, and in doing so I had to fix the
> NS_ENSURE_PROPER_AGGREGATION macro.  The intent of that macro is to require
> callers of CreateInstance to pass an IID other than nsISupports when passing a
> non-null outer instance.  Afterall, the inner object can never be the
> nsISupports identity object, that's the job of the outer object!

This is wrong. You MUST request nsISupports when using aggregation, to obtain
the "special" inner nsISupports that the outer object can query. I can't find
my Don Box for a page# reference at the moment, but I'll look it up.

Comment 6

[Darin Fisher]

> This is wrong. You MUST request nsISupports when using aggregation, to obtain
> the "special" inner nsISupports that the outer object can query. I can't find
> my Don Box for a page# reference at the moment, but I'll look it up.

It doesn't make any sense.  Why would the outer want a pointer to the special
Internal class?  Calling QI on that just calls AggregatedQueryInterace anyway. 
Moreover, in all other cases the Internal class is unused when we are aggregated.

Also, if I have to pass nsISupports to CreateInstance when aggregating
nsProperties, then I cannot do it from within my QueryInterface without creating
some sort of wierd re-entrancy problem:
  
  In my QueryInterface, I call "p = CreateInstance(nsProperties, nsISupports)".
  Then I call "mProperties = p->QueryInterface(nsIProperties)", but that 
  re-enters my QueryInterface, and mProperties is not yet assigned, so I enter 
  CreateInstance again, and repeat.

That can't be right!

If you look at the nsAgg.h macros, it would seem that when we are aggregated,
the refcnt of my object is supposed to be bound to the refcnt of fOuter.  In the
aggregated case, fOuter is the nsHttpChannel... not the nsProperties::Internal.
 I think the Internal class is only for use when not aggregated.

Comment 7

[Darin Fisher]

Ah!  I get it now.  New patch coming up.  I have to get the nsISupports
implementation of nsProperties otherwise I will leak mProperties, since it's
Release calls nsHttpChannel's Release!

I have to keep a nsISupports reference to the inner object so I can control its
lifetime.  And, I was mistaken about the QueryInterface re-entrancy problem. 
That is avoided because the Internal class's QueryInterface just calls
AggregatedQueryInterface.  Ok, forget the NS_ENSURE_PROPER_AGGREGATION macro change.

I'll need to revise my patch for bug 265135.  Thanks bsmedberg!

Comment 8

[Darin Fisher]

Attachment: v2.1 patch

ok, here's the revised patch without the change to
NS_ENSURE_PROPER_AGGREGATION.

Comment 9

[Benjamin Smedberg]

Comment on attachment 162863 [details] [diff] [review]
v2.1 patch

>Index: xpcom/ds/nsProperties.cpp

> NS_IMETHODIMP
> nsProperties::Has(const char* prop, PRBool *result)
> {
>-    nsCStringKey key(prop);
>-    *result = nsHashtable::Exists(&key);
>+    nsCOMPtr<nsISupports> value;
>+    *result = nsProperties_HashBase::Get(prop,
>+                                         getter_AddRefs(value));
>     return NS_OK;
> }

You are allowed to pass null to this getter, so
  *result = nsProperties_HashBase::Get(prop, null));
saves an extra addref-release.

Comment 10

[Darin Fisher]

> You are allowed to pass null to this getter, so
>   *result = nsProperties_HashBase::Get(prop, null));
> saves an extra addref-release.

ok, thanks!

Comment 11

[Darin Fisher]

Comment on attachment 162863 [details] [diff] [review]
v2.1 patch

carrying forward shaver's sr, requesting aviary approval.  we should probably
land this on the 1.7 branch too for gecko compat (though i doubt this would
impact anyone on the 1.7 branch).

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 162863 [details] [diff] [review]
v2.1 patch

a=asa for branches landing

Comment 13

[Darin Fisher]

fixed-aviary1.0, fixed1.7.3

leaving open since this needs to be landed on the trunk.

reassigning to me so i don't forget to land this on the trunk.

Comment 14

[Darin Fisher]

i meant fixed1.7.x

Comment 15

[Darin Fisher]

fixed-on-trunk