Shawn Wilsher :sdwilsh Reporter
	Description • 16 years ago

Bug 452897 makes it so that we don't need to manually create wrapper objects anymore for storage statements.  Hurray for less code!

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 1 • 16 years ago

This should just be a 3 line patch since all statements are being created and then wrapped at a single point.

As soon as bug 452897 is done, I'll whip this up.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 2 • 16 years ago

Line numbers are off since this patch is on top of the patch for bug 451267

	Shawn Wilsher :sdwilsh Reporter
	Comment 3 • 16 years ago

whoops - forgot to comment.

r=sdwilsh

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 4 • 16 years ago

Should I add the "checkin-needed" keyword since I don't have commit access? Or can one of you just commit it?

	Serge Gautherie (:sgautherie)
	Comment 5 • 16 years ago

Comment on attachment 338393 [details] [diff] [review]
Patch v0.1
[Backout: Comment 31]

http://hg.mozilla.org/mozilla-central/rev/fa8fbd81159d

	Serge Gautherie (:sgautherie)
	Comment 6 • 16 years ago

Comment on attachment 338393 [details] [diff] [review]
Patch v0.1
[Backout: Comment 31]

[
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1221925081.1221929389.10223.gz
Linux mozilla-central qm-centos5-03 dep unit test on 2008/09/20 08:38:01
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 60704 bytes during test execution (should have leaked no more than 0 bytes)

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1221925470.1221929904.14697.gz
WINNT 5.2 mozilla-central qm-win2k3-03 dep unit test on 2008/09/20 08:44:30
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 63920 bytes during test execution (should have leaked no more than 0 bytes)
]

