Closed Bug 900322 Opened 12 years ago Closed 11 years ago

wifi tickler causes ~nsHttpHandler to spin the event loop

Categories

(Core :: Networking: HTTP, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: karlt, Assigned: mcmanus)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Summary: wifi ticker causes ~nsHttpHandler to spin the event loop → wifi tickler causes ~nsHttpHandler to spin the event loop
(In reply to Karl Tomlinson (:karlt) from comment #0) > http://hg.mozilla.org/mozilla-central/annotate/fd03bb4d1e48/netwerk/base/src/ > Tickler.cpp#l41 > http://hg.mozilla.org/mozilla-central/annotate/fd03bb4d1e48/netwerk/protocol/ > http/nsHttpHandler.h#l479 > > I don't know much about this code but I wouldn't have expected releasing an > nsIProtocolHandler to spin the event loop. I'm afraid this is exactly what XPCOM threads do... we are hitting this more and more. nsThread::Shutdown() unconditionally spins the event loop of the calling thread. There is no simple way to prevent that. You may need a new bug to add a new ShutdownBlocked() or so method to nsIThread to stop the thread w/o looping the caller thread or Tickler has to be rewritten to use NSPR threads or has it's own XPCOM thread shutdown logic.
Although it may be possible to change nsIThread behavior, this bug is about the Tickler using the existing nsIThread interface incorrectly. If Tickler needs an event queue on the nsIThread, and so doesn't want to use PR_CreateThread() and friends directly, then shutting down the thread off an event ensures the stack is in a safe state to run other events.
(In reply to Karl Tomlinson (:karlt) from comment #2) > Although it may be possible to change nsIThread behavior, this bug is about > the Tickler using the existing nsIThread interface incorrectly. > What exactly you consider as "incorrect usage" ? > If Tickler needs an event queue on the nsIThread, and so doesn't want to use > PR_CreateThread() and friends directly, then shutting down the thread off an > event ensures the stack is in a safe state to run other events. Not a bad idea.
Attachment #792310 - Flags: review?(karlt)
Assignee: nobody → mcmanus
Comment on attachment 792310 [details] [diff] [review] dont nsithread->shutdown() from dtor >+class TicklerThreadDestructor : public nsRunnable >+{ >+public: >+ nsConnEvent(nsIThread *aThread) >+ : mThread(aThread) { } IIRC this is built only on Android. r=karlt assuming the name changes required to make this and the destructor compile. >+ NS_IMETHOD Run() Could add MOZ_OVERRIDE here. >+ { >+ MOZ_ASSERT(NS_IsMainThread()); >+ if (mThread) >+ mThread->Shutdown(); I don't think the "if (mThread)" is necessary here. (In reply to Honza Bambas (:mayhemer) from comment #3) > What exactly you consider as "incorrect usage" ? As addressed in this patch, thanks, calling nsIThread::Shutdown() from somewhere not known to be safe to run the event loop. See for example bug 679375 and bug 477432.
Attachment #792310 - Flags: review?(karlt) → review+
nsHttpHandler and Tickler have thread-safe ref-counting, but ~Tickler asserts NS_IsMainThread(). Does something ensure that the last reference is released on the main thread?
>+ nsConnEvent(nsIThread *aThread) "explicit" may be worthwhile here.
Attachment #792310 - Attachment is obsolete: true
Attachment #793078 - Flags: review+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: