Closed
Bug 473845
Opened 16 years ago
Closed 16 years ago
Break a potential cycle with XPConnect and mozStorageService
Categories
(Core :: SQLite and Embedded Database Bindings, defect, P2)
Core
SQLite and Embedded Database Bindings
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: sdwilsh, Assigned: sdwilsh)
References
Details
(Keywords: fixed1.9.1, memory-leak)
Attachments
(1 file, 3 obsolete files)
9.79 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
We seem to be creating cycles with XPConnect and mozStorageService. We should just release the reference we hold at xpcom-shutdown in storage.
Locally, this makes browser chrome leaks go to zero. yay!
Attachment #357257 -
Flags: review?(bugmail)
Comment 1•16 years ago
|
||
Comment on attachment 357257 [details] [diff] [review]
v1.0
>+ "Did not properlyshut down mozStorageService!");
Nit: missing space.
Updated•16 years ago
|
Flags: blocking1.9.1?
Updated•16 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Assignee | ||
Updated•16 years ago
|
Attachment #357257 -
Flags: review?(bugmail) → review?(bent.mozilla)
Assignee | ||
Updated•16 years ago
|
Target Milestone: --- → mozilla1.9.2a1
Version: 1.9.1 Branch → Trunk
Comment on attachment 357257 [details] [diff] [review]
v1.0
As per our discussion let's just free the cached xpconnect on xpcom-shutdown and leave the sqlite shutdown order alone. Also fix the xpconnect() getter to handle this case so you can't leak xpconnect if one of your wrappers is still alive.
Attachment #357257 -
Flags: review?(bent.mozilla) → review-
Comment 3•16 years ago
|
||
Drop the leak threshold to 0 as well if the follow up patch works as well as
the existing one does.
Comment 4•16 years ago
|
||
Like so?
This clears the leaks from browser-chrome on my machine, haven't tested on other platforms though. Only one thought is whether Init really needs to return a failure if we fail to get and use the observer service, the only consequence is a shutdown leak. But I guess if that happens things are pretty screwed anyway.
Attachment #357257 -
Attachment is obsolete: true
Attachment #357969 -
Flags: review?(sdwilsh)
Assignee | ||
Comment 5•16 years ago
|
||
Comment on attachment 357969 [details] [diff] [review]
simpler patch
The problem with this patch is that it's perfectly legal for someone to try and use xpconnect during xpcom-shutdown. bent and I discussed how to get around this yesterday though, so I'll post a new patch.
Attachment #357969 -
Flags: review?(sdwilsh) → review-
Assignee | ||
Comment 6•16 years ago
|
||
Attachment #357969 -
Attachment is obsolete: true
Attachment #357988 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 7•16 years ago
|
||
And leaks are at zero locally.
Assignee | ||
Updated•16 years ago
|
Whiteboard: [has patch][needs review bent]
Oh, drat, I wasn't CC'd.
Comment on attachment 357988 [details] [diff] [review]
v3.0
>+already_AddRefed<nsIXPConnect>
> mozStorageService::XPConnect()
I think you could make this much simpler by using nsCOMPtr's forget method (this case is what it was esigned for):
nsCOMPtr<nsIXPConnect> retval;
if (sXPConnect) {
// normal
retval = sXPConnect;
}
else {
// shutdown
retval = do_GetService(nsIXPConnect::GetCID());
}
return retval.forget();
>+ // We chache XPConnect for our language helpers.
>+ (void)CallGetService(nsIXPConnect::GetCID(), &sXPConnect);
You don't check the return here so if that call failed you'd crash on shutdown in Shutdown's NS_RELEASE...
I think you should reverse the order of the calls in Init to do the observer service stuff first, then cache xpconnect, and use NS_IF_RELEASE in shutdown. That way you're guaranteed not to leak or crash on shutdown. Mossop is right, though, that if any of that fails we're screwed anyway. Meh.
Oh, and 'chache' is not spelled correctly in that comment i pasted above :)
Assignee | ||
Comment 11•16 years ago
|
||
Attachment #358020 -
Flags: review?(bent.mozilla)
Assignee | ||
Updated•16 years ago
|
Attachment #357988 -
Attachment is obsolete: true
Attachment #357988 -
Flags: review?(bent.mozilla)
Comment on attachment 358020 [details] [diff] [review]
v3.1
r=me.
Attachment #358020 -
Flags: review?(bent.mozilla) → review+
Assignee | ||
Comment 13•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs review bent]
Assignee | ||
Comment 14•16 years ago
|
||
Leak threshold was different for 1.9.1, so I had to manually fix that, but no other changes were needed:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f7c35e9f75cb
Keywords: fixed1.9.1
Comment 16•16 years ago
|
||
I had to commit http://hg.mozilla.org/mozilla-central/rev/64ff884c9127 to get my build to start properly.
Assignee | ||
Comment 17•16 years ago
|
||
(In reply to comment #14)
> Leak threshold was different for 1.9.1, so I had to manually fix that, but no
> other changes were needed:
But linux seems fine, so I suspect we are fine across the board with it.
Comment 18•16 years ago
|
||
My recent builds from 1.9.1 branch have no bookmarks in the menu or sidebar with f7c35e9f75cb applied. I have to back it out or apply 64ff884c9127 to restore bookmarks. Can we get this fix checked into the branch?
Comment 19•16 years ago
|
||
Updated•2 months ago
|
Product: Toolkit → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•