Bug 189455 – Streamline JSRetainPtr, fix leaks of JSString and JSGlobalContext

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Darin Adler]

Streamline JSRetainPtr, fix leaks of JSString and JSGlobalContext

[Darin Adler]

Created attachment 349270 [details]
Patch

[Darin Adler]

I stumbled on these leaks because of a comment Sam made in another patch I was working on. I ended up auditing all the code paths that use the JavaScript C API to allocate a JSStringRef or JSGlobalContextRef.

Most of the leaks were in test-specific code, but not all of them were.

The fixes to use JSRetainPtr more consistently make many of the leaks go away in a subtle way; code that looks the same now doesn't leak. Other leaks were fixed simply by adding release function calls or adding a missing adopt call.

[Darin Adler]

Created attachment 349271 [details]
Patch

[EWS Watchlist]

Comment on attachment 349271 [details]
Patch

Attachment 349271 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9146348

New failing tests:
accessibility/roles-computedRoleString.html
accessibility/attachment-element.html
accessibility/label-with-pseudo-elements.html
accessibility/aria-controls.html
accessibility/img-fallsback-to-title.html
accessibility/math-text.html
accessibility/roles-exposed.html
accessibility/w3c-svg-name-calculation.html
accessibility/w3c-svg-content-language-attribute.html
accessibility/platform-name.html
accessibility/w3c-svg-description-calculation.html
accessibility/mac/ruby-hierarchy-roles.html

[EWS Watchlist]

Created attachment 349273 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Darin Adler]

Oops, looks like I made a mistake that’s causing accessibility tests to fail. Checking on that.

[EWS Watchlist]

Comment on attachment 349271 [details]
Patch

Attachment 349271 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9146502

New failing tests:
accessibility/roles-computedRoleString.html
accessibility/attachment-element.html
accessibility/label-with-pseudo-elements.html
accessibility/aria-controls.html
accessibility/img-fallsback-to-title.html
accessibility/math-text.html
accessibility/roles-exposed.html
accessibility/w3c-svg-name-calculation.html
accessibility/w3c-svg-content-language-attribute.html
accessibility/platform-name.html
accessibility/w3c-svg-description-calculation.html
accessibility/mac/ruby-hierarchy-roles.html

[EWS Watchlist]

Created attachment 349278 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Darin Adler]

Created attachment 349279 [details]
Patch

[Darin Adler]

Found it, was returning "ios" instead of "mac" for platform because of a copy and paste error.

[Keith Miller]

Comment on attachment 349279 [details]
Patch

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

r=me with comments. I think you need to rebase though.

> Source/JavaScriptCore/API/JSObjectRef.cpp:796
> +        propertyNames->array.uncheckedAppend(adopt(OpaqueJSString::create(array[i].string()).leakRef()));

What's the point of adopting the string then leaking it?

> Source/JavaScriptCore/API/JSRetainPtr.h:69
> +    // FIXME: Make this private once Apple's internal code is updated to not rely on it.

nit: Could you file a bugzilla for this and link to it in the FIXME?

> Source/JavaScriptCore/API/JSValue.mm:159
> +    JSStringRef string = JSStringCreateWithCFString((__bridge CFStringRef)description);
> +    auto value = [JSValue valueWithJSValueRef:JSValueMakeSymbol([context JSGlobalContextRef], string) inContext:context];
> +    JSStringRelease(string);

Aww man, I'm so used to RAII lifetime management that I completely missed this! :( Good catch.

Maybe we should use RetainPtr in this file so we don't make these mistakes again?

[Darin Adler]

Comment on attachment 349279 [details]
Patch

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

>> Source/JavaScriptCore/API/JSObjectRef.cpp:796
>> +        propertyNames->array.uncheckedAppend(adopt(OpaqueJSString::create(array[i].string()).leakRef()));
> 
> What's the point of adopting the string then leaking it?

This first leaks the string and then adopts it; I believe this is done to change the type from OpaqueJSString to JSStringRef, but we could also experiment with not doing it to see what kind of compiler error we get.

>> Source/JavaScriptCore/API/JSRetainPtr.h:69
>> +    // FIXME: Make this private once Apple's internal code is updated to not rely on it.
> 
> nit: Could you file a bugzilla for this and link to it in the FIXME?

