base::mac::ScopedNSAutoreleasePool can be used potentially unsafely |
|
Issue descriptionI 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
,
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 |
|
Comment 1 by rsesek@chromium.org
, Oct 30 2017