Bug 136523 – Crash in GMainLoopSource when destroying the object during callback dispatch

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

[Zan Dobersek]

Crash in GMainLoopSource when destroying the object during callback dispatch

[Zan Dobersek]

Created attachment 237612 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:77:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:334:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:47:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 5 files


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

[Carlos Garcia Campos]

Comment on attachment 237612 [details]
Patch

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

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:79
> +    if (m_cancellable)
> +        g_cancellable_cancel(m_cancellable.get());

g_cancellable_cancel is null safe, so we can avoid the if.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:94
> -    if (context.cancellable)
> -        g_cancellable_cancel(context.cancellable.get());
> +    if (context.socketCancellable)
> +        g_cancellable_cancel(context.socketCancellable.get());

And we can do it here too.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:224
> +    if (g_cancellable_is_cancelled(context.cancellable.get())) {
> +        dispatchDestroyCallback(context.destroyCallback);
> +        return;
> +    }

Why only dispatch? we also want to delete the object in case of delete on destroy.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:260
> +    if (g_cancellable_is_cancelled(context.cancellable.get())) {
> +        dispatchDestroyCallback(context.destroyCallback);
> +        return Stop;
> +    }

Ditto.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:338
> +void GMainLoopSource::dispatchDestroyCallback(const std::function<void ()>& destroyCallback)
> +{
> +    if (destroyCallback)
> +        destroyCallback();
> +}

I'm not sure I understand why we would want to do this instead of destroy().

> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:46
> +        g_timeout_add(250,

Why 250?

> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:181
> +            context.test.source().cancel();

Why not finishing the loop here?

> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:185
> +    context.test.delayedFinish();

What's the delayed finish for? To prevent a timeout?

> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:245
> +                context.test.source().cancel();

Ditto.

> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:264
> +                    context.test.source().cancel();

Ditto.

> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:303
> +                // Should not be called.
> +                context.firstDestroyCallbackCalled = true;

I don't think this is right. The destroy callback is not for the GMainLoopSource object, but for the glib source. The only case where the destroy is not called is when the source is cancelled before being scheduled (that's the reason of the early return in cancel when the source is NULL). This is not actually a re-schedule, but a new source using the same GMainLoopSource.

> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:331
> +                // Should not be called.
> +                context.firstDestroyCallbackCalled = true;

Same here. We are only testing cancellation during dispatch, but there are at least 3 more cases of cancellation that it would be good to test as well.

 - Cancel before scheduling. This is what happens in every cancel() called from schedule methods the first time. It should do nothing since there's no active source, so no destroy either.
 - Cancel once scheduled, but before dispatched. The callback should not be called, the source is destroyed and the destroy should be dispatched. This is already covered by cancel(), because the move to the context hasn't happened and m_context.source is a valid pointer.
 - Cancel once dispatched, but before finished. That's what this patch is about. The ongoing source should be destroyed (the glib source is destroyed by the callbacks returning Stop), and destroy should happen.
 - Cancel after source is destroyed. This should be the same than the first case, nothing should happen.

[Zan Dobersek]

Comment on attachment 237612 [details]
Patch

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

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:79
>> +        g_cancellable_cancel(m_cancellable.get());
> 
> g_cancellable_cancel is null safe, so we can avoid the if.

OK.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:94
>> +        g_cancellable_cancel(context.socketCancellable.get());
> 
> And we can do it here too.

OK.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:224
>> +    }
> 
> Why only dispatch? we also want to delete the object in case of delete on destroy.

Right, another special case.

The original crash was caused because GMainLoopSource was being deleted during the callback dispatch. Calling ::destroy() under that condition here would just repeat the problem since you'd again operate on a destroyed object.

>> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:46
>> +        g_timeout_add(250,
> 
> Why 250?

Arbitrary, it should be enough for all the tasks on the main context to be dispatched.

>> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:181
>> +            context.test.source().cancel();
> 
> Why not finishing the loop here?

To avoid having the tested code controlling its own harness.

Basically we'd just like for the callback to only cancel itself, not to finish the test ...

>> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:185
>> +    context.test.delayedFinish();
> 
> What's the delayed finish for? To prevent a timeout?

... which is done here, after a quarter of a second.

>> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:303
>> +                context.firstDestroyCallbackCalled = true;
> 
> I don't think this is right. The destroy callback is not for the GMainLoopSource object, but for the glib source. The only case where the destroy is not called is when the source is cancelled before being scheduled (that's the reason of the early return in cancel when the source is NULL). This is not actually a re-schedule, but a new source using the same GMainLoopSource.

OK, understood.

>> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:331
>> +                context.firstDestroyCallbackCalled = true;
> 
> Same here. We are only testing cancellation during dispatch, but there are at least 3 more cases of cancellation that it would be good to test as well.
> 
>  - Cancel before scheduling. This is what happens in every cancel() called from schedule methods the first time. It should do nothing since there's no active source, so no destroy either.
>  - Cancel once scheduled, but before dispatched. The callback should not be called, the source is destroyed and the destroy should be dispatched. This is already covered by cancel(), because the move to the context hasn't happened and m_context.source is a valid pointer.
>  - Cancel once dispatched, but before finished. That's what this patch is about. The ongoing source should be destroyed (the glib source is destroyed by the callbacks returning Stop), and destroy should happen.
>  - Cancel after source is destroyed. This should be the same than the first case, nothing should happen.

OK.

[Carlos Garcia Campos]

(In reply to comment #4)
> (From update of attachment 237612 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237612&action=review
> 

> > Why only dispatch? we also want to delete the object in case of delete on destroy.
> 
> Right, another special case.
> 
> The original crash was caused because GMainLoopSource was being deleted during the callback dispatch. Calling ::destroy() under that condition here would just repeat the problem since you'd again operate on a destroyed object.

That's also why we called cancel() in the destructor (that ensured destroy happened before the object is destroyed, this is probably another case to test since you are not checking that the destroy callback is called in DestructionDuringDispatch, I think) and destroy() was the last thing done in all callbacks.

> >> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:46
> >> +        g_timeout_add(250,
> > 
> > Why 250?
> 
> Arbitrary, it should be enough for all the tasks on the main context to be dispatched.
> 
> >> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:181
> >> +            context.test.source().cancel();
> > 
> > Why not finishing the loop here?
> 
> To avoid having the tested code controlling its own harness.

Isn't that what the other tests do?

> Basically we'd just like for the callback to only cancel itself, not to finish the test ...
> 
> >> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:185
> >> +    context.test.delayedFinish();
> > 
> > What's the delayed finish for? To prevent a timeout?
> 
> ... which is done here, after a quarter of a second.
>

[Zan Dobersek]

I'll incorporate these changes and the reviewing feedback in bug #135800 which was reopened after rolling out the original patch in r173267.
http://trac.webkit.org/changeset/173267

*** This bug has been marked as a duplicate of bug 135800 ***

[Zan Dobersek]

Comment on attachment 237612 [details]
Patch

Clearing the review flags.