wifi tickler causes ~nsHttpHandler to spin the event loop

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: karlt, Assigned: mcmanus)

Tracking

(Blocks: 1 bug)

Trunk
mozilla26
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Updated

5 years ago
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.
(Reporter)

Comment 2

5 years ago
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.
(Assignee)

Comment 5

5 years ago
Created attachment 792310 [details] [diff] [review]
dont nsithread->shutdown() from dtor
Attachment #792310 - Flags: review?(karlt)
(Assignee)

Updated

5 years ago
Assignee: nobody → mcmanus
(Reporter)

Comment 6

5 years ago
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+
(Reporter)

Comment 7

5 years ago
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?
(Reporter)

Comment 8

5 years ago
>+  nsConnEvent(nsIThread *aThread)

"explicit" may be worthwhile here.
(Assignee)

Updated

5 years ago
Attachment #792310 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #793078 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6f48b270e08f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.