Closed Bug 1010717 Opened 7 years ago Closed 7 years ago

add nsIRunnable support to the DOM Storage thread

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
I will try to look at this in the spare time, no rush.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attached patch patch v1 (obsolete) — Splinter Review
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 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?
Attachment #8534386 - Flags: review?(honzab.moz)
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?
Attachment #8534386 - Flags: review?(honzab.moz) → review-
(In reply to Honza Bambas (:mayhemer) from comment #5)
> > +    ~ThreadObserver() {}
> 
> should be virtual?

It's marked as MOZ_FINAL, it should not be needed.
Attached patch patch v2 (obsolete) — Splinter Review
Is this what you had in mind?
Attachment #8534386 - Attachment is obsolete: true
Attachment #8552535 - Flags: review?(honzab.moz)
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?
Attachment #8552535 - Flags: review?(honzab.moz) → review+
Attached patch patch v3Splinter Review
Attachment #8552535 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/9b1ebcd6d253
Target Milestone: --- → mozilla38
https://hg.mozilla.org/mozilla-central/rev/9b1ebcd6d253
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.