[Webkit-unassigned] [Bug 53361] autorelease pools accumulate memory during automatic testing on Webkit2

Wed Feb 2 15:12:41 PST 2011

https://bugs.webkit.org/show_bug.cgi?id=53361

--- Comment #18 from Geoffrey Garen <ggaren at apple.com>  2011-02-02 15:12:41 PST ---
> It's strange and roundabout, but it helped us detect real problems, see bug 40655. Do you have a better suggestion?

Yes. Put a real ASSERT in a reasonable choke point. For bugs like 40655 (messaging a delegate on a non-main thread), a reasonable choke point is any place in WebKit1 matching the regexp .*Client::dispatch.*.

Such an ASSERT already seems to exist for WebKit2:

    // We only allow sending sync messages from the client run loop.
    ASSERT(RunLoop::current() == m_clientRunLoop);

> We shouldn't be adding autorelease pools to threads that should never run any Objective C.

There is no such thing as a thread that should never run any Objective-C. But if you believe there is such a thing, you can try adding an ASSERT to WTF::String::operator NSString*, and/or HardAutorelease. For what it's worth, that ASSERT would have fired in the case of bug 40655.

Personally, I think it is irrational to assume that just because an accident helped identify one bug, we should keep committing the same accident. That seems like a recipe for paralysis in software development.

> But I'm not sure why we are discussing worker, database and icon database threads. These aren't using WebKit2 RunLoop code, as far as I know. What is using it, besides the vaporware threaded mode?

Threaded mode is not vaporware. It exists today, and Anders asked Stephanie to keep it working.

> Should this class be renamed to not look more general than it is, or should we be calling -_installAutoreleasePoolsOnCurrentThreadIfNecessary in some other code?

If you want to rename RunLoop, please talk to Anders and Sam. Personally, I don't think it's a good idea to name a class after its current set of clients -- then you have to rename it every time it acquires a new client! Anyway, this is pretty far afield from the question at hand, which is, does this patch solve the problem of gigabytes of memory leaking in Safari? I think it does, so I'm inclined to say r+, with the caveat of the WebNSApplicationDetails rename you suggested.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.