Summary: _WKUserStyleSheet leaks string data Test: #import <Foundation/Foundation.h> #import <WebKit/_WKUserStyleSheet.h> int main() { @autoreleasepool { NSString *str = @"body { color: red; }"; _WKUserStyleSheet *styleSheet = [[_WKUserStyleSheet alloc] initWithSource:[[str copy] autorelease] forMainFrameOnly:NO]; [styleSheet release]; } [[NSRunLoop mainRunLoop] run]; return 0; } Steps to Reproduce: 1. Compil 2. $ MallocStackLogging=1 a.out 3. $ leaks `pidof a.out` => String leaks Leaks: Leak: 0x7f9007100020 size=48 zone: WebKit Using System Malloc_0x10582a000 0x00000002 0x00000014 0x07100034 0x00007f90 ........4....... 0x00000008 0x79646f62 0x63207b20 0x726f6c6f ....body { color 0x6572203a 0x7d203b64 0x00000000 0x00000000 : red; }........ Call stack: [thread 0x7fffb4f73340]: | 0x1 | start | main | -[_WKUserStyleSheet initWithSource:forMainFrameOnly:] | WTF::String::String(NSString*) | WTF::StringImpl::create(unsigned char const*, unsigned int) | WTF::fastMalloc(unsigned long) | bmalloc::DebugHeap::malloc(unsigned long) Leak: 0x7f9007100080 size=64 zone: WebKit Using System Malloc_0x10582a000 0x00000002 0x00000012 0x07100094 0x00007f90 ................ 0x00000008 0x72657375 0x7974732d 0x732d656c ....user-style-s 0x74656568 0x0000313a 0x00000000 0x00000000 heet:1.......... 0x00000000 0x00000000 0x00000000 0x00000000 ................ Call stack: [thread 0x7fffb4f73340]: | 0x1 | start | main | -[_WKUserStyleSheet initWithSource:forMainFrameOnly:] | API::UserStyleSheet::generateUniqueURL() | WTF::StringBuilder::appendNumber(unsigned long long) | WTF::StringBuilder::append(unsigned char const*, unsigned int) | void WTF::StringBuilder::reallocateBuffer<unsigned char>(unsigned int) | WTF::StringImpl::reallocate(WTF::Ref<WTF::StringImpl>&&, unsigned int, unsigned char*&) | WTF::fastRealloc(void*, unsigned long)
Created attachment 311206 [details] [PATCH] Proposed Fix
<rdar://problem/32395209>
Created attachment 311207 [details] [PATCH] Proposed Fix
Comment on attachment 311207 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=311207&action=review > Source/WebKit2/ChangeLog:10 > + (-[_WKUserStyleSheet dealloc]): :(
Comment on attachment 311207 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=311207&action=review >> Source/WebKit2/ChangeLog:10 >> + (-[_WKUserStyleSheet dealloc]): > > :( I don't feel a comment is warranted in these kinds of cases. This follows a pattern and the pattern was missed.
Comment on attachment 311207 [details] [PATCH] Proposed Fix Attachment 311207 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3811488 New failing tests: fast/events/before-unload-returnValue.html webrtc/peer-connection-audio-mute.html
Created attachment 311210 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 311210 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 Simulator failures appear to be unrelated.
Comment on attachment 311207 [details] [PATCH] Proposed Fix Clearing flags on attachment: 311207 Committed r217409: <http://trac.webkit.org/changeset/217409>
All reviewed patches have been landed. Closing bug.
The revision caused 4 API failures in all WK1 and WK2 Testers. https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/1752 https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/1752/steps/run-api-tests/logs/stdio
Reverted r217409 for reason: The revision caused api failures Committed r217430: <http://trac.webkit.org/changeset/217430>
(In reply to Matt Lewis from comment #11) > The revision caused 4 API failures in all WK1 and WK2 Testers. > > https://build.webkit.org/builders/ > Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/1752 > https://build.webkit.org/builders/ > Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/1752/steps/run-api-tests/ > logs/stdio Interesting! Before landing this I had run: $ run-api-tests --release --show-leaks --verbose WKUserContentController.ScriptMessageHandlerBasicPostIsolatedWorld But that was not one of the ones failing: WKUserContentController.NonCanonicalizedURL WKUserContentController.UserStyleSheetRemoveAll WKUserContentController.UserStyleSheetRemoveAllByNormalWorld WKUserContentController.UserStyleSheetRemoveAllByWorld
Hmm, this test fails if I reduce it to: TEST(WKUserContentController, UserStyleSheetRemoveAllByWorld) { RetainPtr<WKUserContentController> userContentController = adoptNS([[WKUserContentController alloc] init]); RetainPtr<_WKUserContentWorld> world = adoptNS([_WKUserContentWorld worldWithName:@"TestWorld"]); } Which has now been reduced to not using _WKUserStyleSheet at all. It also fails without my changes...
> RetainPtr<_WKUserContentWorld> world = adoptNS([_WKUserContentWorld worldWithName:@"TestWorld"]); This seems wrong. We should not be adopting this autoreleased object, we should be regularly retaining it.
Affects WKUserScript as well: @autoreleasepool { NSString *source = @"window.alert(1);"; WKUserScript *userScript = [[WKUserScript alloc] initWithSource:[[source copy] autorelease] injectionTime:WKUserScriptInjectionTimeAtDocumentStart forMainFrameOnly:NO]; [userScript release]; } So lets address that as well.
Created attachment 311286 [details] [PATCH] Proposed Fix
Comment on attachment 311286 [details] [PATCH] Proposed Fix Clearing flags on attachment: 311286 Committed r217474: <http://trac.webkit.org/changeset/217474>
(In reply to Joseph Pecoraro from comment #15) > > RetainPtr<_WKUserContentWorld> world = adoptNS([_WKUserContentWorld worldWithName:@"TestWorld"]); > > This seems wrong. We should not be adopting this autoreleased object, we > should be regularly retaining it. The way you landed seems OK, but there is no need for RetainPtr here. In both non-ARC and ARC code, just a _WKUserContentWorld * would work fine to hold this value.