mozStorageService::unregisterConnection() should proxy release

RESOLVED FIXED in Firefox 40

Status

()

Toolkit
Storage
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
mozilla40
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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

3 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

3 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

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

Comment 4

3 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
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

3 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.
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

3 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

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

Comment 10

3 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)
Blocks: 1152019
(Assignee)

Comment 11

3 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)
Attachment #8593573 - Flags: review?(bugmail) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/378a161d2a18
Status: ASSIGNED → RESOLVED
Last Resolved: 3 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.