Opened 6 years ago

Last modified 21 months ago

#9521 assigned defect

"new identity" leaks memory in eventSuppressor.suppressEventHandling()

Reported by: arma Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Major Keywords: tbb-newnym, interview, tbb-torbutton, ff52-esr, tbb-performance-leaking
Cc: gk Actual Points:
Parent ID: #18047 Points:
Reviewer: Sponsor:

Description

According to skruffy, each tab that's open when you click 'new identity' contributes to more lost memory.

For normal tabs, when you close them, eventually the memory from them garbage collects or otherwise returns to the system. For tabs closed by new identity, it remains lost.

Child Tickets

Change History (27)

comment:1 Changed 6 years ago by arma

This is apparently new in FF 17.0.8 and was not present in 17.0.7.

comment:2 Changed 6 years ago by arma

Or maybe that last comment is wrong.

comment:3 Changed 6 years ago by gk

Cc: g.koppen@… added

comment:4 Changed 6 years ago by mikeperry

Summary: "new identity" leaks memory?"new identity" leaks memory in

According to skruffy, this is due to the call to eventSuppressor.suppressEventHandling() during torbutton_disable_all_js(). Removing it eliminates the leak. Possibly DOM events are required to unref all of the objects in a window?

Oddly, the leaks still happen if JS is fully disabled, at least via NoScript's disabling.

comment:5 Changed 6 years ago by mikeperry

Summary: "new identity" leaks memory in"new identity" leaks memory in evenSuppressor.suppressEventHandling()

comment:6 Changed 6 years ago by mikeperry

The leaked memory is primarily concentrated in the 'heap-unclassified' section of about:memory, and this is reproducible even if you just mash New Identity a bunch of times without loading any web pages.

Probably time to dig a bit deeper and try out some of these: https://wiki.mozilla.org/Performance:Leak_Tools.

comment:7 Changed 6 years ago by mikeperry

Actually, there's quite a few window objects from previous new identity sessions in about:memory too.

comment:8 Changed 6 years ago by arma

Keywords: tbb-newnym added

Still an issue with FF24?

comment:9 Changed 6 years ago by mikeperry

Keywords: interview added

comment:10 Changed 5 years ago by erinn

Component: TorBrowserButtonTor Browser
Keywords: tbb-torbutton added
Owner: changed from mikeperry to tbb-team

comment:11 Changed 5 years ago by cypherpunks

Still an issue with FF24?

Yes, it is still an issue with Firefox. All open pages adds content to never purgeable garbage of Firefox's memory if to choose "new identity". Some of used urls visible as window-objects via about:memory (with verbose report enabled)

comment:12 Changed 5 years ago by cypherpunks

Btw, bug which was reason to use of eventSuppressor.suppressEventHandling() now marked as fixed.

comment:13 Changed 5 years ago by cypherpunks

It's time to remove eventSuppressor.suppressEventHandling() from Torbutton shipped with Tor Browser 4.x and to claim this bug fixed.

comment:14 Changed 5 years ago by cypherpunks

Status: newneeds_information

https://bugzilla.mozilla.org/show_bug.cgi?id=409737 was fixed, Torbutton still using workaround https://gitweb.torproject.org/torbutton.git/tree/src/chrome/content/torbutton.js#n2675
If to remove this code then this ticket could be marked as fixed.

comment:15 Changed 5 years ago by cypherpunks

Owner: changed from tbb-team to mikeperry
Status: needs_informationassigned

comment:16 Changed 5 years ago by cypherpunks

Status: assignedneeds_information

comment:17 Changed 5 years ago by cypherpunks

Component: Tor BrowserTorbutton

comment:18 Changed 5 years ago by cypherpunks

Component: TorbuttonTor Browser

comment:19 Changed 5 years ago by cypherpunks

Summary: "new identity" leaks memory in evenSuppressor.suppressEventHandling()"new identity" leaks memory in eventSuppressor.suppressEventHandling()

comment:20 Changed 5 years ago by mikeperry

Keywords: ff38-esr added

I think we should revisit this in the ff38-esr timeframe. I am very nervous about Mozilla's belief that they completely fixed the underlying problem of being able to disable Javascript on a tab, and I'd like to review that code in-depth. However, it seems a little silly to dig through the ff31-esr code right now only to have some crucial detail change on us in ff38-esr.

Unless of course there is a way to avoid the memory leak by removing the event suppressors in an unload handler or something similar while disposing of the associated tab. That might feel a little safer to me. Maybe. I still think I'd be wary of doing this in the 4.5 series though...

comment:21 Changed 5 years ago by cypherpunks

Status: needs_informationneeds_revision

comment:22 Changed 4 years ago by bugzilla

Keywords: tbb-performance-leaking added
Severity: Major
Status: needs_revisionneeds_information

Another one leftover, not reachable for GC. Some group for mem leaks is needed.

comment:23 Changed 4 years ago by bugzilla

Parent ID: #18047

comment:24 Changed 3 years ago by bugzilla

Keywords: ff45-esr added; ff38-esr removed
Owner: changed from mikeperry to tbb-team
Status: needs_informationassigned

comment:25 Changed 2 years ago by arma

Did this ticket get left behind?

comment:26 Changed 2 years ago by gk

Cc: gk added; g.koppen@… removed

I guess this is still an issue but nobody has looked at it yet.

comment:27 Changed 21 months ago by cypherpunks

Keywords: ff52-esr added; ff45-esr removed
Note: See TracTickets for help on using tickets.