mozStorageService::unregisterConnection() should proxy release
RESOLVED
FIXED
in Firefox 40
Status
()
People
(Reporter: bkelly, Assigned: bkelly)
Tracking
Firefox Tracking Flags
(firefox40 fixed)
Details
Attachments
(1 attachment, 2 obsolete attachments)
1.76 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
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]
(Assignee) | ||
Comment 1•4 years ago
|
||
Created attachment 8593375 [details] [diff] [review] Proxy release the connection in mozStorageService::unregisterConnection(). r=asuth https://treeherder.mozilla.org/#/jobs?repo=try&revision=72b3f17c9889
Attachment #8593375 -
Flags: review?(bugmail)
(Assignee) | ||
Comment 2•4 years ago
|
||
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
Attachment #8593375 -
Attachment is obsolete: true
Attachment #8593375 -
Flags: review?(bugmail)
Attachment #8593385 -
Flags: review?(bugmail)
(Assignee) | ||
Comment 3•4 years ago
|
||
Hmm, try is unhappy. Is it possible for a connection to be nullptr in unregisterConnection()?
(Assignee) | ||
Comment 4•4 years ago
|
||
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•4 years ago
|
||
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?
(Assignee) | ||
Comment 6•4 years ago
|
||
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•4 years ago
|
||
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.
(Assignee) | ||
Comment 8•4 years ago
|
||
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.
(Assignee) | ||
Comment 9•4 years ago
|
||
Also, ~Connection() may trigger ~Service(). Is that really safe to do on anything thread?
(Assignee) | ||
Comment 10•4 years ago
|
||
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)
(Assignee) | ||
Comment 11•4 years ago
|
||
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
Attachment #8593385 -
Attachment is obsolete: true
Attachment #8593573 -
Flags: review?(bugmail)
Updated•4 years ago
|
Attachment #8593573 -
Flags: review?(bugmail) → review+
(Assignee) | ||
Updated•4 years ago
|
Keywords: checkin-needed
(Assignee) | ||
Updated•4 years ago
|
Keywords: checkin-needed
(Assignee) | ||
Comment 12•4 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/378a161d2a18
Comment 13•4 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/378a161d2a18
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•