Backout and merge:
http://hg.mozilla.org/mozilla-central/rev/783541270c29
http://hg.mozilla.org/mozilla-central/rev/e0e988b76329
(NB: I wrote a wrong bug number on the merge :-()

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 7 • 16 years ago

Not sure what the problem was here. This shouldn't have caused leaks in all those places... If it wasn't leaking before, it shouldn't be now.

Thoughts Shawn, Justin?

	Shawn Wilsher :sdwilsh Reporter
	Comment 8 • 16 years ago

It looks to me like unrelated things were leaking.  Maybe we should try to land again?

	Justin Dolske [:Dolske]
	Comment 9 • 16 years ago

The tree wasn't in a great state that day, so it's possible that it was caused by something else. OTOH, the leaks disappeared when this was backed out, and I don't see any other backouts immediately around it.

The leak sure seem related to me:

leaked 2 instances of mozStorageConnection with size 72 bytes each
leaked 1 instance of mozStorageFunctionGetUnreversedHost with size 8 bytes
leaked 1 instance of mozStorageService with size 24 bytes
leaked 34 instances of mozStorageStatement with size 40 bytes each
leaked 1 instance of mozStorageStatementParams with size 20 bytes
leaked 2 instances of mozStorageStatementRow with size 16 bytes each

I'd be a bit wary of an experimental landing, since a freeze is coming up tomorrow. Maybe after that, if a cause isn't found.

	Shawn Wilsher :sdwilsh Reporter
	Comment 10 • 16 years ago

I'm also seeing nsNavBookmarks being leaked there, which doesn't have any ties at all with the password manager though.

	Shawn Wilsher :sdwilsh Reporter
	Comment 11 • 16 years ago

storage creates a cycle, which is likely the cause of the leak.  I've filed bug 457743 on it; it's an easy fix if someone wanted to pick it up.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 12 • 16 years ago

I have no free cycles, I'm stretched thin as it is.

I do agree with Justin that it might be best to wait until after the freeze, as long as it doesn't get forgotten.

	Justin Dolske [:Dolske]
	Comment 13 • 16 years ago

Comment on attachment 338393 [details] [diff] [review]
Patch v0.1
[Backout: Comment 31]

sdwilsh says the leak was fixed elsewhere, so this should be good to land again (post-B2, which I presume will need a191 since this isn't a blocker)

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 14 • 16 years ago

Comment on attachment 338393 [details] [diff] [review]
Patch v0.1
[Backout: Comment 31]

a191=beltzner, watch for leaks as she goes (try to land it by itself)

	Shawn Wilsher :sdwilsh Reporter
	Comment 15 • 16 years ago

I'm fairly certain we have the leaks fixed (they were caused by storage)

	Serge Gautherie (:sgautherie)
	Comment 16 • 16 years ago

Comment on attachment 338393 [details] [diff] [review]
Patch v0.1
[Backout: Comment 31]

http://hg.mozilla.org/mozilla-central/rev/c8eb8a8f42d9

	Serge Gautherie (:sgautherie)
	Comment 17 • 16 years ago

Comment on attachment 338393 [details] [diff] [review]
Patch v0.1
[Backout: Comment 31]

Backed out:
http://hg.mozilla.org/mozilla-central/rev/3ffab81215a0
http://hg.mozilla.org/mozilla-central/rev/0eacac1fe6b9
due to
{
RLk:0B
Lk:1.01MB
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228412714.1228414421.8191.gz
Linux mozilla-central leak test build on 2008/12/04 09:45:14
RLk:64.6KB
Lk:3.58MB

RLk:0B
Lk:2.21MB
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228413079.1228414327.7801.gz
OS X 10.5.2 mozilla-central leak test build on 2008/12/04 09:51:19
RLk:64.1KB
Lk:4.19MB
}

	Shawn Wilsher :sdwilsh Reporter
	Comment 18 • 16 years ago

So, there was another patch in the window that may have also caused the leak.  Given that our unit tests for this storage code no longer leak, I'm inclined to believe that the other patch caused the leak.

	Justin Dolske [:Dolske]
	Comment 19 • 16 years ago

Linux mozilla-central leak test build had a good run with the changeset immediately  before this was checked in (9e3d35aea6ee), and its next run was for this changeset only (c8eb8a8f42d9) which leaked. When it was backed out, it the first green cycle was the one that contained only the backout changeset.

So, I think it's fairly clear this did cause the leak.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 20 • 16 years ago

It's odd that this leaks. The only possibility I can think of is that the memoizing is causing issues. We only finalize the statements if there's an error initializing. Maybe we should observe shutdown and finalize statements then?

That's the only theory I have, though we weren't doing that before, so statements weren't getting finalized (Shawn - you said they didn't need to be). Maybe they do now.

Is there another component that has been updated recently like this?

	Shawn Wilsher :sdwilsh Reporter
	Comment 21 • 16 years ago

garbage collection will finalize them anyway, and we have to garbage collect on shutdown...

	Serge Gautherie (:sgautherie)
	Comment 22 • 16 years ago

(In reply to comment #17)

It looks like the (same) leak is still mostly there:
{
...
leaked 1 instance of mozStorageConnection with size 80 bytes
leaked 1 instance of mozStorageFunctionGetUnreversedHost with size 8 bytes
leaked 1 instance of mozStorageService with size 16 bytes
leaked 32 instances of mozStorageStatement with size 40 bytes each (1280 bytes total)
...
}
from
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228415215.1228417890.16316.gz
Linux mozilla-central moz2-linux-slave07 dep unit test on 2008/12/04 10:26:55
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 62684 bytes during test execution
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 63974 bytes during test execution

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228415320.1228422133.27975.gz
MacOSX Darwin 9.2.2 mozilla-central moz2-darwin8-slave02 dep unit test on 2008/12/04 10:28:40
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 62204 bytes during test execution
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 63518 bytes during test execution

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228412719.1228416788.13801.gz
WINNT 5.2 mozilla-central moz2-win32-slave08 dep unit test on 2008/12/04 09:45:19
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 62988 bytes during test execution
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 64454 bytes during test execution

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228413476.1228416938.14100.gz
WINNT 5.2 mozilla-central moz2-win32-slave07 dep unit test on 2008/12/04 09:57:56
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 62988 bytes during test execution
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 64454 bytes during test execution
}

	Shawn Wilsher :sdwilsh Reporter
	Comment 23 • 16 years ago

Well now that's interesting since that exists nowhere in this code:
http://mxr.mozilla.org/mozilla-central/search?string=mozStorageFunctionGetUnreversedHost

	Justin Dolske [:Dolske]
	Comment 24 • 16 years ago

(In reply to comment #22)
> It looks like the (same) leak is still mostly there:

Huh? Those leaks are all from the builds between when this landed and was backed out. We already know it leaked.

	Shawn Wilsher :sdwilsh Reporter
	Comment 25 • 16 years ago

(In reply to comment #24)
> Huh? Those leaks are all from the builds between when this landed and was
> backed out. We already know it leaked.
Silly me - I had totally forgotten that places was leaking before.  We leak less now (the storage leak went away).  What's really strange is how this change is managing to leak the places stuff though.  I'm not really sure how that stuff is being held onto :(

Do we know what test is leaking by chance?

	Serge Gautherie (:sgautherie)
	Comment 26 • 16 years ago

(In reply to comment #24)
> Huh? Those leaks are all from the builds between when this landed and was
> backed out. We already know it leaked.

That's exactly what these leaks are:
my comment was meant to record some details about the (updated) leak,
and to say that previous comments (before relanding) about the leak being (fully) fixed seem optimistic rather than verified...

(In reply to comment #25)
> Do we know what test is leaking by chance?

No idea: we only have the tinderbox logs yet.
Paul, or someone else, should really do a local build and look into this.
(See also your bug 465952 comment 5...)

	Serge Gautherie (:sgautherie)
	Comment 27 • 16 years ago

Does bug 473845 fix help here ?

	Shawn Wilsher :sdwilsh Reporter
	Comment 28 • 16 years ago

Yes, it should.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 29 • 16 years ago

Shawn says "this should absolutely no leak anymore", so let's see if we can land this

	Serge Gautherie (:sgautherie)
	Comment 30 • 16 years ago

Comment on attachment 338393 [details] [diff] [review]
Patch v0.1
[Backout: Comment 31]


http://hg.mozilla.org/mozilla-central/rev/10e8371749f9

after fixing context for
{
patching file toolkit/components/passwordmgr/src/storage-mozStorage.js
Hunk #1 FAILED at 946
1 out of 2 hunks FAILED
}

	Serge Gautherie (:sgautherie)
	Comment 31 • 16 years ago

Comment on attachment 338393 [details] [diff] [review]
Patch v0.1
[Backout: Comment 31]


http://hg.mozilla.org/mozilla-central/rev/11d92035b807
http://hg.mozilla.org/mozilla-central/rev/49d7fee2e9b4
Backed out changeset: 10e8371749f9
As dolske says this patch doesn't apply to tip (anymore) !
That's only the 3rd time I checkin-needed and back this very patch out after all :-(

	Serge Gautherie (:sgautherie)
	Comment 32 • 16 years ago

Please, don't ask for checkin-needed again for this patch:
handle it directly with someone else who would be willing to check it in!
That's more than I can take.

	Justin Dolske [:Dolske]
	Comment 33 • 16 years ago

Updated to tip.

Currently rebuilding to test, my build seemed to be suffering from a JS assert unrelated to this. [...I hope.]

	Justin Dolske [:Dolske]
	Comment 34 • 16 years ago

Awesome. Clobbered, but |make check| still hits an JS assert in test_storage_legacy_5.js. Love this bug!

Assertion failure: ((jsval) obj & JSVAL_TAGMASK) == JSVAL_OBJECT, at /Users/dolske/build/mozilla-central/js/src/jsapi.h:118

My tree was a couple days old, so maybe this was just some transient thing that one of the tracemonkey merges has fixed.

	Justin Dolske [:Dolske]
	Comment 35 • 16 years ago

SDWILSH!!!!

Clean, current tree --> make check is ok
Apply Patch v.2 --> JS asserts.

	Shawn Wilsher :sdwilsh Reporter
	Comment 36 • 16 years ago

Places uses this wrapper all over I swear!

	Jeff Walden [:Waldo]
	Comment 37 • 16 years ago

Stack trace?  You're trying to convert a value that's not an object into an object somewhere, judging by the assertion, and odds are it's a bug in your use of the JSAPI -- but who knows, we've only had that assert for a little while now, so it could be SpiderMonkey's fault still.

	Justin Dolske [:Dolske]
	Comment 38 • 16 years ago

Comment on attachment 338393 [details] [diff] [review]
Patch v0.1
[Backout: Comment 31]

Clearing a191. No particular need to take this on branch, and let's not tempt Murphy. :)

	Shawn Wilsher :sdwilsh Reporter
	Comment 39 • 16 years ago

Stack as requested for Waldo

	Jeff Walden [:Waldo]
	Comment 41 • 15 years ago

------------------------------------------------------------------------
Hit JavaScript "debugger" keyword. JS call stack...
0 anonymous(params = [object Object], query = "INSERT INTO moz_logins (hostname, httpRealm, formSubmitURL, usernameField, passwordField, encryptedUsername, encryptedPassword, guid, encType) VALUES (:hostname, :httpRealm, :formSubmitURL, :usernameField, :passwordField, :encryptedUsername, :encryptedPassword, :guid, :encType)") ["file:///Users/jwalden/moz/background-size/obj-i386-apple-darwin8.11.1/dist/MinefieldDebug.app/Contents/MacOS/components/storage-mozStorage.js":1254]
    wrappedStmt = undefined
    this = [object Object]
1 anonymous(isEncrypted = false, login = [xpconnect wrapped nsILoginInfo @ 0x42713cf0 (native @ 0x3f4f4850)]) ["file:///Users/jwalden/moz/background-size/obj-i386-apple-darwin8.11.1/dist/MinefieldDebug.app/Contents/MacOS/components/storage-mozStorage.js":334]
    stmt = undefined
    params = [object Object]
    query = "INSERT INTO moz_logins (hostname, httpRealm, formSubmitURL, usernameField, passwordField, encryptedUsername, encryptedPassword, guid, encType) VALUES (:hostname, :httpRealm, :formSubmitURL, :usernameField, :passwordField, :encryptedUsername, :encryptedPassword, :guid, :encType)"
    encType = 1
    loginClone = [xpconnect wrapped (nsISupports, nsILoginInfo, nsILoginMetaInfo) @ 0x3eb021b0 (native @ 0x3eb010d0)]
    encPassword = "MDoEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECKjFeE53n2oKBBC5xoiyWvKR+4kTbdzNT98z"
    encUsername = "MDoEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECCzcxPfe3N/tBBA+wjm67L3CojyfsaUuirUo"
    userCanceled = false
    this = [object Object]
2 anonymous(login = [xpconnect wrapped nsILoginInfo @ 0x42713cf0 (native @ 0x3f4f4850)]) ["file:///Users/jwalden/moz/background-size/obj-i386-apple-darwin8.11.1/dist/MinefieldDebug.app/Contents/MacOS/components/storage-mozStorage.js":269]
    this = [object Object]
3 [native frame]
4 anonymous(login = [xpconnect wrapped nsILoginInfo @ 0x3f4f5a80 (native @ 0x3f4f4850)]) ["file:///Users/jwalden/moz/background-size/obj-i386-apple-darwin8.11.1/dist/MinefieldDebug.app/Contents/MacOS/components/nsLoginManager.js":438]
    logins =
    this = [object Object]
5 [native frame]
6 <TOP LEVEL> ["http://localhost:8888/tests/toolkit/components/passwordmgr/test/test_0init.html":28]
    this = [object Window @ 0x3f44b9d0 (native @ 0x3f44ac80)]
------------------------------------------------------------------------

	Jeff Walden [:Waldo]
	Comment 42 • 15 years ago

The JS assert is a JS engine bug, will investigate and file a bug blocking this when I get it sufficiently reduced.

	Jeff Walden [:Waldo]
	Comment 44 • 15 years ago

The loop at fault is the last one in _dbCreateStatement that copies from the passed-in params to the returned statement, if it wasn't clear, which means there isn't a whole lot to minimize.

	Shawn Wilsher :sdwilsh Reporter
	Comment 45 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/3a1f6d6aaea8