Closed Bug 670463 Opened 9 years ago Closed 9 years ago

PlacesAggregatedTransaction arrays prevent chrome windows from being destroyed

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: dao, Assigned: mano)

References

Details

(Keywords: memory-leak, Whiteboard: [MemShrink:P3])

Attachments

(1 file)

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.
Blocks: mlk-fx8
Whiteboard: [MemShrink]
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
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.
Component: Places → Bookmarks & History
Product: Toolkit → Firefox
QA Contact: places → bookmarks
Summary: Places transaction manager prevents chrome windows from being destroyed → Something prevents some library windows from being destroyed
Whiteboard: [MemShrink] → [MemShrink:P3]
Mano is doing an awesome work on this bug, assigning to him.
Assignee: nobody → mano
Status: NEW → ASSIGNED
Morphing back, as Mano identified transactions to be responsible for this (or for a subset of the four tests, at least).
Component: Bookmarks & History → Places
Product: Firefox → Toolkit
QA Contact: bookmarks → places
Summary: Something prevents some library windows from being destroyed → Places transactions prevent chrome windows from being destroyed
Mano, are you still working on this?
Attached patch mano's patchSplinter Review
Fixes three of the four leaks. Still need to identify the test triggering the last leak.
Attachment #548552 - Flags: review?(mak77)
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."
Attachment #548552 - Flags: review?(mak77) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Summary: Places transactions prevent chrome windows from being destroyed → PlacesAggregatedTransaction arrays prevent chrome windows from being destroyed
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.