338645 - Wrap Gecko event processing in an auto release pool

Status: NEW

(Core :: Widget: Cocoa, defect, P3)

Description

[Mark Mentovai]

Right now, we've really only got two autorelease pools working: there's a pool for each native event, and for everything else, there's the root pool.  Too many things are autoreleased to the root pool.  I want to get us to the point where only true application-scope objects that need to live for the life of the application are autoreleased to that pool.

We should at least provide shorter-lived pools for Gecko events.  In the past, Gecko events were handled on native event fetch, with a pool in place, so they wouldn't be autoreleased to the root pool.  The new thread architecture (bug 326273) changed this.

I think we only need to guarantee an autorelease pool for things in widget/src/cocoa that happen on Gecko events on the main thread.  If there's ObjC/Cocoa code that doesn't meet these criteria, it should still provide its own pool, as in bug 338041.

Comment 1

[Stuart Parmenter]

Does this actually fix anything or is it just cleaner code?

Comment 2

[Josh Aas]

It would fix a type of memory leak that I suspect is not uncommon right now.

Comment 3

[Håkan Waara]

We could afford to use autoreleasepools in lots of places in nsCocoaWindow/nsChildView - they are very, very cheap to create (http://www.mikeash.com/blog/pivot/entry.php?id=19) and would help us use the memory more efficiently.

So let's just add pools to some major places where we communicate down to gecko, from nsChildView/nsCocoaWindow. One obvious such place would be for example DispatchEvent().

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:spohl, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 5

[Stephen A Pohl [:spohl]]

Markus, could I ask you to weigh in here on what should happen with this bug?

Comment 6

[Markus Stange [:mstange]]

Interesting bug! It makes sense, but I haven't seen any complaints about too-high temporary memory usage from autoreleased objects, so that's probably why we haven't done it before.
We're also in a very different today where most of the ObjC APIs are only called in the parent process and web content is running in the content process. Though we do use some ObjC APIs in the content process, for example for stuff relating to fonts. It would be interesting to see how the content process event loop manages its auto release pools.

Hmm, I just got an allocation profile and didn't see any deallocations from event loop auto release pools in the content process. We may in fact have some leaks there. This warrants investigation.

Comment 7

[Stephen A Pohl [:spohl]]

Thank you!