868313 - [Storage] Many occurrences of thread-unsafe deallocation

Status: REOPENED

(Core :: SQLite and Embedded Database Bindings, defect, P3)

Description

[David Teller [:Yoric] - still alive but not very active]

Attachment: Activating connection release sanity check

While testing for bug 702559, I encountered what looks like a bad case of connections being released on the wrong thread under Linux (many occurrences) and MacOS X (a few). In my experience, this can be a cause of segfaults.

See https://tbpl.mozilla.org/?tree=Try&rev=71bf0058e15c

Comment 1

[Marco Bonardo [:mak]]

Comment on attachment 745016 [details] [diff] [review]
Activating connection release sanity check

Review of attachment 745016 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand the patch, what's the point of #if 1 here?
There are 2 cases: either there is a good reason for this code to not be always enabled and then the current code makes sense, or there is no valid reason and it should be always enabled. The current commenting out makes me think there must be a reason, though considered it's debug-only I can't recall what that may be.
Is Try happy with this change?

This has been added by Nicholas Nethercote in bug 702815, I guess you may have more lucky directly asking him.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

|#if 1| itself is because I believe that we do not like commenting out code as a source control technique. But I initially activated that code to help me track bugs in my patch for bug 702559, until I realized that most of the issues I witnessed were already visible without my patch.

Nicholas, is this check something that should pass or is this some leftover debugging code?

Comment 3

[Nicholas Nethercote [inactive]]

I just copied that code from NS_IMPL_THREADSAFE_RELEASE (in xpcom/glue/nsISupportsImpl.h).  So I don't have any additional wisdom to add, sorry.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Was checking out thread-related bugs and was wondering about this.  As an update, NS_IMPL_RELEASE_WITH_DESTROY has revised the relevant blurb:
  if (!mRefCnt.isThreadSafe)
    NS_ASSERT_OWNINGTHREAD(_class); 

So that's what storage would want to do, although it seems like it might be better to have the whole "a service owns me" thing handled as an additional macro (argument?) in nsISupportsImpl.h to do the count == 1 case.

Comment 5

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.