Bug 173577 - Fix leak of ModuleInformations in BBQPlan constructors.
Summary: Fix leak of ModuleInformations in BBQPlan constructors.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-06-19 20:00 PDT by Keith Miller
Modified: 2017-06-22 21:06 PDT (History)
18 users (show)

See Also:


Attachments
Patch (5.86 KB, patch)
2017-06-19 20:14 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (6.54 KB, patch)
2017-06-19 20:34 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-elcapitan (327.98 KB, application/zip)
2017-06-19 21:51 PDT, Build Bot
no flags Details
Patch (12.86 KB, patch)
2017-06-19 23:05 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-elcapitan (504.70 KB, application/zip)
2017-06-20 00:22 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (919.57 KB, application/zip)
2017-06-20 00:47 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-elcapitan (1.09 MB, application/zip)
2017-06-20 06:13 PDT, Build Bot
no flags Details
Patch for landing (18.83 KB, patch)
2017-06-20 13:42 PDT, Keith Miller
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-elcapitan (1.40 MB, application/zip)
2017-06-20 15:28 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2017-06-19 20:00:54 PDT
Fix leak of ModuleInformations in BBQPlan constructors.
Comment 1 Keith Miller 2017-06-19 20:14:59 PDT
Created attachment 313354 [details]
Patch
Comment 2 Keith Miller 2017-06-19 20:24:00 PDT
rdar://problem/31949152
Comment 3 Keith Miller 2017-06-19 20:34:48 PDT
Created attachment 313355 [details]
Patch
Comment 4 Build Bot 2017-06-19 21:51:14 PDT
Comment on attachment 313355 [details]
Patch

Attachment 313355 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3963906

Number of test failures exceeded the failure limit.
Comment 5 Build Bot 2017-06-19 21:51:16 PDT
Created attachment 313361 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 6 Keith Miller 2017-06-19 23:05:11 PDT
Created attachment 313373 [details]
Patch
Comment 7 Saam Barati 2017-06-19 23:44:49 PDT
Comment on attachment 313373 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        This patch fixes a leak in the BBQPlan constructiors. Previously,

Typo

> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:62
> +    : BBQPlan(vm, adoptRef(*new ModuleInformation(WTFMove(source))), work, WTFMove(task))

I think we should make a create function for ModuleInformation

> Source/WTF/wtf/ThreadSafeRefCounted.h:33
> +#define CHECK_THREAD_SAFE_REF_COUNTED_LIFECYCLE 0

I’m not a huge fan of this checking code. I see that we do it for RefCounter as well. IMO, it requires too many clients to call the relax function. I’m not sure I get why many of them need to call this. Can you elaborate?

> Source/WTF/wtf/ThreadSafeRefCounted.h:90
> +inline void adopted(ThreadSafeRefCountedBase* refCounted)

Who calls this?
Comment 8 Build Bot 2017-06-20 00:22:50 PDT
Comment on attachment 313373 [details]
Patch

Attachment 313373 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3964544

Number of test failures exceeded the failure limit.
Comment 9 Build Bot 2017-06-20 00:22:51 PDT
Created attachment 313376 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 10 Build Bot 2017-06-20 00:47:03 PDT
Comment on attachment 313373 [details]
Patch

Attachment 313373 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3964587

New failing tests:
webrtc/video-replace-muted-track.html
Comment 11 Build Bot 2017-06-20 00:47:05 PDT
Created attachment 313378 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 12 Build Bot 2017-06-20 06:13:24 PDT
Comment on attachment 313373 [details]
Patch

Attachment 313373 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3965778

New failing tests:
inspector/canvas/create-canvas-contexts.html
Comment 13 Build Bot 2017-06-20 06:13:25 PDT
Created attachment 313396 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 14 Keith Miller 2017-06-20 08:39:16 PDT
Comment on attachment 313373 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        This patch fixes a leak in the BBQPlan constructiors. Previously,
> 
> Typo

Fixed.

