Last Comment Bug 728230 - Adding a bookmark and opening a new window and closing the old one causes runtime leaks
: Adding a bookmark and opening a new window and closing the old one causes run...
Status: RESOLVED FIXED
[MemShrink:P2][Snappy:P2]
:
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: 12 Branch
: x86_64 Linux
: -- major (vote)
: mozilla13
Assigned To: Marco Bonardo [::mak]
:
:
Mentors:
Depends on: 767939 888567 974406
Blocks: leakychrome
  Show dependency treegraph
 
Reported: 2012-02-17 08:04 PST by Olli Pettay [:smaug] (reviewing overload)
Modified: 2014-02-28 02:00 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
dumb hack (14.45 KB, patch)
2012-02-17 09:49 PST, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.0 (57.28 KB, patch)
2012-02-17 16:01 PST, Marco Bonardo [::mak]
bugs: feedback-
Details | Diff | Splinter Review
patch v1.1 (60.28 KB, patch)
2012-02-18 04:31 PST, Marco Bonardo [::mak]
bugs: feedback+
Details | Diff | Splinter Review
patch v1.2 (60.76 KB, patch)
2012-02-22 07:37 PST, Marco Bonardo [::mak]
dietrich: review+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] (reviewing overload) 2012-02-17 08:04:10 PST
We leak the browser.xul of the old window.
JS implemented nsITransaction keeps it alive.

This adds the CC time significantly.
Comment 1 Olli Pettay [:smaug] (reviewing overload) 2012-02-17 08:05:56 PST
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.
Comment 2 Olli Pettay [:smaug] (reviewing overload) 2012-02-17 08:11:26 PST
(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)
Comment 3 Olli Pettay [:smaug] (reviewing overload) 2012-02-17 09:11:25 PST
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
Comment 4 Marco Bonardo [::mak] 2012-02-17 09:49:36 PST
Created attachment 598266 [details] [diff] [review]
dumb hack
Comment 5 Marco Bonardo [::mak] 2012-02-17 13:08:18 PST
working on a better patch.
Comment 6 Marco Bonardo [::mak] 2012-02-17 16:01:23 PST
Created attachment 598413 [details] [diff] [review]
patch v1.0

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 7 Olli Pettay [:smaug] (reviewing overload) 2012-02-17 16:16:06 PST
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
Comment 8 Marco Bonardo [::mak] 2012-02-18 01:27:54 PST
I have found the remaining problem, it's the annotation object we create in browser-places, so I'll also have to copy those :(
Comment 9 Marco Bonardo [::mak] 2012-02-18 04:31:50 PST
Created attachment 598519 [details] [diff] [review]
patch v1.1

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 10 Olli Pettay [:smaug] (reviewing overload) 2012-02-18 04:40:54 PST
Comment on attachment 598519 [details] [diff] [review]
patch v1.1

Seems to work now. Thanks!
Comment 11 Marco Bonardo [::mak] 2012-02-22 07:37:15 PST
Created attachment 599606 [details] [diff] [review]
patch v1.2

a test was correctly failing, I identified an unwanted behavior change and this restores it.
Comment 12 Dietrich Ayala (:dietrich) 2012-02-23 16:57:52 PST
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.
Comment 13 Marco Bonardo [::mak] 2012-02-24 02:29:29 PST
changed to childTransaction, and cleaned up a bit those getters.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9a8c57be783
Comment 14 Marco Bonardo [::mak] 2012-02-24 09:27:06 PST
https://hg.mozilla.org/mozilla-central/rev/a9a8c57be783

Note You need to log in before you can comment on or make changes to this bug.