Marco Bonardo [:mak] Reporter
	Description • 16 years ago

i don't know if this should be a storage bug since to activate it i have to use nsPlacesDBFlush, so i suppose that's involved in the leak.

This test activates the leak, changing the param assignment to bintInt64Param deactivate the leak.

	Marco Bonardo [:mak] Reporter
	Comment 1 • 16 years ago

ps: final reset() should be finalize(), hwv that does not change results.

	Shawn Wilsher :sdwilsh Assignee
	Comment 3 • 16 years ago

Where did you place this test case to run it?

	Marco Bonardo [:mak] Reporter
	Comment 4 • 16 years ago

toolkit/places/tests/unit

	Marco Bonardo [:mak] Reporter
	Comment 5 • 16 years ago

ops i lost a /components/

	Shawn Wilsher :sdwilsh Assignee
	Comment 6 • 16 years ago

Found it with bent's help.  Totally a places bug...

	Dietrich Ayala (:dietrich)
	Comment 8 • 16 years ago

Comment on attachment 353890 [details] [diff] [review]
v1.0
[Checkin: Comment 10 & 17]

talked over irc, r=me

	Shawn Wilsher :sdwilsh Assignee
	Comment 9 • 16 years ago

OK - here's what was happening:
Since we had the getter defined on the prototype, the database would be held onto as long as xpconnect was alive (assuming it was accessed at least once).  The problem showed up when we used the mozStorageStatementJSHelper, which ends up making mozStorageService hold not XPConnect until it goes away.  This ended up creating a cycle though:
XPConnect held a reference to the places database connection
The places database connection held a reference to mozStorageService
mozStorageService held a reference to XPConnect

Fun times...

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

Comment on attachment 353890 [details] [diff] [review]
v1.0
[Checkin: Comment 10 & 17]

http://hg.mozilla.org/mozilla-central/rev/62902ec9424a

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

(In reply to comment #10)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20081220 Minefield/3.2a1pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/e45938aee309)

Iirc, Shawn said this patch fixed the testcase, but not the actual tests:

*Bug 470455 has a patch to fix its code... :-)
*Bug 469062 has been worked around for the time being. :-|
*Bug is still there ! :-(

	Shawn Wilsher :sdwilsh Assignee
	Comment 12 • 16 years ago

(In reply to comment #11)
> Iirc, Shawn said this patch fixed the testcase, but not the actual tests:
So, I'm going to keep this bug closed, since we've fixed this leak (the one that the testcase exposed).  Ideally, we'll get a new bug with another reduced test case, but I just verified that the testcase attached to this bug has been fixed (I had previously reduced it further, and hadn't verified that the original test case didn't leak).

This bug is closed as it stands, and I guess we'll try to tackle the other ones one at a time.

*sigh*

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

(In reply to comment #11)
> *Bug is still there ! :-(

Bug 465952 !

(In reply to comment #12)

Agreed...

Did this fix either of the following bugs ?
*Bug 469062 without its workaround.
*Bug 452899 with its currently attached patch.

	Marco Bonardo [:mak] Reporter
	Comment 14 • 16 years ago

(In reply to comment #13)
> Did this fix either of the following bugs ?
> *Bug 469062 without its workaround.

yes

> *Bug 452899 with its currently attached patch.

yes

	Marco Bonardo [:mak] Reporter
	Comment 15 • 16 years ago

(In reply to comment #14)
> > *Bug 452899 with its currently attached patch.
> 
> yes

ops sorry. i thought this was the embed bug, nevermind me.

	Marco Bonardo [:mak] Reporter
	Comment 16 • 16 years ago

however embed test is good to check this worked, because the actual patch in bug 470455 would not be enough to fix it completely without this.

	Dão Gottwald [:dao]
	Comment 17 • 16 years ago

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b759c7a325f0