1010717 - add nsIRunnable support to the DOM Storage thread

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

mozStorage may have the need to send maintenance runnables on the connection thread, unfortunately DOM Storage uses a raw PR_Thread that, differently from nsIThread, doesn't ensure the runnable to be properly run and released, that means it basically leaks after being dispatched.
Honza suggested filing this bug to add nsIRunnable support to the DOM Storage thread.

Comment 1

[Honza Bambas (:mayhemer)]

Yes, you can get inspired by how CacheIOThread is implemented.  You probably don't need to expose the XPCOM thread tho, so that it will be simpler, we only need to be hooked to the XPCOM thread internally to process NS_DispatchToCurrentThread() runnables immediately.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I will try to look at this in the spare time, no rush.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1

Something like this seems to be working locally. Still testing on Try (that is currently closed :( )

I originally make DOMStorageDBThread directly be the nsIThreadObserver, but by doing that I basically made it managed by a com pointer, the thread manager then became the de-facto owner of it, killing it on destruction. That was causing crashes due to monitor ownership on shutdown and such, so I preferred to split out an observer object owned by both and let alone the thread life management.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8534386 [details] [diff] [review]
patch v1

Try looks happy enough and I locally tested that the runnable is executed.
Honza, what do you think?

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8534386 [details] [diff] [review]
patch v1

Review of attachment 8534386 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/storage/DOMStorageDBThread.cpp
@@ +302,5 @@
>    }
>  
> +  // Create an nsIThread for the current PRThread, so we can observe runnables
> +  // dispatched to it.
> +  nsIThread *thread = NS_GetCurrentThread();

better use a comptr?

@@ +307,5 @@
> +  nsCOMPtr<nsIThreadInternal> threadInternal = do_QueryInterface(thread);
> +  MOZ_ASSERT(threadInternal); // Should always succeed.
> +  mThreadObserver = new ThreadObserver(&mMonitor);
> +  threadInternal->SetObserver(mThreadObserver);
> +  lockMonitor.Notify();

why this notification?  if you just copied this from the cache thread, then there is no need to do this.  I only notify there since I need to sync access to mXPCOMThread member that isn't here at all.  so, unless I'm missing something please remove this Notify() call.

@@ +355,5 @@
>    mStatus = ShutdownDatabase();
> +
> +  if (threadInternal) {
> +    threadInternal->SetObserver(nullptr);
> +  }

I think there is a problem :(  there may be a race when DOMStorageDBThread::ThreadObserver::OnDispatchedEvent is just called but instance of DOMStorageDBThread just goes away at the same moment, and the monitor with it.  That is not good at all.

Possible solution:
- create ThreadObserver in the DOMStorageDBThread's ctor (still have it as a member via nsRefPtr, that's good)
- move the monitor member to ThreadObserver
- let DOMStorageDBThread just refer the monitor from the thread observer

::: dom/storage/DOMStorageDBThread.h
@@ +222,5 @@
> +
> +    bool HasPendingEvents() { return mHasPendingEvents; }
> +    void ClearPendingEvents() { mHasPendingEvents = false; }
> +  private:
> +    ~ThreadObserver() {}

should be virtual?

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> > +    ~ThreadObserver() {}
> 
> should be virtual?

It's marked as MOZ_FINAL, it should not be needed.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v2

Is this what you had in mind?

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 8552535 [details] [diff] [review]
patch v2

Review of attachment 8552535 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab with the comments addressed.

thanks!

::: dom/storage/DOMStorageDBThread.cpp
@@ +37,5 @@
>  DOMStorageDBBridge::DOMStorageDBBridge()
>  {
>  }
>  
> +NS_IMPL_ISUPPORTS(DOMStorageDBThread::ThreadObserver, nsIThreadObserver)

Please move this somewhere else then between a ctor and dtor of a different class.

::: dom/storage/DOMStorageDBThread.h
@@ +219,5 @@
> +      , mMonitor("DOMStorageThreadMonitor")
> +    {
> +    }
> +
> +    bool HasPendingEvents() { return mHasPendingEvents; }

assert you are locked (impl in cpp?)

@@ +220,5 @@
> +    {
> +    }
> +
> +    bool HasPendingEvents() { return mHasPendingEvents; }
> +    void ClearPendingEvents() { mHasPendingEvents = false; }

here as well

@@ +224,5 @@
> +    void ClearPendingEvents() { mHasPendingEvents = false; }
> +    Monitor& GetMonitor() { return mMonitor; }
> +
> +  private:
> +    ~ThreadObserver() {}

virtual?

@@ +272,5 @@
>  private:
>    nsCOMPtr<nsIFile> mDatabaseFile;
>    PRThread* mThread;
>  
> +  // Flag to stop, protected by the ThreadObserver monitor

please, add a comment here that the Monitor to lock is hidden in the mThreadObserver member.  also.. maybe move that member here?

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v3

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/integration/fx-team/rev/9b1ebcd6d253

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b1ebcd6d253