Closed
Bug 596970
Opened 14 years ago
Closed 14 years ago
ASSERTION: You can't dereference a NULL nsCOMPtr with operator->() in AsyncStatement::~AsyncStatement()
Categories
(Toolkit :: Storage, defect)
Toolkit
Storage
Tracking
()
RESOLVED
FIXED
mozilla2.0b7
People
(Reporter: mak, Assigned: mak)
References
Details
Attachments
(1 file, 1 obsolete file)
751 bytes,
patch
|
asuth
:
review+
dietrich
:
approval2.0+
|
Details | Diff | Splinter Review |
Connection *forgottenConn = nsnull; mDBConnection.swap(forgottenConn); (void)::NS_ProxyRelease(mDBConnection->threadOpenedOn, forgottenConn); mDBConnection is nsnull and NS_ProxyRelease is trying to use it. See bug 556631 comment 11 and following.
Assignee | ||
Updated•14 years ago
|
Summary: ASSERTION: You can't dereference a NULL nsCOMPtr with operator->() in AsynStatement::~AsyncStatement() → ASSERTION: You can't dereference a NULL nsCOMPtr with operator->() in AsyncStatement::~AsyncStatement()
Assignee | ||
Comment 1•14 years ago
|
||
So just replacing mDBConnection->threadOpenedOn with forgottenConn solves the problem. but I'm unsure if there are other changes wanted based on Asuth's comments. Also I have no idea how to write a test to touch this code.
Assignee | ||
Comment 2•14 years ago
|
||
I meant replacing with forgottenConn->threadOpenedOn
Comment 3•14 years ago
|
||
That certainly sounds reasonable to me, especially given that that is what the code was meant to do. One could write a test by having a C++ test where we spin up 2 non-main threads... one opens the connection, and then the other one issues an async statement. If we don't assert/crash, then all is well. I'm not sure if sdwilsh wants that level of coverage or to claim that we support such a use case flawlessly though. :)
Comment 4•14 years ago
|
||
(In reply to comment #3) > I'm not sure if sdwilsh wants that level of coverage or to claim that we > support such a use case flawlessly though. :) I don't think I care about it that much.
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•14 years ago
|
||
I seem to understand testing this edge case is unneeded.
Attachment #476061 -
Flags: review?(bugmail)
Assignee | ||
Comment 6•14 years ago
|
||
gah, the right patch.
Attachment #476061 -
Attachment is obsolete: true
Attachment #476062 -
Flags: review?(bugmail)
Attachment #476061 -
Flags: review?(bugmail)
Comment 7•14 years ago
|
||
Comment on attachment 476062 [details] [diff] [review] patch v1.0 Thanks for the fix!
Attachment #476062 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 8•14 years ago
|
||
Comment on attachment 476062 [details] [diff] [review] patch v1.0 Requesting approval on this one-line change, it is blocking my work on a blocker.
Attachment #476062 -
Flags: approval2.0?
Updated•14 years ago
|
Attachment #476062 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 9•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/5bb4f093a50b
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Updated•14 years ago
|
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in
before you can comment on or make changes to this bug.
Description
•