Bug 172558 - REGRESSION (r216977): 6 leaks introduced in new WebKit2_WKHTTPCookieStoreWithoutProcessPool_Test
Summary: REGRESSION (r216977): 6 leaks introduced in new WebKit2_WKHTTPCookieStoreWith...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on: 171987 172607
Blocks:
  Show dependency treegraph
 
Reported: 2017-05-24 15:58 PDT by David Kilzer (:ddkilzer)
Modified: 2017-05-27 11:07 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.22 KB, patch)
2017-05-24 15:58 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (3.36 KB, patch)
2017-05-24 19:54 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (6.48 KB, patch)
2017-05-27 08:43 PDT, David Kilzer (:ddkilzer)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (20.43 MB, application/zip)
2017-05-27 11:07 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2017-05-24 15:58:03 PDT
Need the bug URL (OOPS!).

Reviewed by NOBODY (OOPS!).

* TestWebKitAPI/Tests/WebKit2Cocoa/WKHTTPCookieStore.mm:
(WebKit2_WKHTTPCookieStoreWithoutProcessPool_Test): Fix the leaks.
---
 2 files changed, 18 insertions(+), 6 deletions(-)
Comment 1 David Kilzer (:ddkilzer) 2017-05-24 15:58:04 PDT
Created attachment 311164 [details]
Patch
Comment 2 WebKit Commit Bot 2017-05-24 16:02:23 PDT
Comment on attachment 311164 [details]
Patch

Rejecting attachment 311164 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ed 'websiteDataStore' in 'WTF::RetainPtr<WKWebViewConfiguration>'
            configuration.websiteDataStore = defaultStore;
            ~~~~~~~~~~~~~ ^
2 errors generated.

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/TestWebKitAPI.build/Release/TestWebKitAPILibrary.build/Objects-normal/x86_64/WKHTTPCookieStore.o Tests/WebKit2Cocoa/WKHTTPCookieStore.mm normal x86_64 objective-c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/3809236
Comment 3 David Kilzer (:ddkilzer) 2017-05-24 19:54:29 PDT
Created attachment 311185 [details]
Patch v2
Comment 4 Sam Weinig 2017-05-25 08:40:20 PDT
Comment on attachment 311185 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=311185&action=review

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKHTTPCookieStore.mm:196
> +            RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]);

auto?

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKHTTPCookieStore.mm:199
> +            RetainPtr<CookieUIDelegate> delegate = adoptNS([[CookieUIDelegate alloc] init]);

auto?

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKHTTPCookieStore.mm:215
> +            RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]);

auto?

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKHTTPCookieStore.mm:218
> +            RetainPtr<CookieUIDelegate> delegate = adoptNS([[CookieUIDelegate alloc] init]);

auto?
Comment 5 David Kilzer (:ddkilzer) 2017-05-25 08:54:50 PDT
Committed r217423: <http://trac.webkit.org/changeset/217423>
Comment 6 David Kilzer (:ddkilzer) 2017-05-25 08:58:16 PDT
Fix typo.

Committed r217424: <http://trac.webkit.org/changeset/217424>
Comment 7 WebKit Commit Bot 2017-05-25 13:55:08 PDT
Re-opened since this is blocked by bug 172607
Comment 9 David Kilzer (:ddkilzer) 2017-05-26 10:28:05 PDT
Alex, do you know why this would have caused a timeout?
Comment 10 David Kilzer (:ddkilzer) 2017-05-27 02:54:55 PDT
(In reply to David Kilzer (:ddkilzer) from comment #9)
> Alex, do you know why this would have caused a timeout?

Ha!  I think the problem may be that WKWebView.UIDelegate is a weak pointer and that CookieUIDelegate no longer leaks (and is deallocated at the end of its block), such that the UIDelegate callback never happens and the test times out.

We probably just need a place to assign the CookieUIDelegate so that it's not marked as a leak and not released before the end of the test.
Comment 11 David Kilzer (:ddkilzer) 2017-05-27 07:05:49 PDT
I didn't notice this before, but WebKit2.WKHTTPCookieStoreWithoutProcessPool also leaks two WKWebView objects.
Comment 12 David Kilzer (:ddkilzer) 2017-05-27 08:43:54 PDT
Created attachment 311418 [details]
Patch v2
Comment 13 David Kilzer (:ddkilzer) 2017-05-27 08:44:57 PDT
Comment on attachment 311418 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=311418&action=review

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKHTTPCookieStore.mm:209
> +    delegate = nil;
> +    // FIXME: Setting webView = nil here causes persistent test to fail on macOS.

Note the FIXME here.  I don't understand why setting webView to nil here causes the next test to fail.  Seems like a real bug.
Comment 14 Build Bot 2017-05-27 11:07:30 PDT
Comment on attachment 311418 [details]
Patch v2

Attachment 311418 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3828019

New failing tests:
http/tests/cache/cancel-during-revalidation-succeeded.html
Comment 15 Build Bot 2017-05-27 11:07:32 PDT
Created attachment 311420 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5