670463 - PlacesAggregatedTransaction arrays prevent chrome windows from being destroyed

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Dão Gottwald [:dao]]

4 of the windows logged in bug 658738 are chrome://browser/content/places/places.xul. The tests opening these windows are:

browser_410196_paste_into_tags.js
browser_416459_cut.js
browser_library_batch_delete.js
browser_library_left_pane_commands.js

I suspect the transaction manager in PlacesUtils.jsm is responsible for this. It looks like each transaction has a reference to a DOM node and the transactions stick around until places-shutdown. I only skimmed this code, though.

Comment 1

[Marco Bonardo [:mak]]

the transactions should not have references to dom nodes, they get just ids to be built ad are simple js objects (no external references). I'll take a look if I can figure out what we may be keeping alive

Comment 2

[Dão Gottwald [:dao]]

Yeah, I read through PlacesUtils.jsm a second time and it seems that a "container" should always be a number. So my second guess would be PlacesUIUtils.jsm.

Comment 3

[Marco Bonardo [:mak]]

Mano is doing an awesome work on this bug, assigning to him.

Comment 4

[Dão Gottwald [:dao]]

Morphing back, as Mano identified transactions to be responsible for this (or for a subset of the four tests, at least).

Comment 5

[Dão Gottwald [:dao]]

Mano, are you still working on this?

Comment 6

[Dão Gottwald [:dao]]

Attachment: mano's patch

Fixes three of the four leaks. Still need to identify the test triggering the last leak.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 548552 [details] [diff] [review]
mano's patch

Review of attachment 548552 [details] [diff] [review]:
-----------------------------------------------------------------

Do we already have a bug filed to make the test harness able to detect regressions in leaked windows? I don't think it's particularly useful to make a specific test for the Library windows, but for sure we need to test this somehow, better if globally. Should I file one?

What's the remaining test leaking?

::: toolkit/components/places/PlacesUtils.jsm
@@ +2263,5 @@
>  
>  function PlacesAggregatedTransaction(aName, aTransactions)
>  {
> +  // Copy the transactions array in order to not keep its global object alive.
> +  this._transactions = Array.slice(aTransactions);

What about "Copy the transactions array to decouple it from its prototype, that may keep alive the global object it was created into."

Comment 8

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/acb5043ab50c


(In reply to comment #7)
> Do we already have a bug filed to make the test harness able to detect
> regressions in leaked windows? I don't think it's particularly useful to
> make a specific test for the Library windows, but for sure we need to test
> this somehow, better if globally. Should I file one?

Bug 633670

> What's the remaining test leaking?

browser_410196_paste_into_tags.js, filed bug 633670 on it.