[Cocoa] Remove all uses of NSAutoreleasePool as part of preparation for ARC
Created attachment 342292 [details] Patch
Comment on attachment 342292 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342292&action=review > Source/WTF/wtf/AutodrainedPool.h:37 > +// This class allows non-Objective-C C++ code to create an autorelease pool. > +// It should not be used in Objective-C++ code; instead @autoreleasepool should be used. > +// It can be used in cross-platform code; will compile down to nothing for non-Cocoa platforms. Can you enforce this by using #error if __OBJC__ is defined? > Source/WTF/wtf/AutodrainedPoolMac.mm:37 > + : m_pool(NSPushAutoreleasePool(0)) { } initialization? > Source/WebKitLegacy/mac/History/WebHistory.mm:601 > + // FIXME: If we can test and see good performance draining the autorelease pool every time through the loop, > + // instead of once every 50 iterations, then we should switch to @autoreleasepool and get rid of the use of > + // FoundationSPI.h in this source file. I'd go a step further and just use @autoreleasepool here - it should be fast enough and given that WebKit Legacy is deprecated I don't think it matters even if it's a little slower. > Source/WebKitLegacy/mac/WebView/WebView.mm:7908 > + // FIXME: If we can test and see good performance draining the autorelease pool every time > + // through the loop, instead of once every 10 iterations, then we should switch to > + // @autoreleasepool and get rid of the use of FoundationSPI.h in this source file. Even more true here, since this is SPI that Safari no longer uses - not sure if anyone else does.
OK, I’ll fix the compilation errors and do what you suggested.
(In reply to Anders Carlsson from comment #2) > Can you enforce this by using #error if __OBJC__ is defined? The trick is to do that while allowing AutodrainedPoolMac.mm to compile. But I will do it.
Created attachment 342336 [details] Patch
/Volumes/Data/EWS/WebKit/Tools/TestWebKitAPI/Tests/mac/StopLoadingFromDidFinishLoading.mm:51:21: error: use of undeclared identifier 'pool'; did you mean 'powl'? AutodrainedPool pool; ^~~~ powl
Got a better version coming. Found another file using autorelease pool.
Created attachment 342342 [details] Patch
Attachment 342342 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:47: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:48: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:49: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:51: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:52: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:53: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:36: objc_autoreleasePoolPush is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:37: objc_autoreleasePoolPop is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 8 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 342360 [details] Patch
Attachment 342360 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:47: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:48: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:49: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:51: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:52: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:53: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:36: objc_autoreleasePoolPush is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:37: objc_autoreleasePoolPop is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 8 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r232660: <https://trac.webkit.org/changeset/232660>
<rdar://problem/40969584>
Looks like we are getting an API test failure after this patch Sample output: Test suite failed Crashed TestWebKitAPI.WebKitLegacy.StopLoadingFromDidFinishLoading https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/builds/4986/steps/run-api-tests/logs/stdio Excerpt from Crash log: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 libobjc.A.dylib 0x00007fff796bad3b objc_retain + 27 1 com.apple.WebKitLegacy 0x000000010b0ea647 WebDocumentLoaderMac::setDataSource(WebDataSource*, WebView*) + 87 (WebDocumentLoaderMac.mm:62) 2 com.apple.WebKitLegacy 0x000000010b100eb7 WebFrameLoaderClient::createDocumentLoader(WebCore::ResourceRequest const&, WebCore::SubstituteData const&) + 167 (WebFrameLoaderClient.mm:1353) 3 com.apple.WebCore 0x000000010c3d99d7 WebCore::FrameLoader::load(WebCore::FrameLoadRequest&&) + 583 (FrameLoader.cpp:1454) 4 com.apple.WebKitLegacy 0x000000010b0faf90 -[WebFrame _loadData:MIMEType:textEncodingName:baseURL:unreachableURL:] + 1248 5 com.apple.WebKitLegacy 0x000000010b0fb439 -[WebFrame _loadHTMLString:baseURL:unreachableURL:] + 89 (WebFrame.mm:2548) 6 TestWebKitAPI 0x00000001084093fb TestWebKitAPI::WebKitLegacy_StopLoadingFromDidFinishLoading_Test::TestBody() + 197 (StopLoadingFromDidFinishLoading.mm:54) 7 TestWebKitAPI 0x00000001084e3958 testing::Test::Run() + 92 8 TestWebKitAPI 0x00000001084e41b4 testing::internal::TestInfoImpl::Run() + 180 9 TestWebKitAPI 0x00000001084e459c testing::TestCase::Run() + 196 10 TestWebKitAPI 0x00000001084e7d76 testing::internal::UnitTestImpl::RunAllTests() + 614 11 TestWebKitAPI 0x0000000108411bee TestWebKitAPI::TestsController::run(int, char**) + 120 (TestsController.cpp:86) 12 TestWebKitAPI 0x00000001084c37fb main + 344 (mainMac.mm:52) 13 libdyld.dylib 0x00007fff7a2df015 start + 1
Created attachment 342443 [details] Sample Crash Log
OK, fine to roll out if the test is failing. I can fix the problem and re-land later.
Oh, I see why it’s failing. The error is in the test, not WebCore and is really trivial to fix.
Feel free to roll out, or to just disable this one test. Either way I can easily quickly fix this once I get to a computer.
Here’s the fix for StopLoadingFromDidFinishLoading.mm; resourceLoadDelegate is not retained so it needs to be this: - webView.get().resourceLoadDelegate = adoptNS([[StopLoadingFromDidFinishLoadingDelegate alloc] init]).get(); + auto resourceLoadDelegate = adoptNS([[StopLoadingFromDidFinishLoadingDelegate alloc] init]); + webView.get().resourceLoadDelegate = resourceLoadDelegate.get(); The spacing isn’t right on the change, but that will fix it.
(In reply to Darin Adler from comment #19) > Here’s the fix for StopLoadingFromDidFinishLoading.mm; resourceLoadDelegate > is not retained so it needs to be this: > > - webView.get().resourceLoadDelegate = > adoptNS([[StopLoadingFromDidFinishLoadingDelegate alloc] init]).get(); > + auto resourceLoadDelegate = > adoptNS([[StopLoadingFromDidFinishLoadingDelegate alloc] init]); > + webView.get().resourceLoadDelegate = resourceLoadDelegate.get(); > > The spacing isn’t right on the change, but that will fix it. Followup landed in https://trac.webkit.org/changeset/232727/webkit
(In reply to Brady Eidson from comment #20) > (In reply to Darin Adler from comment #19) > > Here’s the fix for StopLoadingFromDidFinishLoading.mm; resourceLoadDelegate > > is not retained so it needs to be this: > > > > - webView.get().resourceLoadDelegate = > > adoptNS([[StopLoadingFromDidFinishLoadingDelegate alloc] init]).get(); > > + auto resourceLoadDelegate = > > adoptNS([[StopLoadingFromDidFinishLoadingDelegate alloc] init]); > > + webView.get().resourceLoadDelegate = resourceLoadDelegate.get(); > > > > The spacing isn’t right on the change, but that will fix it. > > Followup landed in https://trac.webkit.org/changeset/232727/webkit great thanks!