Adding a bookmark and opening a new window and closing the old one causes runtime leaks

RESOLVED FIXED in mozilla13

Status

()

Toolkit
Places
--
major
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: smaug, Assigned: mak)

Tracking

(Blocks: 1 bug)

12 Branch
mozilla13
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2][Snappy:P2])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
We leak the browser.xul of the old window.
JS implemented nsITransaction keeps it alive.

This adds the CC time significantly.
(Reporter)

Comment 1

6 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

6 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)
Whiteboard: [MemShrink]
(Reporter)

Comment 3

6 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

6 years ago
Whiteboard: [MemShrink] → [MemShrink][Snappy]
(Assignee)

Comment 4

6 years ago
Created attachment 598266 [details] [diff] [review]
dumb hack
(Assignee)

Comment 5

6 years ago
working on a better patch.
Assignee: nobody → mak77
(Assignee)

Updated

6 years ago
Attachment #598266 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Blocks: 728407
(Assignee)

Comment 6

6 years ago
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.
Attachment #598413 - Flags: feedback?(bugs)
(Reporter)

Comment 7

6 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

6 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

6 years ago
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.
Attachment #598413 - Attachment is obsolete: true
Attachment #598519 - Flags: review?(dietrich)
Attachment #598519 - Flags: feedback?(bugs)
(Reporter)

Comment 10

6 years ago
Comment on attachment 598519 [details] [diff] [review]
patch v1.1

Seems to work now. Thanks!
Attachment #598519 - Flags: feedback?(bugs) → feedback+
Whiteboard: [MemShrink][Snappy] → [MemShrink:P2][Snappy]

Updated

6 years ago
Whiteboard: [MemShrink:P2][Snappy] → [MemShrink:P2][Snappy:P2]
(Assignee)

Comment 11

6 years ago
Created attachment 599606 [details] [diff] [review]
patch v1.2

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 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

6 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

6 years ago
https://hg.mozilla.org/mozilla-central/rev/a9a8c57be783
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Depends on: 767939

Updated

4 years ago
Depends on: 888567
(Assignee)

Updated

3 years ago
Depends on: 974406
You need to log in before you can comment on or make changes to this bug.