Bug 158369 – Remove RefPtr::release() and change calls sites to use WTFMove()

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Keith Rollin]

After Bug 157596 is closed, RefPtr::release() will just be releasing it's own reference to the managed object and using it to create a new RefPtr to manage it. This is a waste. Instead of release, call sites should use WTFMove().

[Keith Rollin]

Created attachment 281401 [details]
Patch

[WebKit Commit Bot]

Attachment 281401 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:193:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/svg/SVGFESpecularLightingElement.cpp:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 208 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Does not build on GTK:
../../Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1840:20: error: request for member 'leakRef' in 'newPage', which is of pointer type 'WebKit::WebPageProxy*' (maybe you meant to use '->' ?)
../../Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1841:1: warning: control reaches end of non-void function [-Wreturn-type]
ICECC[19432] 22:55:27: Compiled on 192.168.10.38
ninja: build stopped: subcommand failed.

[Chris Dumez]

Comment on attachment 281401 [details]
Patch

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

> Source/WTF/ChangeLog:8
> +        Add a new RefPtr constructor that takes a Ref&. This eases some

Why doesn't this patch drop release() from RefPtr.h ?

> Source/WTF/wtf/PassRefPtr.h:58
> +        template<typename U> PassRefPtr(const Ref<U>&);

I don't think we need this, we should really WTFMove() at call sites to avoid ref-counting churn.

> Source/WebCore/Modules/webdatabase/Database.cpp:249
> +        PassRefPtr<ScriptExecutionContext> passedContext = WTFMove(m_scriptExecutionContext);

