155202 - crash in proxytest.exe main() if gEventQueue is 0

Status: RESOLVED WORKSFORME

(Core :: XPCOM, defect)

Description

[timeless]

I've hit this in both debug and opt profile.
In debug, I get an assert warning me that i'm about to die (thanks).

The reason that I manage to trigger this is that i'm using the debugger to step 
along the program, and the occasionally debugger doesn't encourage other 
threads to run long enough to initialize gEventQueue.

aEventThread =
  PR_CreateThread(
    PR_USER_THREAD, EventLoop, 0, PR_PRIORITY_NORMAL, PR_GLOBAL_THREAD,
    PR_JOINABLE_THREAD, 0);
PR_Sleep(PR_MillisecondsToInterval(1000));
NS_ASSERTION(gEventQueue, "no main event queue");
// BAD BAD BAD.  EVENT THREAD DID NOT CREATE QUEUE.
// This may be a timing issue,
// set the sleep about longer, and try again.
ArgsStruct *args = (ArgsStruct *) malloc (sizeof(ArgsStruct));
args->queue = gEventQueue;
NS_ADDREF(args->queue); // <- crash.

I think we should use PR_WaitCondVar.

wtc: does that sound right?

Comment 1

[Wan-Teh Chang]

Yes, we should use PR_WaitCondVar.  Instead of
PR_Sleep(PR_MillisecondsToInterval(1000)), this
thread should do something like:

PR_Lock(lock);
while (!gEventQueue) {
    PR_WaitCondVar(cv, PR_INTERVAL_NO_TIMEOUT);
}
PR_Unlock(lock);

In EventLoop, you would do something like the
following after gEventQueue has been initialized:

PR_Lock(lock);
PR_NotifyCondVar(cv);
PR_Unlock(lock);

The PR_Sleep(PR_MillisecondsToInterval(1)) call in
EventLoop is also dubious.

Comment 2

[Doug Turner (:dougt)]

this was just my feeble attempt to test the proxy code from within my debugger.
 it was never meant (by me anywayz) as a real testcase.  timeless, if you have
the time to clean it up, that would be awesome.

Comment 3

[Brant Gurganus]

By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.

Comment 4

[timeless]

Attachment: fix

Comment 5

[timeless]

ok, i had most of the fix in my tree for a while, but the call to shutdown xpcom
crashed because of some improper management of the eventqueues. i came back
today and solved that problem. running the app i get 0 asserts, i don't hang and
from reading the code you can see that many more things are cleaned up.

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 111958 [details] [diff] [review]
fix

I could be mistaken, but we should be creating an event queue here?

+    nsCOMPtr<nsIEventQueueService> eventQService = 
+	      do_GetService(kEventQueueServiceCID, &rv);
+    if (NS_FAILED(rv)) return;

-   NS_RELEASE( ((ArgsStruct*) arg)->queue);
-   free((void*) arg);
+    rv = eventQService->GetThreadEventQueue(NS_CURRENT_THREAD, &eventQ);
+    if (NS_FAILED(rv)) return;



out of order? :
     gEventQueue->ProcessPendingEvents(); 
+    gEventQueue->StopAcceptingEvents();


NS_RELEASE nulls:
-    delete gEventQueue;
+    NS_RELEASE(gEventQueue);
     gEventQueue = nsnull;


How are we not destroying the lockset if argc > 1 - don't we have to allocate
the lockset struct in the heap?

could you provide more context in your next diff?

Comment 7

[timeless]

Attachment: sure stop first :), release fixed, and the scoping stuff rearranged so it is localized

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 112173 [details] [diff] [review]
sure stop first :), release fixed, and the scoping stuff rearranged so it is localized

This is a struct allocated on the stack:

+	 LockSet lockset;


PR_CreateThread(PR_USER_THREAD,
				      EventLoop,
-				      NULL,
+					  &lockset,

Here we pass the address of the stack to another function will which store it
for the new thread.  When the need thread accesses it, poof.

What am I missing?

Comment 9

[Brad Roberts]

That passing of a pointer to a stack variable into the thread create /should/ be
safe.  That stack frame will live longer than the lifetime of the thread that's
being created.  If the original thread (aka, the one with main()) was to exit
first, then yes, there'd be a problem, but in this case, before main ends
there's a PR_JoinThread(aEventThread) that will block until the spawned thread
exits and thus the pointer is no longer used.

Comment 10

[Doug Turner (:dougt)]

Comment on attachment 112173 [details] [diff] [review]
sure stop first :), release fixed, and the scoping stuff rearranged so it is localized

don't we leak the nsIEventQueue here.

+    nsIEventQueue* eventQ;
+    nsresult rv;
+    nsCOMPtr<nsIEventQueueService> eventQService = 
+	      do_GetService(kEventQueueServiceCID, &rv);
+    if (NS_FAILED(rv)) return;

+    rv = eventQService->GetThreadEventQueue(NS_CURRENT_THREAD, &eventQ);
+    if (NS_FAILED(rv)) return;
+
+    eventQ->StopAcceptingEvents();
    NS_RELEASE( ((ArgsStruct*) arg)->queue);
    free((void*) arg);
 }


The indention is a little off in this patch.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

wfm, due to bug 326273

Comment 12

[timeless]

gavin: actually, a number of the things in this patch still apply and should still be committed.