We leak the browser.xul of the old window.
JS implemented nsITransaction keeps it alive.
This adds the CC time significantly.
load a page which hasn't been bookmarked.
click the star twice, click done.
open a new window
close the old window.
(Btw, once Bug 726346 is fixed, you can install the addon from that bug.
Then after STR load about:cc
run CC and search for the name nsDocument.
Then take the address of that and search for roots and you get nsITransaction)
If I read the logs correctly, we seem to leak the window at least via uri wrapper
0x7faa56040160 XPCWrappedNative_NoHelper 7faa579a44e0
> 0x7faa5472df40 type
> 0x7faa5471d448 shape
> 0x7faa56040130 XPCWrappedNativeProto::mJSProtoObject
> 0x7faa5c5d04c0 XPCWrappedNativeScope::mGlobalJSObject
> 0x7faa5c568bc0 XPCWrappedNativeScope::mPrototypeJSObject
> 0x7faa54700640 equals
> 0x7faa547007c0 schemeIs
> 0x7faa547c5c40 clone
Created attachment 598266 [details] [diff] [review]
working on a better patch.
Created attachment 598413 [details] [diff] [review]
a bit larger than I was expecting, but I ended up refactoring it to reduce possibilities of future leak regressions. All data is handled through a common object that enforces copying.
Comment on attachment 598413 [details] [diff] [review]
I can't see any change. The document is still there and kept alive by
I have found the remaining problem, it's the annotation object we create in browser-places, so I'll also have to copy those :(
Created attachment 598519 [details] [diff] [review]
I tested this with your about:cc add-on, before the patch I see the browser.xul leak, after the patch it tells me no documents have been leaked.
The test coverage is given by test_placesTxn.js, that is a sucky test for the way it's written, but gives good coverage, I tried to avoid behavior changes as far as possible to reduce regressions, though the sooner we land the more testing we get, it's hard to get 100% coverage here.
Comment on attachment 598519 [details] [diff] [review]
Seems to work now. Thanks!
Created attachment 599606 [details] [diff] [review]
a test was correctly failing, I identified an unwanted behavior change and this restores it.
Comment on attachment 599606 [details] [diff] [review]
Review of attachment 599606 [details] [diff] [review]:
@@ +2342,5 @@
> + name: null,
> + _transactions: null,
> + set childTransactions(val) this._transactions =
> + (val && Array.isArray(val) ? Array.slice(val) : null),
> + get childTransactions() this._transactions,
for clarity, should be transactions & _transactions or childTransactions and _childTransactions.
changed to childTransaction, and cleaned up a bit those getters.