RefPtr<>. I would write this like so:
Auto& context = *m_scriptExecutionContext;
context.postTask({ScriptExecutionContext::Task::CleanupTask, [contextToRelease = WTFMove(m_scriptExecutionContext)] (ScriptExecutionContext& context) {
    ASSERT_UNUSED(context, &context == contextToRelease);
}

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:681
> +        removeNode(ancestor);

WTFMove(ancestor)

> Source/WebCore/inspector/InspectorCSSAgent.cpp:634
>      RefPtr<Inspector::Protocol::CSS::CSSStyle> attributes = buildObjectForAttributesStyle(element);

Could if inside the if() condition:
If (auto attributes = buildObjectForAttributesStyle(element))
  ...

> Source/WebKit2/Shared/API/c/mac/WKWebArchive.cpp:64
> +    auto mainResource = toImpl(webArchiveRef)->mainResource();

Looks like a behavior change. We used to return a ref'd mainResource but not anymore :(

> Source/WebKit2/Shared/API/c/mac/WKWebArchive.cpp:70
> +    auto subresources = toImpl(webArchiveRef)->subresources();

Looks like a behavior change. We used to return a ref'd subresources but not anymore :(

> Source/WebKit2/Shared/API/c/mac/WKWebArchive.cpp:76
> +    auto subframeArchives = toImpl(webArchiveRef)->subframeArchives();

Looks like a behavior change. We used to return a ref'd subframeArchives but not anymore :(

> Source/WebKit2/UIProcess/WebPreferences.cpp:79
>  PassRefPtr<WebPreferences> WebPreferences::copy() const

We should really return a Ref<>, it looks really ugly otherwise.

> Source/WebKit2/WebProcess/Plugins/PDF/DeprecatedPDFPlugin.mm:670
> +    return widget;

WTFMove(widget) ?

> Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:119
> +    return frame;

WTFMove()

> Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:135
> +    return frame;

WTFMove()

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1880
> +        return snapshot;

WTFMove()

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1912
> +    return snapshot;

WTFMove()

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1965
> +    return snapshot;

WTFMove()

> Tools/DumpRenderTree/mac/PixelDumpSupportMac.mm:197
> +    return bitmapContext;

WTFMove()

> Tools/DumpRenderTree/mac/PixelDumpSupportMac.mm:215
> +    return bitmapContext;

WTFMove()

[Keith Rollin]

(In reply to comment #4)
> Comment on attachment 281401 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281401&action=review
> 
> > Source/WTF/ChangeLog:8
> > +        Add a new RefPtr constructor that takes a Ref&. This eases some
> 
> Why doesn't this patch drop release() from RefPtr.h ?

The changes in this patch were determined by my removing release() and then fixing all the build failures. I'm not configured to build for other platforms, so can't safely update their platform-specific code. Until that work can be achieved, release() needs to go back into RefPtr.h.

> > Source/WTF/wtf/PassRefPtr.h:58
> > +        template<typename U> PassRefPtr(const Ref<U>&);
> 
> I don't think we need this, we should really WTFMove() at call sites to
> avoid ref-counting churn.

I added that c'tor to satisfy some need. But I've now removed it and it no longer seems needed.

> > Source/WebCore/Modules/webdatabase/Database.cpp:249
> > +        PassRefPtr<ScriptExecutionContext> passedContext = WTFMove(m_scriptExecutionContext);
> 
> RefPtr<>. I would write this like so:
> Auto& context = *m_scriptExecutionContext;
> context.postTask({ScriptExecutionContext::Task::CleanupTask,
> [contextToRelease = WTFMove(m_scriptExecutionContext)]
> (ScriptExecutionContext& context) {
>     ASSERT_UNUSED(context, &context == contextToRelease);
> }

I agree that this code needs cleaning up. But it's tricky enough that I think it should be part of another patch. That way it can be rolled out by itself if the change causes problems.

> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:681
> > +        removeNode(ancestor);
> 
> WTFMove(ancestor)

OK

> > Source/WebCore/inspector/InspectorCSSAgent.cpp:634
> >      RefPtr<Inspector::Protocol::CSS::CSSStyle> attributes = buildObjectForAttributesStyle(element);
> 
> Could if inside the if() condition:
> If (auto attributes = buildObjectForAttributesStyle(element))
>   ...

OK

> > Source/WebKit2/Shared/API/c/mac/WKWebArchive.cpp:64
> > +    auto mainResource = toImpl(webArchiveRef)->mainResource();
> 
> Looks like a behavior change. We used to return a ref'd mainResource but not
> anymore :(
> 
> > Source/WebKit2/Shared/API/c/mac/WKWebArchive.cpp:70
> > +    auto subresources = toImpl(webArchiveRef)->subresources();
> 
> Looks like a behavior change. We used to return a ref'd subresources but not
> anymore :(
> 
> > Source/WebKit2/Shared/API/c/mac/WKWebArchive.cpp:76
> > +    auto subframeArchives = toImpl(webArchiveRef)->subframeArchives();
> 
> Looks like a behavior change. We used to return a ref'd subframeArchives but
> not anymore :(

OK.

> > Source/WebKit2/UIProcess/WebPreferences.cpp:79
> >  PassRefPtr<WebPreferences> WebPreferences::copy() const
> 
> We should really return a Ref<>, it looks really ugly otherwise.

I limited my changes to just what was needed to build after removing calls to release(). I figured that more extensive changes to call sites, such as changing them to return Ref/RefPtr instead of PassRefPtr, could wait for a separate effort to remove PassRefPtr from those modules.

> > Source/WebKit2/WebProcess/Plugins/PDF/DeprecatedPDFPlugin.mm:670
> > +    return widget;
> 
> WTFMove(widget) ?

widget is a PassRefPtr, and the function returns a PassRefPtr, so WTFMove should not be needed.

> > Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:119
> > +    return frame;
> 
> WTFMove()

Ditto.

> > Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:135
> > +    return frame;
> 
> WTFMove()

Ditto.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1880
> > +        return snapshot;
> 
> WTFMove()

Ditto.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1912
> > +    return snapshot;
> 
> WTFMove()

Ditto.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1965
> > +    return snapshot;
> 
> WTFMove()

Ditto.

> > Tools/DumpRenderTree/mac/PixelDumpSupportMac.mm:197
> > +    return bitmapContext;
> 
> WTFMove()

Ditto.

> > Tools/DumpRenderTree/mac/PixelDumpSupportMac.mm:215
> > +    return bitmapContext;
> 
> WTFMove()

Ditto.

[Keith Rollin]

Created attachment 281513 [details]
Patch

[WebKit Commit Bot]

Attachment 281513 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:193:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/svg/SVGFESpecularLightingElement.cpp:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 206 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Rollin]

Created attachment 281574 [details]
Patch

[WebKit Commit Bot]

Attachment 281574 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:193:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/svg/SVGFESpecularLightingElement.cpp:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 206 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Rollin]

Created attachment 281579 [details]
Patch

[WebKit Commit Bot]

Attachment 281579 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:193:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/svg/SVGFESpecularLightingElement.cpp:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 206 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Rollin]

Created attachment 281584 [details]
Patch

[WebKit Commit Bot]

Attachment 281584 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:193:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/svg/SVGFESpecularLightingElement.cpp:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 206 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 281584 [details]
Patch

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

R=me with a fix.

> Source/WebCore/Modules/webdatabase/Database.cpp:249
> +        PassRefPtr<ScriptExecutionContext> passedContext = WTFMove(m_scriptExecutionContext);

This does not include my previous review feedback, please fix.

[Chris Dumez]

Comment on attachment 281584 [details]
Patch

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

>> Source/WebCore/Modules/webdatabase/Database.cpp:249
>> +        PassRefPtr<ScriptExecutionContext> passedContext = WTFMove(m_scriptExecutionContext);
> 
> This does not include my previous review feedback, please fix.

I see now that you thought my proposal was to big a change for this patch. However, at the very least, you need to get rid of this PassRefPtr<> local variable and use RefPtr<> instead.

[Chris Dumez]

Comment on attachment 281584 [details]
Patch

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

>>> Source/WebCore/Modules/webdatabase/Database.cpp:249
>>> +        PassRefPtr<ScriptExecutionContext> passedContext = WTFMove(m_scriptExecutionContext);
>> 
>> This does not include my previous review feedback, please fix.
> 
> I see now that you thought my proposal was to big a change for this patch. However, at the very least, you need to get rid of this PassRefPtr<> local variable and use RefPtr<> instead.

Even better, use auto:
auto context = WTFMove(m_scriptExecutionContext);

[Keith Rollin]

(In reply to comment #16)
> Comment on attachment 281584 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281584&action=review
> 
> >>> Source/WebCore/Modules/webdatabase/Database.cpp:249
> >>> +        PassRefPtr<ScriptExecutionContext> passedContext = WTFMove(m_scriptExecutionContext);
> >> 
> >> This does not include my previous review feedback, please fix.
> > 
> > I see now that you thought my proposal was to big a change for this patch. However, at the very least, you need to get rid of this PassRefPtr<> local variable and use RefPtr<> instead.
> 
> Even better, use auto:
> auto context = WTFMove(m_scriptExecutionContext);

Making this change will change the behavior of the code. Here's the block:

<foo> passedContext = WTFMove(m_scriptExecutionContext);
passedContext->postTask({ScriptExecutionContext::Task::CleanupTask, [passedContext] (ScriptExecutionContext& context) {
    ASSERT_UNUSED(context, &context == passedContext);
    RefPtr<ScriptExecutionContext> scriptExecutionContext(passedContext);
}});

If <foo> is a PassRefPtr, then the WTFMove will leak the object from m_scriptExecutionContext to passedContext. m_scriptExecutionContext ends up with nullptr, passedContext owns the object, and no refcounts are changed. The passedContext is then used to create the lambda. I'm assuming a copy of passedContext is created here, such that the copy now has the object and passedContext has nullptr and no refcounts were changed[1]. Finally, scriptExecutionContext is created. The managed object is transferred to scriptExecutionContext, the passedContext copy has nullptr, and no refcounts were changed. Finally, scriptExecutionContext is destructed, dereffing the managed object, possibly deleting it.

If <foo> is a RefPtr, then the WTFMove will transfer the managed object as before: m_scriptExecutionContext ends up with nullptr, passedContext owns the object, and no refcounts are changed. But then the lambda is created. The passedContext is copied, incrementing the refcount. Then scriptExecutionContext is created, incrementing the refcount again. It's now at least three: one for passedContext, one for the passedContext copy, and one for scriptExecution. If the lambda is executed asynchronously, that first one will actually have been dropped when the Database destructor exited. And the one for scriptExecutionContext will get dropped when the lambda exits. This means that there will still be a reference to the managed object if <foo> is a RefPtr, whereas it could be zero if <foo> is a PassRefPtr.

At the very least, this change in behavior means that the ScriptExecutionContext will get deleted at a different point in the flow. But -- without checking postTask and associated code -- I could possibly see that objects won't get deleted at all.

Using RefPtr also means additional reffing and dereffing, leading to an insignificant performance regression.

------

[1] I'm actually unsure about what's actually going on here. 'passedContext' is captured by value, which would seem to mean that a copy is created. According to cppreference.com, specifying just a variable name in the capture list is a "simple by-copy capture". However, if that were to happen and the original passedContext had its internal pointer set to nullptr, then I don't know how passedContext->postTask() could work.

Here's a little test to see what's going on:

#include <stdio.h>
#include <utility>

class Fred
{
    public:
        Fred() : m_member(-1) {
            printf("Default constructor\n");
            m_member = 10;
        }
        Fred(const Fred& other) : m_member(-2) {
            printf("Copy constructor\n");
            m_member = other.m_member;
            const_cast<Fred&>(other).m_member = -12;
        }
        Fred(Fred&& other) : m_member(-3) {
            printf("Move constructor\n");
            m_member = std::exchange(other.m_member, 0);
            const_cast<Fred&>(other).m_member = -13;
        }

        int m_member;
};

int main()
{
    Fred aFred;

    printf("main: aFred.m_member = %d\n", aFred.m_member);

    auto closure = [aFred] () {
        printf("closure: aFred.m_member = %d\n", aFred.m_member);
    };

    printf("main: aFred.m_member = %d\n", aFred.m_member);
    closure();
    printf("main: aFred.m_member = %d\n", aFred.m_member);

    return 0;
}

$ clang -std=c++14 -O2 main.cpp 
$ ./a.out 
Default constructor
main: aFred.m_member = 10
Copy constructor
main: aFred.m_member = -12
closure: aFred.m_member = 10
main: aFred.m_member = -12

So this shows that the captured object's copy constructor is called. Which should mean that passedContext's managed pointer should be nullptr after the copy is created for the lambda. So I don't know how passedContext->postTask(...) is not crashing. I must be missing something stupid.

[Chris Dumez]

(In reply to comment #17)
> (In reply to comment #16)
> > Comment on attachment 281584 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=281584&action=review
> > 
> > >>> Source/WebCore/Modules/webdatabase/Database.cpp:249
> > >>> +        PassRefPtr<ScriptExecutionContext> passedContext = WTFMove(m_scriptExecutionContext);
> > >> 
> > >> This does not include my previous review feedback, please fix.
> > > 
> > > I see now that you thought my proposal was to big a change for this patch. However, at the very least, you need to get rid of this PassRefPtr<> local variable and use RefPtr<> instead.
> > 
> > Even better, use auto:
> > auto context = WTFMove(m_scriptExecutionContext);
> 
> Making this change will change the behavior of the code. Here's the block:
> 
> <foo> passedContext = WTFMove(m_scriptExecutionContext);
> passedContext->postTask({ScriptExecutionContext::Task::CleanupTask,
> [passedContext] (ScriptExecutionContext& context) {
>     ASSERT_UNUSED(context, &context == passedContext);
>     RefPtr<ScriptExecutionContext> scriptExecutionContext(passedContext);
> }});
> 
> If <foo> is a PassRefPtr, then the WTFMove will leak the object from
> m_scriptExecutionContext to passedContext. m_scriptExecutionContext ends up
> with nullptr, passedContext owns the object, and no refcounts are changed.
> The passedContext is then used to create the lambda. I'm assuming a copy of
> passedContext is created here, such that the copy now has the object and
> passedContext has nullptr and no refcounts were changed[1]. Finally,
> scriptExecutionContext is created. The managed object is transferred to
> scriptExecutionContext, the passedContext copy has nullptr, and no refcounts
> were changed. Finally, scriptExecutionContext is destructed, dereffing the
> managed object, possibly deleting it.
> 
> If <foo> is a RefPtr, then the WTFMove will transfer the managed object as
> before: m_scriptExecutionContext ends up with nullptr, passedContext owns
> the object, and no refcounts are changed. But then the lambda is created.
> The passedContext is copied, incrementing the refcount. Then
> scriptExecutionContext is created, incrementing the refcount again. It's now
> at least three: one for passedContext, one for the passedContext copy, and
> one for scriptExecution. If the lambda is executed asynchronously, that
> first one will actually have been dropped when the Database destructor
> exited. And the one for scriptExecutionContext will get dropped when the
> lambda exits. This means that there will still be a reference to the managed
> object if <foo> is a RefPtr, whereas it could be zero if <foo> is a
> PassRefPtr.

This does not seem totally accurate to me. RefCount should definitely be 0 by the time the Lambda object is destroyed. When the lambda is destroyed, the refcount should be decreased by 2, once when scriptExecutionContext is destroyed and a second time when passedContext is destroyed.

You are right that what I suggested does a bit more refcount churn but:
1. We should drop the scriptExecutionContext variable that is in the lambda. The captured passedContext will already be a RefPtr.
2. It seems safer. As you said, when passedContext is captured, the one outside the lambda becomes null. It looks like it works since this code isn't crashing but I am not 100% sure this is defined behavior since we are calling a function on passedContext on the same line.
3. My original proposal did not cause any additional ref counting churn I believe and seemed safe.

> 
> At the very least, this change in behavior means that the
> ScriptExecutionContext will get deleted at a different point in the flow.
> But -- without checking postTask and associated code -- I could possibly see
> that objects won't get deleted at all.
> 
> Using RefPtr also means additional reffing and dereffing, leading to an
> insignificant performance regression.
> 
> ------
> 
> [1] I'm actually unsure about what's actually going on here. 'passedContext'
> is captured by value, which would seem to mean that a copy is created.
> According to cppreference.com, specifying just a variable name in the
> capture list is a "simple by-copy capture". However, if that were to happen
> and the original passedContext had its internal pointer set to nullptr, then
> I don't know how passedContext->postTask() could work.
> 
> Here's a little test to see what's going on:
> 
> #include <stdio.h>
> #include <utility>
> 
> class Fred
> {
>     public:
>         Fred() : m_member(-1) {
>             printf("Default constructor\n");
>             m_member = 10;
>         }
>         Fred(const Fred& other) : m_member(-2) {
>             printf("Copy constructor\n");
>             m_member = other.m_member;
>             const_cast<Fred&>(other).m_member = -12;
>         }
>         Fred(Fred&& other) : m_member(-3) {
>             printf("Move constructor\n");
>             m_member = std::exchange(other.m_member, 0);
>             const_cast<Fred&>(other).m_member = -13;
>         }
> 
>         int m_member;
> };
> 
> int main()
> {
>     Fred aFred;
> 
>     printf("main: aFred.m_member = %d\n", aFred.m_member);
> 
>     auto closure = [aFred] () {
>         printf("closure: aFred.m_member = %d\n", aFred.m_member);
>     };
> 
>     printf("main: aFred.m_member = %d\n", aFred.m_member);
>     closure();
>     printf("main: aFred.m_member = %d\n", aFred.m_member);
> 
>     return 0;
> }
> 
> $ clang -std=c++14 -O2 main.cpp 
> $ ./a.out 
> Default constructor
> main: aFred.m_member = 10
> Copy constructor
> main: aFred.m_member = -12
> closure: aFred.m_member = 10
> main: aFred.m_member = -12
> 
> So this shows that the captured object's copy constructor is called. Which
> should mean that passedContext's managed pointer should be nullptr after the
> copy is created for the lambda. So I don't know how
> passedContext->postTask(...) is not crashing. I must be missing something
> stupid.

[Chris Dumez]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > Comment on attachment 281584 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=281584&action=review
> > > 
> > > >>> Source/WebCore/Modules/webdatabase/Database.cpp:249
> > > >>> +        PassRefPtr<ScriptExecutionContext> passedContext = WTFMove(m_scriptExecutionContext);
> > > >> 
> > > >> This does not include my previous review feedback, please fix.
> > > > 
> > > > I see now that you thought my proposal was to big a change for this patch. However, at the very least, you need to get rid of this PassRefPtr<> local variable and use RefPtr<> instead.
> > > 
> > > Even better, use auto:
> > > auto context = WTFMove(m_scriptExecutionContext);
> > 
> > Making this change will change the behavior of the code. Here's the block:
> > 
> > <foo> passedContext = WTFMove(m_scriptExecutionContext);
> > passedContext->postTask({ScriptExecutionContext::Task::CleanupTask,
> > [passedContext] (ScriptExecutionContext& context) {
> >     ASSERT_UNUSED(context, &context == passedContext);
> >     RefPtr<ScriptExecutionContext> scriptExecutionContext(passedContext);
> > }});
> > 
> > If <foo> is a PassRefPtr, then the WTFMove will leak the object from
> > m_scriptExecutionContext to passedContext. m_scriptExecutionContext ends up
> > with nullptr, passedContext owns the object, and no refcounts are changed.
> > The passedContext is then used to create the lambda. I'm assuming a copy of
> > passedContext is created here, such that the copy now has the object and
> > passedContext has nullptr and no refcounts were changed[1]. Finally,
> > scriptExecutionContext is created. The managed object is transferred to
> > scriptExecutionContext, the passedContext copy has nullptr, and no refcounts
> > were changed. Finally, scriptExecutionContext is destructed, dereffing the
> > managed object, possibly deleting it.
> > 
> > If <foo> is a RefPtr, then the WTFMove will transfer the managed object as
> > before: m_scriptExecutionContext ends up with nullptr, passedContext owns
> > the object, and no refcounts are changed. But then the lambda is created.
> > The passedContext is copied, incrementing the refcount. Then
> > scriptExecutionContext is created, incrementing the refcount again. It's now
> > at least three: one for passedContext, one for the passedContext copy, and
> > one for scriptExecution. If the lambda is executed asynchronously, that
> > first one will actually have been dropped when the Database destructor
> > exited. And the one for scriptExecutionContext will get dropped when the
> > lambda exits. This means that there will still be a reference to the managed
> > object if <foo> is a RefPtr, whereas it could be zero if <foo> is a
> > PassRefPtr.
> 
> This does not seem totally accurate to me. RefCount should definitely be 0
> by the time the Lambda object is destroyed. When the lambda is destroyed,
> the refcount should be decreased by 2, once when scriptExecutionContext is
> destroyed and a second time when passedContext is destroyed.
> 
> You are right that what I suggested does a bit more refcount churn but:
> 1. We should drop the scriptExecutionContext variable that is in the lambda.
> The captured passedContext will already be a RefPtr.
> 2. It seems safer. As you said, when passedContext is captured, the one
> outside the lambda becomes null. It looks like it works since this code
> isn't crashing but I am not 100% sure this is defined behavior since we are
> calling a function on passedContext on the same line.
> 3. My original proposal did not cause any additional ref counting churn I
> believe and seemed safe.
> 
> > 
> > At the very least, this change in behavior means that the
> > ScriptExecutionContext will get deleted at a different point in the flow.
> > But -- without checking postTask and associated code -- I could possibly see
> > that objects won't get deleted at all.
> > 
> > Using RefPtr also means additional reffing and dereffing, leading to an
> > insignificant performance regression.
> > 
> > ------
> > 
> > [1] I'm actually unsure about what's actually going on here. 'passedContext'
> > is captured by value, which would seem to mean that a copy is created.
> > According to cppreference.com, specifying just a variable name in the
> > capture list is a "simple by-copy capture". However, if that were to happen
> > and the original passedContext had its internal pointer set to nullptr, then
> > I don't know how passedContext->postTask() could work.
> > 
> > Here's a little test to see what's going on:
> > 
> > #include <stdio.h>
> > #include <utility>
> > 
> > class Fred
> > {
> >     public:
> >         Fred() : m_member(-1) {
> >             printf("Default constructor\n");
> >             m_member = 10;
> >         }
> >         Fred(const Fred& other) : m_member(-2) {
> >             printf("Copy constructor\n");
> >             m_member = other.m_member;
> >             const_cast<Fred&>(other).m_member = -12;
> >         }
> >         Fred(Fred&& other) : m_member(-3) {
> >             printf("Move constructor\n");
> >             m_member = std::exchange(other.m_member, 0);
> >             const_cast<Fred&>(other).m_member = -13;
> >         }
> > 
> >         int m_member;
> > };
> > 
> > int main()
> > {
> >     Fred aFred;
> > 
> >     printf("main: aFred.m_member = %d\n", aFred.m_member);
> > 
> >     auto closure = [aFred] () {
> >         printf("closure: aFred.m_member = %d\n", aFred.m_member);
> >     };
> > 
> >     printf("main: aFred.m_member = %d\n", aFred.m_member);
> >     closure();
> >     printf("main: aFred.m_member = %d\n", aFred.m_member);
> > 
> >     return 0;
> > }
> > 
> > $ clang -std=c++14 -O2 main.cpp 
> > $ ./a.out 
> > Default constructor
> > main: aFred.m_member = 10
> > Copy constructor
> > main: aFred.m_member = -12
> > closure: aFred.m_member = 10
> > main: aFred.m_member = -12
> > 
> > So this shows that the captured object's copy constructor is called. Which
> > should mean that passedContext's managed pointer should be nullptr after the
> > copy is created for the lambda. So I don't know how
> > passedContext->postTask(...) is not crashing. I must be missing something
> > stupid.

Seems to be what I discussed in the point 2 above. I agree with you that the existing code looks risky. Even though it seems to be working, I suspect this is undefined behavior. Both of my proposals (the better original one or the less efficient / simpler recent one) would be safer AFAICT thought.

[Keith Rollin]

After chatting with Chris, we've agreed to clean up Database::~Database in a second patch, and leave that destructor completely unchanged in this patch.

[Keith Rollin]

Created attachment 281662 [details]
Patch

[WebKit Commit Bot]

Attachment 281662 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:193:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/svg/SVGFESpecularLightingElement.cpp:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 205 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 281662 [details]
Patch

Rejecting attachment 281662 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 281662, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebKit/mac/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/1538034

[Keith Rollin]

Created attachment 281671 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 281671 [details]
Patch

Clearing flags on attachment: 281671

Committed r202242: <http://trac.webkit.org/changeset/202242>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.