Bug 173577 – Fix leak of ModuleInformations in BBQPlan constructors.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Fix leak of ModuleInformations in BBQPlan constructors.

[Keith Miller]

Created attachment 313354 [details]
Patch

[Keith Miller]

rdar://problem/31949152

[Keith Miller]

Created attachment 313355 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Keith Miller]

Created attachment 313373 [details]
Patch

[Saam Barati]

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?

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Keith Miller]

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.

[Keith Miller]

Created attachment 313432 [details]
Patch for landing

[Build Bot]

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.

[Build Bot]

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

[Keith Miller]

Committed r218619: <http://trac.webkit.org/changeset/218619>

[Joseph Pecoraro]

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?

[Joseph Pecoraro]

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.

[Keith Miller]

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

[Joseph Pecoraro]

Ahh, I see. Thanks for clarifying. The patch that landed is very different from the "Patch for landing" attachment.