Bug 186436 – [Cocoa] Remove all uses of NSAutoreleasePool as part of preparation for ARC

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Darin Adler]

[Cocoa] Remove all uses of NSAutoreleasePool as part of preparation for ARC

[Darin Adler]

Created attachment 342292 [details]
Patch

[Anders Carlsson]

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.

[Darin Adler]

OK, I’ll fix the compilation errors and do what you suggested.

[Darin Adler]

(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.

[Darin Adler]

Created attachment 342336 [details]
Patch

[Anders Carlsson]

/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

[Darin Adler]

Got a better version coming. Found another file using autorelease pool.

[Darin Adler]

Created attachment 342342 [details]
Patch

[EWS Watchlist]

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.

[Darin Adler]

Created attachment 342360 [details]
Patch

[EWS Watchlist]

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.

[Darin Adler]

Committed r232660: <https://trac.webkit.org/changeset/232660>

[Radar WebKit Bug Importer]

<rdar://problem/40969584>

[Dawei Fenton (:realdawei)]

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

[Dawei Fenton (:realdawei)]

Created attachment 342443 [details]
Sample Crash Log

[Darin Adler]

OK, fine to roll out if the test is failing. I can fix the problem and re-land later.

[Darin Adler]

Oh, I see why it’s failing. The error is in the test, not WebCore and is really trivial to fix.

[Darin Adler]

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.

[Darin Adler]

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.

[Brady Eidson]

(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

[Dawei Fenton (:realdawei)]

(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!