1155193 - mozStorageService::unregisterConnection() should proxy release

Status: RESOLVED FIXED

Description

[Ben Kelly [:bkelly, not reviewing]]

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]

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Created attachment 8593375 [details] [diff] [review]
Proxy release the connection in mozStorageService::unregisterConnection(). r=asuth

https://treeherder.mozilla.org/#/jobs?repo=try&revision=72b3f17c9889

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Created attachment 8593385 [details] [diff] [review]
Proxy release the connection in mozStorageService::unregisterConnection(). r=asuth

Slightly better patch as it does not AddRef() off the opening thread either.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=04c5f14a8d38

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Hmm, try is unhappy.  Is it possible for a connection to be nullptr in unregisterConnection()?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 5

[Andrew Sutherland [:asuth]]

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?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 7

[Andrew Sutherland [:asuth]]

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.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Also, ~Connection() may trigger ~Service().  Is that really safe to do on anything thread?

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8593385 [details] [diff] [review]
Proxy release the connection in mozStorageService::unregisterConnection(). r=asuth

Dropping review until we have a better plan.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Created attachment 8593573 [details] [diff] [review]
Proxy release the Connection in mozStorageService::unregisterConnection(). r=asuth

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

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/378a161d2a18

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/378a161d2a18