214839 - Need shared PLDHashTableOps for [const] char *key use-cases

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Brendan Eich [:brendan]]

We want PL_DHashMatchStringKey (for matchEntry) and PL_DHashFreeStringKey (for
clearEntry where the key is an owning ref to something malloc'd by the xpcom
malloc).  The naming symmetry is fractured, but I don't want longer names, and I
don't think these are that bad.

Patch soon. Of course, it modifies js/src/jsdhash.[ch] too (or first; the
xpcom/ds peers of these files are derived via js/src/plify_jsdhash.sed).

/be

Comment 1

[Brendan Eich [:brendan]]

Cc'ing more people who need to review the forthcoming patch.  It won't hurt
much, I promise.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix (apply this)

diff -w version coming up for review in a second.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: diff -w version of last patch (review this)

- Add DHashMatchStringKey and DHashFreeStringKey default ops for string keyed
  entry types.

- Give the optional PLDHashTableOps.initEntry hook a boolean return value to
  cause PL_DHashTableOperate(PL_DHASH_ADD) to fail with a null return and no
  added entry, in case of OOM or other errors during initialization.

- Fix netwerk/dns/src/nsDnsService.cpp,
security/manager/ssl/src/nsCertTree.cpp,  and widget/src/gtk/nsWindow.cpp to
use PL_DHASH_ADD instead of LOOKUP and
  then maybe ADD.

  Unless you have to unlock a critical section and let other threads race, or
  just nest reentrant or similar calls that add to the table, it is always
  better to ADD without LOOKUP.  ADD does a LOOKUP and returns the existing
  entry for the given key, if any.  You have to null-check the return value of
  PL_DHashTableOperate for ADD, of course (see below), and you must have an
  initEntry hook, or else be able to zero-test a member (key, e.g.) of the
  entry to see if it pre-existed the ADD call.

- Fix abuses of PL_DHASH_ENTRY_IS_LIVE -- use PL_DHASH_ENTRY_IS_BUSY to test
  for a hit after PL_DHASH_LOOKUP (see the comments in pldhash.h), and use
  PL_DHASH_ENTRY_IS_FREE(e) instead of !PL_DHASH_ENTRY_IS_LIVE(e).  Small
  perf/code-size win.

- Don't null-test the return value from PL_DHashTableOperate(PL_DHASH_LOOKUP),
  it can't be null (PL_DHASH_ADD can be; PL_DHASH_REMOVE always returns null).

- Subclass the PLDHashEntryHdr struct and use NS_STATIC_CAST consistently in
  files that don't just use old-C-style casts (trace.cpp, e.g.).

/be

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 129077 [details] [diff] [review]
diff -w version of last patch (review this)

Looking for reviews of relevant files by all on the cc: list, and I'd like to
get this in for 1.5b, which means in the next day or two, realistically.

/be

Comment 5

[timeless]

Comment on attachment 129077 [details] [diff] [review]
diff -w version of last patch (review this)

