Closed Bug 1155193 Opened 4 years ago Closed 4 years ago

mozStorageService::unregisterConnection() should proxy release

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(1 file, 2 obsolete files)

While running a try build I hit a non-threadsafe assert in mozStorage.  From the stack it appears a memory-pressure observer event is triggering unregisterConnection which in turn causes the Release() of a connection off its owning thread.

I think unregisterConnection() should explicitly proxy release the connection's owning thread to be correct.

Stack:

21:41:25 INFO - #01: mozilla::storage::Connection::Release() [memory/mozalloc/mozalloc.h:211]
21:41:25 INFO - #02: nsTArray_Impl<nsRefPtr<mozilla::storage::Connection>, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned long, unsigned long) [xpcom/glue/nsTArray.h:1726]
21:41:25 INFO - #03: mozilla::storage::Service::unregisterConnection(mozilla::storage::Connection*) [xpcom/glue/Mutex.h:169]
21:41:25 INFO - #04: mozilla::storage::Connection::Release() [storage/src/mozStorageConnection.cpp:544]
21:41:25 INFO - #05: nsTArray_Impl<nsRefPtr<mozilla::storage::Connection>, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned long, unsigned long) [xpcom/glue/nsTArray.h:1726]
21:41:25 INFO - #06: mozilla::storage::Service::minimizeMemory() [xpcom/glue/nsTArray-inl.h:21]
Slightly better patch as it does not AddRef() off the opening thread either.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=04c5f14a8d38
Attachment #8593375 - Attachment is obsolete: true
Attachment #8593375 - Flags: review?(bugmail)
Attachment #8593385 - Flags: review?(bugmail)
Hmm, try is unhappy.  Is it possible for a connection to be nullptr in unregisterConnection()?
Maybe it doesn't like referencing the nsRefPtr<> and a .forget() in the same statement, but it seems that should be ok.  Lets just verify the pointer exists first.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c9c9f244bdf
NS_ProxyRelease has a signature that understands nsRefPtr's and just does a swap:
https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsProxyRelease.h#43

Can we do that instead of the forget().take() stuff?
I can try that.  Also I see it has an option to allow immediate release if on same thread.  I should use that instead of the thread comparison.
Also, for context from email but not this bug, the crash was:
21:40:50 INFO - Hit MOZ_CRASH(nsStandardURL not thread-safe) at /builds/slave/try-m64-d-00000000000000000000/build/src/netwerk/base/nsStandardURL.cpp:959
21:41:25 INFO - #01: mozilla::storage::Connection::Release() [memory/mozalloc/mozalloc.h:211]

And so this isn't one of our previous mozStorage Benny Hill thread bouncing things, but that nsStandardURL really does not want to be released on a different thread.

Which raises the question of whether we should instead be:
- normalizing the URL to something thread-safe.  We use GetFile and GetSpec.  I don't think these are particularly fancy.
- not holding onto the URL at all.  Unfortunately, it's used for cloning connections, so the above uses still need to be satisfied somehow.
Hmm... My patch is probably not right.  In the light of the caffeinated day, I see mozStorageConnection.cpp currently does NS_ProxyRelease() to the main thread always.

But clearly connections created off-main-thread with an nsStandardUrl cannot be finally released on the main thread.

It seems there is a deeper thread safety issue with the connection here.
Also, ~Connection() may trigger ~Service().  Is that really safe to do on anything thread?
Comment on attachment 8593385 [details] [diff] [review]
Proxy release the connection in mozStorageService::unregisterConnection(). r=asuth

Dropping review until we have a better plan.
Attachment #8593385 - Flags: review?(bugmail)
Based on IRC discussion we decided to proceed with the NS_ProxyRelease() in unregisterConnection().  The other NS_ProxyRelease() calls are for AsyncConnections which are expected to only work on main thread.

This green try tests equivalent code to this patch:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcd1781e29ad
Attachment #8593385 - Attachment is obsolete: true
Attachment #8593573 - Flags: review?(bugmail)
Attachment #8593573 - Flags: review?(bugmail) → review+
https://hg.mozilla.org/mozilla-central/rev/378a161d2a18
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.