Sure, I would be willing to if that’s definitely how you like to work, but is that really a good way to do it? I don’t think this is super-important or urgent. But I wrote the FIXME so people understand the intent is that it’s private.

>> Source/JavaScriptCore/API/JSValue.mm:159
>> +    JSStringRelease(string);
> 
> Aww man, I'm so used to RAII lifetime management that I completely missed this! :( Good catch.
> 
> Maybe we should use RetainPtr in this file so we don't make these mistakes again?

Sure we could do that here. I tried to fix the bug with minimal style changes in this patch but I’d be happy to follow up and use JSRetainPtr a bit more.

[Keith Miller]

Comment on attachment 349279 [details]
Patch

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

>>> Source/JavaScriptCore/API/JSRetainPtr.h:69
>>> +    // FIXME: Make this private once Apple's internal code is updated to not rely on it.
>> 
>> nit: Could you file a bugzilla for this and link to it in the FIXME?
> 
> Sure, I would be willing to if that’s definitely how you like to work, but is that really a good way to do it? I don’t think this is super-important or urgent. But I wrote the FIXME so people understand the intent is that it’s private.

The main reason why I like having a bugzilla is that it helps locate all the different places related to the issue. It's probably not super important for this case but I think it's good hygiene.

[Darin Adler]

Comment on attachment 349279 [details]
Patch

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

>>> Source/JavaScriptCore/API/JSObjectRef.cpp:796
>>> +        propertyNames->array.uncheckedAppend(adopt(OpaqueJSString::create(array[i].string()).leakRef()));
>> 
>> What's the point of adopting the string then leaking it?
> 
> This first leaks the string and then adopts it; I believe this is done to change the type from OpaqueJSString to JSStringRef, but we could also experiment with not doing it to see what kind of compiler error we get.

Turns out this has to do leakRef/adopt to turn a RefPtr into a JSRetainPtr. Code inside JavaScriptCore and perhaps inside all of WebKit can just include OpaqueJSString.h and use Ref and RefPtr for JSStringRef, which should be more efficient than using JSRetainPtr.

Code outside WebKit would of course need to use something more like JSRetainPtr. It’s unclear to me if we should keep JSRetainPtr around inside the WebKit project.

[Darin Adler]

Comment on attachment 349279 [details]
Patch

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

>>>> Source/JavaScriptCore/API/JSObjectRef.cpp:796
>>>> +        propertyNames->array.uncheckedAppend(adopt(OpaqueJSString::create(array[i].string()).leakRef()));
>>> 
>>> What's the point of adopting the string then leaking it?
>> 
>> This first leaks the string and then adopts it; I believe this is done to change the type from OpaqueJSString to JSStringRef, but we could also experiment with not doing it to see what kind of compiler error we get.
> 
> Turns out this has to do leakRef/adopt to turn a RefPtr into a JSRetainPtr. Code inside JavaScriptCore and perhaps inside all of WebKit can just include OpaqueJSString.h and use Ref and RefPtr for JSStringRef, which should be more efficient than using JSRetainPtr.
> 
> Code outside WebKit would of course need to use something more like JSRetainPtr. It’s unclear to me if we should keep JSRetainPtr around inside the WebKit project.

I found a great solution. Turns out this is the *only* use of JSRetainPtr inside WebKit. All the other uses are either in test code (DumpRenderTree and WebKitTestRunner) or in Apple internal software. SO I changed this class to not use JSRetainPtr at all.

I suppose that now we should consider how to prevent anyone from accidentally using JSRetainPtr inside WebKit, basically pointing them towards RefPtr and Ref instead.

[Darin Adler]

Created attachment 349849 [details]
Patch for EWS and possibly landing

[Darin Adler]

Created attachment 349853 [details]
Patch for landing and another EWS pass

[Darin Adler]

Created attachment 349854 [details]
One more try at fixing Windows, give EWS a chance

[Darin Adler]

Committed r236032: <https://trac.webkit.org/changeset/236032>

[Radar WebKit Bug Importer]

<rdar://problem/44489024>

[Darin Adler]

Looks like I missed the JavaScriptCore/API/glib directory when making this change. The code in that directory still uses the adopt constructor, not the adopt function.