oh man, this is going to cause conflicts for any of the unfixed
PL_DHashTableInit call bugs (bug 211260 -
http://bugzilla.mozilla.org/showdependencytree.cgi?id=211260&hide_resolved=1 )
that i have. that's ok.

Index: content/base/src/nsGenericElement.cpp
@@ -916,16 +917,17 @@ RangeListHashClearEntry(PLDHashTable *ta
 }

 PR_STATIC_CALLBACK(void)
 EventListenerManagerHashInitEntry(PLDHashTable *table, PLDHashEntryHdr *entry,
				   const void *key)
 {
   // Initialize the entry with placement new
   new (entry) EventListenerManagerMapEntry(key);
+  return PR_TRUE;
 }

^ Signature needs to change from (void) to (PRBool)

+    /* XXX tolerate null keys on account of sloppy Mozilla callers. */
is this a permanent policy change or do you want to tie that to some bug?

warning re:  nsDNSService::FindOrCreateLookup(const char* hostName)
darin blocked bug 205726 to fix that caller. he should probably be cc'd to
this.

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 129077 [details] [diff] [review]
diff -w version of last patch (review this)

The caps/ changes look good to me.  Nice cleanup with the
PL_DHASH_ENTRY_IS_LIVE stuff!  content/ and dom/ changes also look good with
the signature update timeless mentioned.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 129077 [details] [diff] [review]
diff -w version of last patch (review this)

maybe the comments in the patch should spell OOM out.

in config/trace.cpp, I don't understand how you were able to not have to zero
the CallEntry after adding it to the hash.

Please change the return type of EventListenerManagerHashInitEntry.

Would asserting be something you want to do for null keys -- finger the sloppy
callers?


 JS_PUBLIC_API(void)
+JS_DHashFreeStringKey(JSDHashTable *table, JSDHashEntryHdr *entry)
+{
+    const JSDHashEntryStub *stub = (const JSDHashEntryStub *)entry;

and others: NS_STATIC_CAST... maybe not since this file seams to be clean of
nsnull and other mozilla #defines.

in gtk/nsWindow.cpp, what is this about:

+#ifdef NS_DEBUG
+    PRUint32 generation = sIconCache->generation;
+#endif

The spacing looks off in nsWindow::IMEGetInputContext.

no case insensitive version of PL_DHashMatchStringKey?	

Most of the above are nits.  explain the trace.cpp changes and r=dougt

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Doug, note that some of these files are written in C, and as such they should
not be using the new C++ casts.

Comment 9

[Brendan Eich [:brendan]]

> in config/trace.cpp, I don't understand how you were able to not have to zero
> the CallEntry after adding it to the hash.

All entry storage starts out zeroed, and the clearEntry stub memsets to 0.

> Please change the return type of EventListenerManagerHashInitEntry.

Thanks -- why did gcc let me compile this file?

> Would asserting be something you want to do for null keys -- finger the
sloppy> callers?

{js,pl}dhash.c has only {JS,PR}_ASSERT, which aborts.  I'll add an assertion
at the start of 1.6a, not now (when I might delay others in the last few days
before 1.5b freezes).

>  JS_PUBLIC_API(void)
> +JS_DHashFreeStringKey(JSDHashTable *table, JSDHashEntryHdr *entry)
> +{
> +    const JSDHashEntryStub *stub = (const JSDHashEntryStub *)entry;

> and others: NS_STATIC_CAST... maybe not since this file seams to be clean of
> nsnull and other mozilla #defines.

See above: this is C code, not C++.

> in gtk/nsWindow.cpp, what is this about:

> +#ifdef NS_DEBUG
> +    PRUint32 generation = sIconCache->generation;
> +#endif

It's my paranoia, in case some thing called indirectly between the PL_DHASH_ADD
and the update of entry->* grows or shrinks sIconCache.

> The spacing looks off in nsWindow::IMEGetInputContext.

You're reading a diff -wu -- check out the diff -u, it all looks good there
(and in my vim buffer ;-).

> no case insensitive version of PL_DHashMatchStringKey?

Don't see the point yet.  Did I miss something?

> Most of the above are nits.  explain the trace.cpp changes and r=dougt

Thanks.

/be

Comment 10

[Brendan Eich [:brendan]]

I've updated the patch slightly, in js/src/jsdhash.c and xpcom/ds/pldhash.c: if
initEntry returns false, memset the entry struct (except for keyHash, of
course!) to 0, in case the failed initEntry did not restore the status-quo-ante.

/be

Comment 11

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

Comment on attachment 129077 [details] [diff] [review]
diff -w version of last patch (review this)

nsScriptSecurityManager.h:

  InitClassPolicyEntry:
	 cp->key = PL_strdup((const char*)key);
    That could be an NS_CONST_CAST, while you're there.

    +	     PL_strfree((void*)cp->key);

    Why cast to (void*)?  I'm surprised it even works.	It's already a
    |char*|, and that's what PL_strfree takes.

nsScriptSecurityManager.cpp:

  The NS_REINTERPRET_CASTs on the result of PL_DHashTableOperate should
  all be NS_STATIC_CASTs.

EventListenerManagerHashInitEntry

  needs return type changed (as stated before)

-  for (int i = 0; i < ARRAY_LENGTH(gsGtkCursorCache); ++i) {
+  for (int i = 0, n = ARRAY_LENGTH(gsGtkCursorCache); i < n; ++i) {

I don't see any reason to do this, since ARRAY_LENGTH should be
constant-folded (sizeof(a)/sizeof(a[0])), but while you're here, could
you remove the local ARRAY_LENGTH and use NS_ARRAY_LENGTH?


With that, and the change in comment 10, sr=dbaron.

Comment 12

[Brendan Eich [:brendan]]

dbaron: thanks!  I added n to capture ARRAY_LENGTH's result only to avoid a
signed vs. unsigned comparison warning.  I'll fix the other things you pointed
out and get this in today.

/be

Comment 13

[Brendan Eich [:brendan]]

I hate gcc. It warned (should have error'ed) about another void/PRBool return
mismatch, in caps/include/nsScriptSecurityManager.h (this time there was no
return in the void function, but it completely mismatched the function-pointer
signature of the initEntry member it was initializing), and I missed that amid
all the usual warnings in my build log.

Fixed.

/be