Bug 189455 - Streamline JSRetainPtr, fix leaks of JSString and JSGlobalContext
Summary: Streamline JSRetainPtr, fix leaks of JSString and JSGlobalContext
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks: 189644
  Show dependency treegraph
 
Reported: 2018-09-08 17:29 PDT by Darin Adler
Modified: 2018-09-23 15:15 PDT (History)
9 users (show)

See Also:


Attachments
Patch (217.47 KB, patch)
2018-09-08 18:31 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (223.14 KB, patch)
2018-09-08 18:53 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.61 MB, application/zip)
2018-09-08 19:37 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (3.15 MB, application/zip)
2018-09-08 20:33 PDT, EWS Watchlist
no flags Details
Patch (223.27 KB, patch)
2018-09-08 20:36 PDT, Darin Adler
keith_miller: review+
Details | Formatted Diff | Diff
Patch for EWS and possibly landing (242.29 KB, patch)
2018-09-14 20:25 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch for landing and another EWS pass (247.15 KB, patch)
2018-09-15 07:19 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
One more try at fixing Windows, give EWS a chance (247.85 KB, patch)
2018-09-15 08:16 PDT, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2018-09-08 17:29:39 PDT
Streamline JSRetainPtr, fix leaks of JSString and JSGlobalContext
Comment 1 Darin Adler 2018-09-08 18:31:37 PDT Comment hidden (obsolete)
Comment 2 Darin Adler 2018-09-08 18:33:56 PDT
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.
Comment 3 Darin Adler 2018-09-08 18:53:22 PDT Comment hidden (obsolete)
Comment 4 EWS Watchlist 2018-09-08 19:37:34 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2018-09-08 19:37:36 PDT Comment hidden (obsolete)
Comment 6 Darin Adler 2018-09-08 19:37:45 PDT
Oops, looks like I made a mistake that’s causing accessibility tests to fail. Checking on that.
Comment 7 EWS Watchlist 2018-09-08 20:32:59 PDT Comment hidden (obsolete)
Comment 8 EWS Watchlist 2018-09-08 20:33:01 PDT Comment hidden (obsolete)
Comment 9 Darin Adler 2018-09-08 20:36:37 PDT
Created attachment 349279 [details]
Patch
Comment 10 Darin Adler 2018-09-08 20:37:06 PDT
Found it, was returning "ios" instead of "mac" for platform because of a copy and paste error.
Comment 11 Keith Miller 2018-09-10 13:28:30 PDT
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?
Comment 12 Darin Adler 2018-09-12 12:02:07 PDT
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.
Comment 13 Keith Miller 2018-09-12 12:55:30 PDT
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.
Comment 14 Darin Adler 2018-09-14 19:54:28 PDT
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.
Comment 15 Darin Adler 2018-09-14 20:04:31 PDT
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.
Comment 16 Darin Adler 2018-09-14 20:25:12 PDT Comment hidden (obsolete)
Comment 17 Darin Adler 2018-09-15 07:19:36 PDT Comment hidden (obsolete)
Comment 18 Darin Adler 2018-09-15 08:16:30 PDT
Created attachment 349854 [details]
One more try at fixing Windows, give EWS a chance
Comment 19 Darin Adler 2018-09-15 09:17:30 PDT
Committed r236032: <https://trac.webkit.org/changeset/236032>
Comment 20 Radar WebKit Bug Importer 2018-09-15 09:18:27 PDT
<rdar://problem/44489024>
Comment 21 Darin Adler 2018-09-23 15:15:04 PDT
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.