Closed
Bug 452899
Opened 16 years ago
Closed 15 years ago
Don't explicitly create the statement wrapper - storage does it for us
Categories
(Toolkit :: Password Manager, defect)
Toolkit
Password Manager
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: sdwilsh, Assigned: zpao)
References
Details
Attachments
(4 files, 2 obsolete files)
9.08 KB,
text/plain
|
Details | |
1.34 KB,
patch
|
Details | Diff | Splinter Review | |
3.11 KB,
patch
|
Details | Diff | Splinter Review | |
11.62 KB,
text/plain
|
Details |
Bug 452897 makes it so that we don't need to manually create wrapper objects anymore for storage statements. Hurray for less code!
Reporter | ||
Updated•16 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: in-testsuite-
Flags: in-litmus-
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.
Assignee | ||
Comment 2•16 years ago
|
||
Line numbers are off since this patch is on top of the patch for bug 451267
Reporter | ||
Updated•16 years ago
|
Attachment #338393 -
Flags: review?(sdwilsh)
Attachment #338393 -
Flags: review?(dolske)
Attachment #338393 -
Flags: review+
Reporter | ||
Comment 3•16 years ago
|
||
whoops - forgot to comment. r=sdwilsh
Updated•16 years ago
|
Attachment #338393 -
Flags: review?(dolske) → review+
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?
Reporter | ||
Updated•16 years ago
|
Keywords: checkin-needed
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
Attachment #338393 -
Attachment description: Patch v0.1 → Patch v0.1
[Checkin: Comment 5]
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
Updated•16 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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 :-()
Attachment #338393 -
Attachment description: Patch v0.1
[Checkin: Comment 5] → Patch v0.1
[Backout: Comment 6]
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?
Reporter | ||
Comment 8•16 years ago
|
||
It looks to me like unrelated things were leaking. Maybe we should try to land again?
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.
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.
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.
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.
Updated•16 years ago
|
Target Milestone: mozilla1.9.1b1 → mozilla1.9.1
Updated•16 years ago
|
Attachment #338393 -
Flags: approval1.9.1?
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)
Updated•16 years ago
|
Attachment #338393 -
Flags: approval1.9.1? → approval1.9.1+
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)
Reporter | ||
Comment 15•16 years ago
|
||
I'm fairly certain we have the leaks fixed (they were caused by storage)
Keywords: checkin-needed
Updated•16 years ago
|
Attachment #338393 -
Attachment description: Patch v0.1
[Backout: Comment 6] → Patch v0.1
[Checkin: Comment 16]
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
Updated•16 years ago
|
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Whiteboard: [c-n: baking for 1.9.1]
Updated•16 years ago
|
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Whiteboard: [c-n: baking for 1.9.1]
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 }
Attachment #338393 -
Attachment description: Patch v0.1
[Checkin: Comment 16] → Patch v0.1
[Backout: Comment 17]
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.
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.
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?
Reporter | ||
Comment 21•16 years ago
|
||
garbage collection will finalize them anyway, and we have to garbage collect on shutdown...
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 }
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
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.
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?
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...)
Status: REOPENED → NEW
Depends on: 465952
Updated•16 years ago
|
Updated•16 years ago
|
Reporter | ||
Updated•16 years ago
|
Comment 27•16 years ago
|
||
Does bug 473845 fix help here ?
Reporter | ||
Comment 28•16 years ago
|
||
Yes, it should.
Assignee | ||
Comment 29•16 years ago
|
||
Shawn says "this should absolutely no leak anymore", so let's see if we can land this
Keywords: checkin-needed
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 }
Attachment #338393 -
Attachment description: Patch v0.1
[Backout: Comment 17] → Patch v0.1
[Checkin: Comment 30]
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Whiteboard: [needs 1.9.1 landing]
Target Milestone: mozilla1.9.1 → mozilla1.9.2a1
Updated•16 years ago
|
Whiteboard: [needs 1.9.1 landing] → [needs 1.9.1 landing][cursed]
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 :-(
Attachment #338393 -
Attachment description: Patch v0.1
[Checkin: Comment 30] → Patch v0.1
[Backout: Comment 31]
Attachment #338393 -
Attachment is obsolete: true
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.
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Whiteboard: [needs 1.9.1 landing][cursed] → [cursed]
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.]
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.
Comment 35•16 years ago
|
||
SDWILSH!!!! Clean, current tree --> make check is ok Apply Patch v.2 --> JS asserts.
Reporter | ||
Comment 36•16 years ago
|
||
Places uses this wrapper all over I swear!
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.
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. :)
Attachment #338393 -
Flags: approval1.9.1+
Reporter | ||
Comment 39•16 years ago
|
||
Stack as requested for Waldo
Comment 40•16 years ago
|
||
Attachment #368624 -
Attachment is obsolete: true
Comment 41•16 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)] ------------------------------------------------------------------------
Comment 42•16 years ago
|
||
The JS assert is a JS engine bug, will investigate and file a bug blocking this when I get it sufficiently reduced.
Comment 43•16 years ago
|
||
Comment 44•16 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.
Updated•16 years ago
|
Attachment #384156 -
Attachment is patch: true
Reporter | ||
Comment 45•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/3a1f6d6aaea8
Status: REOPENED → RESOLVED
Closed: 16 years ago → 15 years ago
Resolution: --- → FIXED
Whiteboard: [cursed]
Updated•15 years ago
|
Attachment #383980 -
Attachment description: Patch v2 unbitrotted → Patch v2 unbitrotted
[Checkin: Comment 45]
You need to log in
before you can comment on or make changes to this bug.
Description
•