457743 - Automatic wrapper creates cycles, and thus leaks

Status: VERIFIED FIXED

(Toolkit :: Storage, defect)

Description

[Shawn Wilsher :sdwilsh]

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)

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 2

[Dave Camp (: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.

Comment 3

[Shawn Wilsher :sdwilsh]

mozStorageStatement also has the same issue

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Comment 5

[Serge Gautherie (:sgautherie)]

Shawn, will you add the patch to
https://wiki.mozilla.org/1.9.1_beta_2_checkins ?

Comment 6

[Shawn Wilsher :sdwilsh]

I plan to land this after the beta.

Comment 7

[Peter Van der Beken [:peterv]]

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

Comment 8

[Shawn Wilsher :sdwilsh]

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

Comment 9

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

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.

Comment 10

[Shawn Wilsher :sdwilsh]

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

Comment 11

[Shawn Wilsher :sdwilsh]

Attachment: v1.2 [Checkin: Comment 20]

Comment 12

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

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.

Comment 13

[Jeff Walden [:Waldo]]

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.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 15

[Jeff Walden [:Waldo]]

This needs to be fixed for b2 if bug 464202 actually blocks, because that bug requires this fix to actually work.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

That makes sense; this should be a b2 blocker.

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 348582 [details] [diff] [review]
v1.2
[Checkin: Comment 20]

Resetting "approval1.9.1b2=?" per previous comments.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348582 [details] [diff] [review]
v1.2
[Checkin: Comment 20]

(b2 blockers don't need a+)

This is FIXED now, right?

Comment 19

[Jeff Walden [:Waldo]]

Yeah, just waiting for some cycling first.

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

Whee!

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 348582 [details] [diff] [review]
v1.2
[Checkin: Comment 20]

http://hg.mozilla.org/mozilla-central/rev/20245c2d97d0

Comment 21

[Serge Gautherie (:sgautherie)]

V.Fixed, per bug 464202 comment 17.

Comment 22

[Nochum Sossonko [:Natch]]

removing fixed1.9.1 b/c these bugs are verified1.9.1, sorry for the spam.