Bug 198346 - IndexedDatabase Server thread in com.apple.WebKit.Networking process leaks objects into an autoreleasePool that's never cleared
Summary: IndexedDatabase Server thread in com.apple.WebKit.Networking process leaks ob...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-29 12:59 PDT by David Kilzer (:ddkilzer)
Modified: 2019-05-29 15:07 PDT (History)
13 users (show)

See Also:


Attachments
Patch v1 (6.02 KB, patch)
2019-05-29 13:12 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2019-05-29 12:59:49 PDT
IndexedDatabase Server thread in com.apple.WebKit.Networking process leaks objects into an autoreleasePool that's never cleared.

Running a bunch of IndexedDB tests on a single process shows how these objects build up:

$ ./Tools/Scripts/run-webkit-tests --no-build --release --batch-size=1500 --child-processes=1 --leaks --no-retry --no-sample --time-out-ms=60000 --no-show-results LayoutTests/inspector/indexeddb LayoutTests/storage/indexeddb LayoutTests/http/tests/IndexedDB LayoutTests/imported/blink/storage/indexeddb LayoutTests/imported/w3c/IndexedDB-private-browsing LayoutTests/imported/w3c/web-platform-tests/IndexedDB

$ leaks -auto com.apple.WebKit.Networking.Development-58655-UNFIXED.memgraph | less
[…]
Thread 5  Thread_17325843: IndexedDatabase Server
    <@autoreleasepool content 0x7face9815000> [4096]
        0:  <CFString 0x7face8710870> [192]
        1:  <NSPathStore2 0x7face87188f0> [368]
        2:  2 (304 bytes) <NSURL 0x7face871ae10> [96]
               1 (208 bytes) _clients --> <CFString 0x7face87030b0> [208]
        3:  <NSMutableArray 0x7face8717f40> [48]
        4:  <NSArray 0x7face8715ac0> [16]
[…]
        9595:  <NSMutableArray 0x7face865e0f0> [48]
        9596:  <NSArray 0x7face865c9d0> [16]

Thread 6  Thread_17325989: IndexedDatabase Server
    no autorelease pool
[…]
Comment 1 David Kilzer (:ddkilzer) 2019-05-29 13:00:55 PDT
<rdar://problem/50895658>
Comment 2 David Kilzer (:ddkilzer) 2019-05-29 13:12:44 PDT
Created attachment 370877 [details]
Patch v1
Comment 3 EWS Watchlist 2019-05-29 13:14:32 PDT
Attachment 370877 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/CrossThreadTaskHandler.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 David Kilzer (:ddkilzer) 2019-05-29 13:23:58 PDT
(In reply to Build Bot from comment #3)
> Attachment 370877 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WTF/wtf/CrossThreadTaskHandler.cpp:29:  Alphabetical sorting
> problem.  [build/include_order] [4]
> Total errors found: 1 in 5 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Bug 198349: check-webkit-style reports false-positive build/include_order warning in WTF C++ source files
Comment 5 Brent Fulgham 2019-05-29 14:15:00 PDT
Comment on attachment 370877 [details]
Patch v1

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

> Source/WTF/wtf/CrossThreadTaskHandler.h:44
> +    WTF_EXPORT_PRIVATE CrossThreadTaskHandler(const char* threadName, AutodrainedPoolForRunLoop = AutodrainedPoolForRunLoop::DoNotUse);

Since the only use of CrossThreadTaskHandler is IndexedDB, and IndexedDB needs an auto drained pool, should we just make auto drained the default?
Comment 6 David Kilzer (:ddkilzer) 2019-05-29 14:22:19 PDT
Comment on attachment 370877 [details]
Patch v1

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

>> Source/WTF/wtf/CrossThreadTaskHandler.h:44
>> +    WTF_EXPORT_PRIVATE CrossThreadTaskHandler(const char* threadName, AutodrainedPoolForRunLoop = AutodrainedPoolForRunLoop::DoNotUse);
> 
> Since the only use of CrossThreadTaskHandler is IndexedDB, and IndexedDB needs an auto drained pool, should we just make auto drained the default?

The reason I made AutodrainedPoolForRunLoop::DoNotUse the default is that I think it tends to be unusual to run Objective-C code (on background threads) in WebKit, but maybe I'm wrong?

However, I don't have an objection to making AutodrainedPoolForRunLoop::Use the default since CrossThreadTaskHandler is only used by IndexedDB currently.

I do think we should keep the AutodrainedPoolForRunLoop enum class argument, though, since it make using the class in other scenarios easier, and hopefully makes future users ask the question whether they need an AutodrainedPool or not for their task runloop.
Comment 7 Brent Fulgham 2019-05-29 14:40:21 PDT
Comment on attachment 370877 [details]
Patch v1

Looks good! r=me
Comment 8 WebKit Commit Bot 2019-05-29 15:07:30 PDT
Comment on attachment 370877 [details]
Patch v1

Clearing flags on attachment: 370877

Committed r245871: <https://trac.webkit.org/changeset/245871>
Comment 9 WebKit Commit Bot 2019-05-29 15:07:31 PDT
All reviewed patches have been landed.  Closing bug.