Closed
Bug 728230
Opened 12 years ago
Closed 12 years ago
Adding a bookmark and opening a new window and closing the old one causes runtime leaks
Categories
(Toolkit :: Places, defect)
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: smaug, Assigned: mak)
References
(Blocks 1 open bug)
Details
(Whiteboard: [MemShrink:P2][Snappy:P2])
Attachments
(1 file, 3 obsolete files)
60.76 KB,
patch
|
dietrich
:
review+
|
Details | Diff | Splinter Review |
We leak the browser.xul of the old window. JS implemented nsITransaction keeps it alive. This adds the CC time significantly.
Reporter | ||
Comment 1•12 years ago
|
||
STR: start FF load a page which hasn't been bookmarked. click the star twice, click done. open a new window close the old window.
Reporter | ||
Comment 2•12 years ago
|
||
(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)
Updated•12 years ago
|
Whiteboard: [MemShrink]
Reporter | ||
Comment 3•12 years ago
|
||
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
Updated•12 years ago
|
Whiteboard: [MemShrink] → [MemShrink][Snappy]
Assignee | ||
Comment 4•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #598266 -
Attachment is obsolete: true
Reporter | ||
Updated•12 years ago
|
Blocks: leakychrome
Assignee | ||
Comment 6•12 years ago
|
||
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.
Attachment #598413 -
Flags: feedback?(bugs)
Reporter | ||
Comment 7•12 years ago
|
||
Comment on attachment 598413 [details] [diff] [review] patch v1.0 I can't see any change. The document is still there and kept alive by nsITransaction
Attachment #598413 -
Flags: feedback?(bugs) → feedback-
Assignee | ||
Comment 8•12 years ago
|
||
I have found the remaining problem, it's the annotation object we create in browser-places, so I'll also have to copy those :(
Assignee | ||
Comment 9•12 years ago
|
||
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.
Attachment #598413 -
Attachment is obsolete: true
Attachment #598519 -
Flags: review?(dietrich)
Attachment #598519 -
Flags: feedback?(bugs)
Reporter | ||
Comment 10•12 years ago
|
||
Comment on attachment 598519 [details] [diff] [review] patch v1.1 Seems to work now. Thanks!
Attachment #598519 -
Flags: feedback?(bugs) → feedback+
Updated•12 years ago
|
Whiteboard: [MemShrink][Snappy] → [MemShrink:P2][Snappy]
Updated•12 years ago
|
Whiteboard: [MemShrink:P2][Snappy] → [MemShrink:P2][Snappy:P2]
Assignee | ||
Comment 11•12 years ago
|
||
a test was correctly failing, I identified an unwanted behavior change and this restores it.
Attachment #598519 -
Attachment is obsolete: true
Attachment #599606 -
Flags: review?(dietrich)
Attachment #598519 -
Flags: review?(dietrich)
Comment 12•12 years ago
|
||
Comment on attachment 599606 [details] [diff] [review] patch v1.2 Review of attachment 599606 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/PlacesUtils.jsm @@ +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.
Attachment #599606 -
Flags: review?(dietrich) → review+
Assignee | ||
Comment 13•12 years ago
|
||
changed to childTransaction, and cleaned up a bit those getters. https://hg.mozilla.org/integration/mozilla-inbound/rev/a9a8c57be783
Target Milestone: --- → mozilla13
Assignee | ||
Comment 14•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a9a8c57be783
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•