230092 - [FIX]improve hashtable stuff in event queue service

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Die, returners of addrefed raw ptrs, die!

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Like this-ish

The changes to nsHashKeys were done by bsmedberg; r=me on those.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 138404 [details] [diff] [review]
Like this-ish

darin, bsmedberg, could you review?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Oops. Forgot to init the hashtable...

Comment 4

[Benjamin Smedberg]

Comment on attachment 138421 [details] [diff] [review]
Oops.  Forgot to init the hashtable...

>--- xpcom/threads/nsEventQueueService.cpp	3 Dec 2003 23:53:22 -0000	1.41
>+++ xpcom/threads/nsEventQueueService.cpp	5 Jan 2004 16:46:48 -0000
nsEventQueueServiceImpl::PushThreadEventQueue(nsIEventQueue **aNewQueue)
[snip]
>   if (ourChain)
>     ourChain->AppendQueue(newQueue); // append new queue to it
> 
>   *aNewQueue = newQueue;
>-  NS_IF_ADDREF(*aNewQueue);

Don't you still need this?

Comment 5

[Boris Zbarsky [:bzbarsky]]

No, I do not.  See the part right above that:

-  nsCOMPtr<nsIEventQueue> newQueue;
-  MakeNewQueue((PRThread*)key.GetValue(), native, getter_AddRefs(newQueue)); //
create new queue
+  nsIEventQueue* newQueue = nsnull;
+  MakeNewQueue(currentThread, native, &newQueue); // create new queue; addrefs

Comment 6

[Benjamin Smedberg]

Comment on attachment 138421 [details] [diff] [review]
Oops.  Forgot to init the hashtable...

ah yes, ok

Comment 7

[Darin Fisher]

Comment on attachment 138421 [details] [diff] [review]
Oops.  Forgot to init the hashtable...

>Index: xpcom/threads/nsEventQueueService.h

>-  nsSupportsHashtable mEventQTable;
>+  nsInterfaceHashtable<nsVoidPtrHashKey, nsIEventQueue> mEventQTable;
>   PRMonitor           *mEventQMonitor;
> };

nit: looks like some effort was previously made to line up the variable
names in a column.  maybe you want to either preserve that or simply
eliminate the extra space in front of *mEventQMonitor?


>Index: xpcom/threads/nsEventQueueService.cpp


>+PR_STATIC_CALLBACK(PLDHashOperator)
>+hash_enum_remove_queues(const void *aThread_ptr,
>+                        nsCOMPtr<nsIEventQueue>& aQueue,
>+                        void* closure)
> {
>-  // 'aData' should be the eldest queue.
>-  nsIEventQueue *eldestQueue = (nsIEventQueue*)aData;
>-
>-  nsCOMPtr<nsPIEventQueueChain> pie(do_QueryInterface(eldestQueue));
>+  // 'aQueue' should be the eldest queue.
>+  nsCOMPtr<nsPIEventQueueChain> pie(do_QueryInterface(aQueue));

nitty nit: i think aQueue should be called "aEldestQueue" to
preserve the self-documenting name.


sr=darin

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to nits

Comment 9

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 10

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

Comment on attachment 138729 [details] [diff] [review]
Patch updated to nits

 nsEventQueueServiceImpl::CreateEventQueue(PRThread *aThread, PRBool aNative)
+  mEventQTable.Get(aThread, getter_AddRefs(queue));
do you need a strong reference?

+  nsCOMPtr<nsIEventQueue> queue;
+  mEventQTable.Get(currentThread, getter_AddRefs(queue));
and here?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> nsEventQueueServiceImpl::CreateEventQueue(PRThread *aThread, PRBool aNative)

No, I guess I could push the strong ref down into the !queue case....

> and here?

Also looks like it may be doable, yes.

Wanna post patches?  Or should I work on it when I get the chance to?

Comment 12

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

I'd prefer you to do it... I don't have much time this month

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to that effect

This has three changes:

1) Use GetWeak in those two spots
2) Fix what looks like an obvious bug in CreateEventQueue (passing wrong thread

   to MakeNewQueue).
3) Fix what looks like a bug in PushThreadEventQueue (looking at the original
   queue for the thread rather than the youngest one).

Comment 14

[Boris Zbarsky [:bzbarsky]]

Reopening till I get that in.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 138763 [details] [diff] [review]
Patch to that effect

darin, would you review?

Comment 16

[Darin Fisher]

Comment on attachment 138763 [details] [diff] [review]
Patch to that effect

sr=darin

Comment 17

[Boris Zbarsky [:bzbarsky]]

Checked that in too.