Bug 172583 – _WKUserStyleSheet and WKUserScript leak string data

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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)

[Joseph Pecoraro]

Created attachment 311206 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

<rdar://problem/32395209>

[Joseph Pecoraro]

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]):

:(

[Joseph Pecoraro]

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.

[Build Bot]

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

[Build Bot]

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

[Joseph Pecoraro]

Comment on attachment 311210 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

Simulator failures appear to be unrelated.

[WebKit Commit Bot]

Comment on attachment 311207 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 311207

Committed r217409: <http://trac.webkit.org/changeset/217409>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

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

[Matt Lewis]

Reverted r217409 for reason:

The revision caused api failures

Committed r217430: <http://trac.webkit.org/changeset/217430>

[Joseph Pecoraro]

(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

[Joseph Pecoraro]

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

[Joseph Pecoraro]

> RetainPtr<_WKUserContentWorld> world = adoptNS([_WKUserContentWorld worldWithName:@"TestWorld"]);

This seems wrong. We should not be adopting this autoreleased object, we should be regularly retaining it.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 311286 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 311286 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 311286

Committed r217474: <http://trac.webkit.org/changeset/217474>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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