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

[bugzilla-daemon at webkit.org]

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


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #80539|review?                     |review-
               Flag|                            |




--- Comment #1 from Alexey Proskuryakov <ap at webkit.org>  2011-01-28 22:16:15 PST ---
(From update of attachment 80539)
View in context: https://bugs.webkit.org/attachment.cgi?id=80539&action=review

Looks good. I have many style nitpicks though, seems worth another quick review round.

> Source/WebKit2/Platform/RunLoop.h:33
>  #include <wtf/PassOwnPtr.h>

PLease fix alphabetic sorting.

> Source/WebKit2/Platform/RunLoop.h:143
> +    RetainPtr<CFRunLoopObserverRef> m_CurrentRunLoopEnterObserverRef;
> +    RetainPtr<CFRunLoopObserverRef> m_CurrentRunLoopExitObserverRef;

WebKit style is m_currentRunLoopEnter/ExitObserver (lower case first letter, and there is no need for "Ref" suffix.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:30
> +#define WRAP_RUN_LOOP_WITH_AUTORELEASE_POOLS_OBSERVER_ORDER 1000000

We prefer consts to macros.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:36
> +    switch (activity) {
> +        case kCFRunLoopEntry:

We don't indent case statements.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:43
> +                const NSAutoreleasePool* pool = static_cast<const NSAutoreleasePool*>(CFArrayGetValueAtIndex(autoreleasePoolStack, count-1));

Please also use const_cast to remove the const. There should be a space around "-" in count-1.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:45
> +                CFArrayRemoveValueAtIndex(autoreleasePoolStack, count-1);

There should be a space around "-" in count-1.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:118
> +// Set up an Autorelease Pool on the main RunLoop
> +// FIXME: remove when <rdar://problem/8929426> is fixed.
> +void RunLoop::installAutoreleasePoolsOnCurrentRunLoop()

Does this function work on the main, or on current run loop? Please bring comments and function name in sync.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:123
> +        CFRunLoopObserverRef newRunLoopEnterObserverRef = 0;

Please don't use local CF objects without RetainPtr. I think that here, you could just assign to m_currentRunLoopEnterObserver immediately.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:125
> +        CFRunLoopObserverRef newRunLoopExitObserverRef = 0;

Ditto.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:131
> +        newRunLoopEnterObserverContext.info     = CFArrayCreateMutable(kCFAllocatorSystemDefault /* Don't retain the autorelease pool tokens. */,0,0);
> +        newRunLoopEnterObserverContext.retain   = CFRetain;         // retain the array
> +        newRunLoopEnterObserverContext.release  = CFRelease;        // release the array when the context info goes away.
> +        newRunLoopEnterObserverContext.copyDescription = CFCopyDescription;

Please don't align "=" signs or comments. There should be spaces in ",0,0".

> Source/WebKit2/Platform/mac/RunLoopMac.mm:134
> +        // Install a repeating observer to fire on exit of the current run loop with an very late ordering like kWrapRunLoopWithAutoreleasePoolObserverOrder.  CFRunLoop gives lowest priority to those observers close to positive infinity.

We don't put two spaces between sentences.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:138
> +        newRunLoopExitObserverContext.version  = 0;
> +        newRunLoopExitObserverContext.info     = newRunLoopEnterObserverContext.info;
> +        newRunLoopExitObserverContext.retain   = CFRetain;         // retain the array
> +        newRunLoopExitObserverContext.release  = CFRelease;        // release the array when the context info goes away.

Please don't align.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:142
> +        CFRelease((CFMutableArrayRef)newRunLoopEnterObserverContext.info);        // owned by the observer. (only one release is needed since Enter/Exit point to the same instance).

One space before "//". Please fix grammar in the comment, too.

I would have put newRunLoopEnterObserverContext.info into a local RetainPtr variable to avoid the need for explicit CFRelease.

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