Summary: | Injected bundle for WebKitTestRunner leaks WKTypeRef objects | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
Component: | Tools / Tests | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin, ews-watchlist, joepeck, lforschler, simon.fraser, thorton, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Local Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2018-12-06 16:04:32 PST
Created attachment 356760 [details]
Patch v1
Comment on attachment 356760 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=356760&action=review > Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:125 > + WKRetainPtr<WKDictionaryRef> initializationDictionary(AdoptWK, static_cast<WKDictionaryRef>(result)); Can we make a version of WKBundlePostSynchronousMessage that is more explicit that the result is retained? At least add a comment in WKBundle.h documenting this above WKBundlePostSynchronousMessage. Comment on attachment 356760 [details] Patch v1 Attachment 356760 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10298192 New failing tests: imported/w3c/web-platform-tests/webrtc/simplecall-no-ssrcs.https.html Created attachment 356773 [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.13.6
Comment on attachment 356760 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=356760&action=review >> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:125 >> + WKRetainPtr<WKDictionaryRef> initializationDictionary(AdoptWK, static_cast<WKDictionaryRef>(result)); > > Can we make a version of WKBundlePostSynchronousMessage that is more explicit that the result is retained? At least add a comment in WKBundle.h documenting this above WKBundlePostSynchronousMessage. It's both WKBundlePostSynchronousMessage() and WKBundlePagePostSynchronousMessageForTesting(). Will update headers. Committed r238948: <https://trac.webkit.org/changeset/238948> Comment on attachment 356760 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=356760&action=review >>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:125 >>> + WKRetainPtr<WKDictionaryRef> initializationDictionary(AdoptWK, static_cast<WKDictionaryRef>(result)); >> >> Can we make a version of WKBundlePostSynchronousMessage that is more explicit that the result is retained? At least add a comment in WKBundle.h documenting this above WKBundlePostSynchronousMessage. > > It's both WKBundlePostSynchronousMessage() and WKBundlePagePostSynchronousMessageForTesting(). > > Will update headers. Was changing the parameter name of each function from `returnData[Ref]` to `returnRetainedData[Ref]` too subtle? Comment on attachment 356760 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=356760&action=review > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:135 > + WKRetainPtr<WKBooleanRef> result(AdoptWK, static_cast<WKBooleanRef>(returnData)); > + return WKBooleanGetValue(result.get()); It’s significantly simpler to write this these with adoptWK instead of a local variable. return WKBooleanGetValue(adoptWK(static_cast<WKBooleanRef>(returnData)).get()); Comment on attachment 356760 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=356760&action=review >> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:135 >> + return WKBooleanGetValue(result.get()); > > It’s significantly simpler to write this these with adoptWK instead of a local variable. > > return WKBooleanGetValue(adoptWK(static_cast<WKBooleanRef>(returnData)).get()); Fixed in this follow-up commit: Committed r239050: <https://trac.webkit.org/changeset/239050> |