Closed Bug 457743 Opened 11 years ago Closed 11 years ago

Automatic wrapper creates cycles, and thus leaks

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.1b2

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

(Keywords: memory-leak, regression, verified1.9.1)

Attachments

(1 file, 2 obsolete files)

http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageStatementParams.h#61
http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageStatementRow.h#67

These should be raw pointers - we don't need to hold a reference since:
1) when these are used by the automatic wrapper, the statement *must* exists
2) when used by the explicit wrapper (depreciated), they are strongly held onto as long as the wrapper exists, and thus the statement must (http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageStatementWrapper.h#79)
Attached patch v1.0 (obsolete) — Splinter Review
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Attachment #342097 - Flags: review?(dcamp)
Whiteboard: [has patch][needs review dcamp]
As mentioned on irc, a dumb consumer COULD create a wrapper, get a reference to
the row, destroy the wrapper, and then crash trying to access the row (even though it's clearly documented that the row is invalid without an accompanying statement).  Seems like it'd be reasonable for the wrappers to invalidate/close out the row/params when it goes away.
mozStorageStatement also has the same issue
Whiteboard: [has patch][needs review dcamp] → [needs new patch]
Blocks: 464202
Flags: blocking1.9.1?
Keywords: mlk
Attached patch v1.1 (obsolete) — Splinter Review
Attachment #342097 - Attachment is obsolete: true
Attachment #347626 - Flags: review?
Attachment #342097 - Flags: review?(dcamp)
Attachment #347626 - Flags: review? → review?(dcamp)
Whiteboard: [needs new patch] → [has patch][needs review dcamp]
Attachment #347626 - Flags: review?(dcamp) → review+
Flags: blocking1.9.1? → blocking1.9.1+
Target Milestone: --- → mozilla1.9.1
Whiteboard: [has patch][needs review dcamp] → [has patch][has review][can land]
Blocks: 431558
I plan to land this after the beta.
Whiteboard: [has patch][has review][can land] → [has patch][has review][needs approval]
(In reply to comment #6)
> I plan to land this after the beta.

Why? Like bug 464202, I think this should go into b2. We have had zero tolerance for leaks for a while now, this one just slipped by because we can't yet set the leak treshold to 0 for browser-chrome tests. If we could, the patch that caused this (obvious) leak would have been backed out due to orange tinderboxes.
(In reply to comment #7)
> Why? Like bug 464202, I think this should go into b2. We have had zero
> tolerance for leaks for a while now, this one just slipped by because we can't
> yet set the leak treshold to 0 for browser-chrome tests. If we could, the patch
> that caused this (obvious) leak would have been backed out due to orange
> tinderboxes.
Mostly because this isn't used in any core code - just test files.  That, and I don't want to have to watch the tree with the current state it's been in.  If someone else wants to land these, by all means, please do so.
Is it me or does the block added to mozStorageStatement::Finalize only take effect if you call Finalize twice?  If the statement exists, control flows into the if-case which returns, never leaving that block.
(In reply to comment #9)
> Is it me or does the block added to mozStorageStatement::Finalize only take
> effect if you call Finalize twice?  If the statement exists, control flows into
> the if-case which returns, never leaving that block.
Yup.  For whatever reason I thought that was if (!mDBStatement), but it's clearly if (mDBStatement).  Ugh.  New patch soon.
Whiteboard: [has patch][has review][needs approval]
Attachment #347626 - Attachment is obsolete: true
Attachment #348582 - Flags: review?(bugmail)
Whiteboard: [has patch][needs review asuth]
Comment on attachment 348582 [details] [diff] [review]
v1.2
[Checkin: Comment 20]

>diff --git a/storage/src/mozStorageStatementJSHelper.cpp b/storage/src/mozStorageStatementJSHelper.cpp
>--- a/storage/src/mozStorageStatementJSHelper.cpp
>+++ b/storage/src/mozStorageStatementJSHelper.cpp
>@@ -110,17 +110,17 @@ mozStorageStatementJSHelper::getRow(mozS
> {
>   nsresult rv;
> 
>   PRInt32 state;
>   (void)aStatement->GetState(&state);
>   if (state != mozIStorageStatement::MOZ_STORAGE_STATEMENT_EXECUTING)
>     return NS_ERROR_UNEXPECTED;
> 
>-  if (!aStatement->mStatementRowHolder) {
>+  if (!aStatement->mStatementRowHolder && aStatement->mDBStatement) {

nit: I'd suggest not adding the mDBStatement check.  The state-check already ensures that aStatement->mDBStatement is non-null (otherwise the state would be MOZ_STORAGE_STATEMENT_INVALID).  My concern is that the code looks buggy with the additional constraint, since it looks like the 'if' fails to conceive of the possibility that both mStatementRowHolder and mDBStatement are null, allowing a null de-reference to occur in the call to GetJSObject.  This can't happen, of course, but it looks like it could.

Perhaps add a comment about mDBStatement being known valid and an explanation of why that code cares if this is intended.
Attachment #348582 - Flags: review?(bugmail) → review+
Attachment #348582 - Flags: approval1.9.1b2?
Comment on attachment 348582 [details] [diff] [review]
v1.2
[Checkin: Comment 20]

This would also be nice to get in b2, because it prevents a leak of SQL statements and a couple miscellaneous objects when using named parameters in JS.  It will fix the remaining leak running the specific test mentioned here (after bug 464202 is committed, which will be landing soon).  Extensions, as one of the more likely candidates for using this API, would benefit from it not leaking if they investigate potential leaks caused by their extensions (no false positives).  Last, it's not a very complicated patch, and the life cycle interactions here are well-understood, so it should be safe.
Whiteboard: [has patch][needs review asuth] → [has patch][needs approval]
Comment on attachment 348582 [details] [diff] [review]
v1.2
[Checkin: Comment 20]

We're closing down for beta 2 at the moment, so we'll have to take this after branch. Find me on IRC if you disagree vehemently.
Attachment #348582 - Flags: approval1.9.1b2?
Attachment #348582 - Flags: approval1.9.1b2-
Attachment #348582 - Flags: approval1.9.1?
This needs to be fixed for b2 if bug 464202 actually blocks, because that bug requires this fix to actually work.
Target Milestone: mozilla1.9.1 → mozilla1.9.1b2
That makes sense; this should be a b2 blocker.
Comment on attachment 348582 [details] [diff] [review]
v1.2
[Checkin: Comment 20]

Resetting "approval1.9.1b2=?" per previous comments.
Attachment #348582 - Flags: approval1.9.1b2- → approval1.9.1b2?
Comment on attachment 348582 [details] [diff] [review]
v1.2
[Checkin: Comment 20]

(b2 blockers don't need a+)

This is FIXED now, right?
Attachment #348582 - Flags: approval1.9.1b2?
Attachment #348582 - Flags: approval1.9.1?
Yeah, just waiting for some cycling first.

WARNING leaked 293147 bytes during test execution ---> WARNING leaked 73506 bytes during test execution

Whee!
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs approval] → [has patch]
Comment on attachment 348582 [details] [diff] [review]
v1.2
[Checkin: Comment 20]

http://hg.mozilla.org/mozilla-central/rev/20245c2d97d0
Attachment #348582 - Attachment description: v1.2 → v1.2 [Checkin: Comment 20]
V.Fixed, per bug 464202 comment 17.
Status: RESOLVED → VERIFIED
Keywords: regression
Whiteboard: [has patch]
Keywords: verified1.9.1
removing fixed1.9.1 b/c these bugs are verified1.9.1, sorry for the spam.
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.