>> Source/WTF/wtf/ThreadSafeRefCounted.h:33
>> +#define CHECK_THREAD_SAFE_REF_COUNTED_LIFECYCLE 0
> 
> I’m not a huge fan of this checking code. I see that we do it for RefCounter as well. IMO, it requires too many clients to call the relax function. I’m not sure I get why many of them need to call this. Can you elaborate?

Honestly, most of the clients could be refactored to not call the relax function. I just didn't do it here to minimize the code change and avoid introducing bugs. In my experience, very few new clients will need to call relax and memory leaks are definitely a bummer.

>> Source/WTF/wtf/ThreadSafeRefCounted.h:90
>> +inline void adopted(ThreadSafeRefCountedBase* refCounted)
> 
> Who calls this?

The adoptRef functions call an adopted function. There is a default `void adopted(void*);` function. This is more specialized so it gets called for ThreadSafeRefCounted things instead.
Comment 15 Keith Miller 2017-06-20 13:42:39 PDT
Created attachment 313432 [details]
Patch for landing
Comment 16 Build Bot 2017-06-20 15:28:17 PDT
Comment on attachment 313432 [details]
Patch for landing

Attachment 313432 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3967949

Number of test failures exceeded the failure limit.
Comment 17 Build Bot 2017-06-20 15:28:18 PDT
Created attachment 313441 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 18 Keith Miller 2017-06-20 17:32:30 PDT
Committed r218619: <http://trac.webkit.org/changeset/218619>
Comment 19 Joseph Pecoraro 2017-06-22 20:15:08 PDT
Comment on attachment 313432 [details]
Patch for landing

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

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabaseTracker.cpp:45
> -    return adoptRef(*new LocalStorageDatabaseTracker(WTFMove(queue), localStorageDirectory));
> +    return makeRef(*new LocalStorageDatabaseTracker(WTFMove(queue), localStorageDirectory));

Shouldn't this adopt the new?

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabaseTracker.cpp:57
> -    m_queue->dispatch([protectedThis = makeRef(*this)]() mutable {
> +    m_queue->dispatch([protectedThis = adoptRef(*this)]() mutable {

Why is this not adopting? Don't we want +1 ourselves during the running of this block, which makeRef would do?
Comment 20 Joseph Pecoraro 2017-06-22 20:17:22 PDT
Comment on attachment 313432 [details]
Patch for landing

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

>> Source/WebKit2/UIProcess/Storage/LocalStorageDatabaseTracker.cpp:57
>> +    m_queue->dispatch([protectedThis = adoptRef(*this)]() mutable {
> 
> Why is this not adopting? Don't we want +1 ourselves during the running of this block, which makeRef would do?

I meant to ask why I this adopting. I'd have expected it to be a makeRef. We have the `makeRef(*this)` pattern in a LOT of lambdas.
Comment 21 Keith Miller 2017-06-22 20:19:09 PDT
(In reply to Joseph Pecoraro from comment #19)
> Comment on attachment 313432 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313432&action=review
> 
> > Source/WebKit2/UIProcess/Storage/LocalStorageDatabaseTracker.cpp:45
> > -    return adoptRef(*new LocalStorageDatabaseTracker(WTFMove(queue), localStorageDirectory));
> > +    return makeRef(*new LocalStorageDatabaseTracker(WTFMove(queue), localStorageDirectory));
> 
> Shouldn't this adopt the new?
> 
> > Source/WebKit2/UIProcess/Storage/LocalStorageDatabaseTracker.cpp:57
> > -    m_queue->dispatch([protectedThis = makeRef(*this)]() mutable {
> > +    m_queue->dispatch([protectedThis = adoptRef(*this)]() mutable {
> 
> Why is this not adopting? Don't we want +1 ourselves during the running of
> this block, which makeRef would do?

I actually removed most of the debug assertions before landing. I found out that almost all the users of ThreadSafeRefCounted were spawning a thread in their constructor and passing themselves to that thread. This made the assertions mostly just an inconvenience as everyone had to relax them.
Comment 22 Joseph Pecoraro 2017-06-22 21:06:47 PDT
Ahh, I see. Thanks for clarifying. The patch that landed is very different from the "Patch for landing" attachment.