New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 772489 link

Starred by 11 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

base::mac::ScopedNSAutoreleasePool can be used potentially unsafely

Project Member Reported by rsesek@chromium.org, Oct 6 2017

Issue description

I have occasionally, but not in a way that is reproducible, seen messages printed in Chrome like: "objc[66484]: Invalid or prematurely-freed autorelease pool 0x100804048. Set a breakpoint on objc_autoreleasePoolInvalid to debug. Proceeding anyway because the app is old (SDK version 10.10). Memory errors are likely." Now that we link against the 10.12 SDK ( issue 669240 ), those errors will become fatal:

  * frame #0: 0x00007fff9cb600aa libsystem_kernel.dylib`__abort_with_payload + 10
    frame #1: 0x00007fff9cb5c326 libsystem_kernel.dylib`abort_with_payload_wrapper_internal + 90
    frame #2: 0x00007fff9cb5c2cc libsystem_kernel.dylib`abort_with_reason + 22
    frame #3: 0x00007fff9c152e1b libobjc.A.dylib`_objc_fatalv(unsigned long long, unsigned long long, char const*, __va_list_tag*) + 102
    frame #4: 0x00007fff9c152cc6 libobjc.A.dylib`_objc_fatal(char const*, ...) + 146
    frame #5: 0x00007fff9c146457 libobjc.A.dylib`(anonymous namespace)::AutoreleasePoolPage::pop(void*) + 1071
    frame #6: 0x00007fff86e27b86 CoreFoundation`_CFAutoreleasePoolPop + 22
    frame #7: 0x00007fff8888cb57 Foundation`-[NSAutoreleasePool drain] + 146
    frame #8: 0x0000000100001c1f autorelease-pool`ScopedPool::~ScopedPool(this=0x0000000100506250) + 31 at autorelease-pool.mm:9
    frame #9: 0x0000000100001815 autorelease-pool`ScopedPool::~ScopedPool(this=0x0000000100506250) + 21 at autorelease-pool.mm:8
    frame #10: 0x0000000100001ace autorelease-pool`Test4() + 190 at autorelease-pool.mm:54
    frame #11: 0x0000000100001ba4 autorelease-pool`main + 20 at autorelease-pool.mm:58
    frame #12: 0x00007fff9ca32235 libdyld.dylib`start + 1

I think these errors are likely related to use of base::mac::ScopedNSAutoreleasePool. The NSAutoreleasePool documentation states: "You should always drain an autorelease pool in the same context (invocation of a method or function, or body of a loop) that it was created" [1]. But because ScopedNSAutoreleasePool can be heap allocated, it can be moved around and stored in other heap objects, leading to usage against the documentation. And I can find a few places in which we do heap-allocate a ScopedNSAutoreleasePool and pass its pointer around [2][3].

In my testing, the error message is printed when an autorelease pool is drained out-of-order. When a pool is drained, the internal linked-list stack of AutoreleasePoolPage is pop()'d [4], which then does a check to ensure that the page's parent is valid. If they are drained out of order, then it calls badPop() to print the error. I've attached a test program that demonstrates different ways of triggering this.

Given that the documentation also states: "@autoreleasepool blocks are more efficient than using an instance of NSAutoreleasePool directly; you can also use them even if you do not use ARC" [1], it seems like we should move to that.

The drawbacks of dropping ScopedNSAutoreleasePool are:
1) Not usable in plain .cc files
2) Requires another level of indention when used

But if it's faster and avoids unsafe usage, it may still be better.


[1] https://developer.apple.com/documentation/foundation/nsautoreleasepool
[2] https://chromium.googlesource.com/chromium/src/+/077661a48a5e9d3bf7126947603efd45ffc54e34/services/service_manager/embedder/main.cc#398
[3] https://chromium.googlesource.com/chromium/src/+/82959bdd37f937c21bf6a1b053c7e84f3ed35de5/content/renderer/renderer_main.cc#104
[4] https://opensource.apple.com/source/objc4/objc4-723/runtime/NSObject.mm.auto.html
 
autorelease-pool.mm
991 bytes Download

Comment 1 by rsesek@chromium.org, Oct 30 2017

Cc: a...@chromium.org rsesek@chromium.org
 Issue 458596  has been merged into this issue.

Comment 2 by a...@chromium.org, Feb 15 2018

We implemented this the way we did because we didn't have @autoreleasepool at the time. Given the ability to misuse ScopedNSAutoreleasePool, I'd be in favor of deprecating it.

Sign in to add a comment