Bug 135800 – [GTK] GMainLoopSource is exposed to race conditions

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

GMainLoopSource is exposed to race conditions

[Zan Dobersek]

Created attachment 236370 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:74:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:91:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:93:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:94:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:311:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 3 files


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

[Carlos Garcia Campos]

Comment on attachment 236370 [details]
Patch

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

Is there a test case to reproduce the issue? Would it be possible to add a test case to the unit test? Have you considered that GSource is already thread-safe?

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:44
> +    , m_mutex(new GMutex)

Why a heap allocated mutex?

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:46
> +    g_mutex_init(m_mutex);

You can use g_mutex_init with a stack allocated mutex.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:54
> +    , m_mutex(new GMutex)
>  {
> +    g_mutex_init(m_mutex);

Ditto.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:64
> +    g_mutex_clear(m_mutex);

And then you don't need this

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:65
> +    delete m_mutex;

Neither this.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:71
> +    GMutexLocker locker(m_mutex);
>      return m_status == Scheduled;

Do we really need a mutex lock to this simple query?

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:77
> +    GMutexLocker locker(m_mutex);
>      return m_status != Ready;

Ditto.

[Zan Dobersek]

Comment on attachment 236370 [details]
Patch

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

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:44
>> +    , m_mutex(new GMutex)
> 
> Why a heap allocated mutex?

That's how GMutex is currently allocated elsewhere in the code base. I'll switch those cases to a non-heap-allocated GMutex as well.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:64
>> +    g_mutex_clear(m_mutex);
> 
> And then you don't need this

Are you sure? g_mutex_clear() frees up the pthread mutex that g_mutex_init() allocates via malloc.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:71
>>      return m_status == Scheduled;
> 
> Do we really need a mutex lock to this simple query?

On a second thought I think mutex locking shouldn't matter here.

[Carlos Garcia Campos]

(In reply to comment #4)
> (From update of attachment 236370 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236370&action=review
> 
> >> Source/WTF/wtf/gobject/GMainLoopSource.cpp:44
> >> +    , m_mutex(new GMutex)
> > 
> > Why a heap allocated mutex?
> 
> That's how GMutex is currently allocated elsewhere in the code base. I'll switch those cases to a non-heap-allocated GMutex as well.
> 
> >> Source/WTF/wtf/gobject/GMainLoopSource.cpp:64
> >> +    g_mutex_clear(m_mutex);
> > 
> > And then you don't need this
> 
> Are you sure? g_mutex_clear() frees up the pthread mutex that g_mutex_init() allocates via malloc.

I was wrong, I don't know why I was assuming it was a static mutex.

> >> Source/WTF/wtf/gobject/GMainLoopSource.cpp:71
> >>      return m_status == Scheduled;
> > 
> > Do we really need a mutex lock to this simple query?
> 
> On a second thought I think mutex locking shouldn't matter here.

[Zan Dobersek]

Created attachment 236441 [details]
Patch

Doesn't yet include a test case. It should build, though.

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:75:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:93:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:94:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:95:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:306:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 3 files


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

[Zan Dobersek]

Created attachment 236583 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:75:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:93:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:94:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:95:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:306:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 6 files


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

[Carlos Garcia Campos]

Comment on attachment 236583 [details]
Patch

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

Thanks for the patch, I haven't had time to look at this in detail yet, so I have only a couple of comments.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:60
> -    cancel();
> +    {
> +        GMutexLocker locker(m_mutex);
> +        cancelInternal();
> +    }

This is exactly cancel now, no?

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:81
> +void GMainLoopSource::cancelInternal()

I would rather use cancelUnlocked()

> Tools/ChangeLog:12
> +        * TestWebKitAPI/Tests/WebKit2Gtk/CMakeLists.txt:
> +        * TestWebKitAPI/Tests/WebKit2Gtk/TestGMainLoopSource.cpp: Added.

This is not public WebKit2GTK API, would it be possible to add it to the TestWTF instead? like Tools/TestWebKitAPI/Tests/WTF/gobject/GUniquePtr.cpp

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestGMainLoopSource.cpp:136
> +    GMainLoopSourceTest::add("GMainLoopSource", "rescheduling-from-different-thread-during-dispatch", testGMainLoopSourceDifferentThreadRescheduling);

So, this one fails with current code? or is it flaky because the issue is indeed a race condition?

[Zan Dobersek]

Comment on attachment 236583 [details]
Patch

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

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:60
>> +    }
> 
> This is exactly cancel now, no?

Ah, yes it is.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:81
>> +void GMainLoopSource::cancelInternal()
> 
> I would rather use cancelUnlocked()

cancelUnlocked() tells me that the mutex is not locked during the execution of that method, even if the caller did lock it. How about cancelWithoutLocking()?

>> Tools/ChangeLog:12
>> +        * TestWebKitAPI/Tests/WebKit2Gtk/TestGMainLoopSource.cpp: Added.
> 
> This is not public WebKit2GTK API, would it be possible to add it to the TestWTF instead? like Tools/TestWebKitAPI/Tests/WTF/gobject/GUniquePtr.cpp

Yeah, it'll fit better there.

>> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestGMainLoopSource.cpp:136
>> +    GMainLoopSourceTest::add("GMainLoopSource", "rescheduling-from-different-thread-during-dispatch", testGMainLoopSourceDifferentThreadRescheduling);
> 
> So, this one fails with current code? or is it flaky because the issue is indeed a race condition?

That's the one. It's easy to fail the test on single-threaded systems, but it's rather hard to have it fail on multi-threaded ones.

[Zan Dobersek]

Created attachment 236592 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:75:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:93:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:94:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:95:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:306:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 6 files


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

[Philippe Normand]

Comment on attachment 236592 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:26
> +#if 0

o_O

[Zan Dobersek]

Created attachment 236649 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:75:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:93:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:94:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:95:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:306:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 6 files


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

[Carlos Garcia Campos]

(In reply to comment #11)
> (From update of attachment 236583 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236583&action=review
> 
> >> Source/WTF/wtf/gobject/GMainLoopSource.cpp:60
> >> +    }
> > 
> > This is exactly cancel now, no?
> 
> Ah, yes it is.
> 
> >> Source/WTF/wtf/gobject/GMainLoopSource.cpp:81
> >> +void GMainLoopSource::cancelInternal()
> > 
> > I would rather use cancelUnlocked()
> 
> cancelUnlocked() tells me that the mutex is not locked during the execution of that method,

That's exactly what is happening

> even if the caller did lock it. 

ah!, well, the unlocked suffix is a common pattern in glib, but I'm fine with any name that makes it clear this is executed without the lock. Internal is a very confusing name in this case, IMO.

> How about cancelWithoutLocking()?

Sounds good to me.

> >> Tools/ChangeLog:12
> >> +        * TestWebKitAPI/Tests/WebKit2Gtk/TestGMainLoopSource.cpp: Added.
> > 
> > This is not public WebKit2GTK API, would it be possible to add it to the TestWTF instead? like Tools/TestWebKitAPI/Tests/WTF/gobject/GUniquePtr.cpp
> 
> Yeah, it'll fit better there.
> 
> >> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestGMainLoopSource.cpp:136
> >> +    GMainLoopSourceTest::add("GMainLoopSource", "rescheduling-from-different-thread-during-dispatch", testGMainLoopSourceDifferentThreadRescheduling);
> > 
> > So, this one fails with current code? or is it flaky because the issue is indeed a race condition?
> 
> That's the one. It's easy to fail the test on single-threaded systems, but it's rather hard to have it fail on multi-threaded ones.

I see

[Zan Dobersek]

Created attachment 237313 [details]
Patch

Thought I have already uploaded the refined version.

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:75:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:93:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:94:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:95:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:306:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 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 237313 [details]
Patch

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

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:60
> -    cancel();
> +    {
> +        GMutexLocker locker(m_mutex);
> +        cancelWithoutLocking();
> +    }

I still think this is not needed, because cancel() is now lock + cancelWithoutLocking()

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:84
> +        m_status = Ready;

Shouldn't this be Ready when context is NULL? Should we add an assert instead?

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:94
> +    destroy(context.destroyCallback);
> +    g_source_destroy(context.source.get());

Are you sure we can change the order here?

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:204
> +        if (!m_context.source)
> +            return;

Do we really need this check inside the lock?

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:219
> +    bool shouldDestroy = false;
> +    {
> +        GMutexLocker locker(m_mutex);
> +        shouldDestroy = !m_context.source;
> +    }

Ok, so we no longer need to check if the source is the same, because with the move, the m_context is reset and its sources is NULL, right? so we only need to check if a new source has been scheduled. Since it's only a null check, do we really need to take the lock here?

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:223
> +    g_source_destroy(context.source.get());

I don't think you need to destroy the source here, voidSourceCallback always returns G_SOURCE_REMOVE so it will be destroyed.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:253
> +        if (retval) {
> +            if (!m_context.source)
> +                m_context = WTF::move(context);
> +            return Continue;
> +        }
> +
> +        shouldDestroy = !m_context.source;

Maybe we could simply this a bit:

if (retval && !m_context.source)
    m_context = WTF::move(context);
else
    shouldDestroy = !m_context.source;

And then leave it returning retval instead of Stop.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:258
> +    g_source_destroy(context.source.get());

I don't think you should explicitly destroy the source, it will be destroyed when we return Stop here.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:281
> +        g_source_destroy(context.source.get());

Same here.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:294
> +        if (retval) {
> +            if (!m_context.source)
> +                m_context = WTF::move(context);
> +            return Continue;
> +        }

Same comments here since it's mostly the same than the bool callback case.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:301
> +    g_source_destroy(context.source.get());

Ditto.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:309
> +void GMainLoopSource::destroy(const std::function<void ()>& destroyCallback)
>  {
> -    auto destroyCallback = WTF::move(m_destroyCallback);
> -    auto deleteOnDestroy = m_deleteOnDestroy;
> -    reset();
> +    m_status = Ready;
> +    DeleteOnDestroyType deleteOnDestroy = m_deleteOnDestroy;

So, since we don't have reset now, because the reset is done by the move and the callback func is passed as an argument, we should make sure this destroy is always called on an already reset(moved) context, right? Could we add an assert here to ensure that?

> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:30
> +        : m_mainLoop(g_main_loop_new(0, TRUE))

Use nullptr instead of 0 here.

> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:64
> +        ASSERT_NOT_REACHED();

I guess this is noop in Release builds, the unit tests should assert also in Release builds. I don't understand the test, though, why the callback should never be called?

> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:67
> +    context.test.source().schedule("[Test] SecondTask", [&] {

Ah, ok, because it's the same source, the first one is cancelled. I think it would be easier to understand if you add a comment before the ASSERT or you use another bool variable finishedFirstTask and then you also check it's false.

> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:73
> +    ASSERT(context.finishedSecondTask);

You should use the gtest assert macros instead, ASSERT_TRUE/ASSERT_FALSE, etc.

> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:81
> +        bool finishedFirstTask = false;
> +        bool finishedSecondTask = false;

Like this :-)

[Carlos Garcia Campos]

Comment on attachment 237313 [details]
Patch

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

> Tools/ChangeLog:11
> +        * TestWebKitAPI/PlatformGTK.cmake:

This change is not part of the patch

[Zan Dobersek]

Comment on attachment 237313 [details]
Patch

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

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:60
>> +    }
> 
> I still think this is not needed, because cancel() is now lock + cancelWithoutLocking()

OK.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:84
>> +        m_status = Ready;
> 
> Shouldn't this be Ready when context is NULL? Should we add an assert instead?

No, m_status isn't Ready when a callback that is being executed in e.g. voidCallback() (m_status at that point equaling Dispatched) reschedules the same GMainLoopSource with a new callback.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:94
>> +    g_source_destroy(context.source.get());
> 
> Are you sure we can change the order here?

I'll revert it, just in case. It shouldn't affect anything.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:204
>> +            return;
> 
> Do we really need this check inside the lock?

We do, because between the time this check is performed outside of this scope (with m_context being non-null) and entering the scope and moving out of m_context below, the ::reset() method could have been called, and suddenly the context ends up with a null value.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:219
>> +    }
> 
> Ok, so we no longer need to check if the source is the same, because with the move, the m_context is reset and its sources is NULL, right? so we only need to check if a new source has been scheduled. Since it's only a null check, do we really need to take the lock here?

Correct on the first part.

Again, the lock is required here to avoid parallel clashes with the execution of the ::cancel() method, for instance if the GMainLoopSource object was rescheduled inside the callback and a cancellation followed immediately after.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:223
>> +    g_source_destroy(context.source.get());
> 
> I don't think you need to destroy the source here, voidSourceCallback always returns G_SOURCE_REMOVE so it will be destroyed.

OK, makes sense.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:253
>> +        shouldDestroy = !m_context.source;
> 
> Maybe we could simply this a bit:
> 
> if (retval && !m_context.source)
>     m_context = WTF::move(context);
> else
>     shouldDestroy = !m_context.source;
> 
> And then leave it returning retval instead of Stop.

OK, I'll try this out.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:258
>> +    g_source_destroy(context.source.get());
> 
> I don't think you should explicitly destroy the source, it will be destroyed when we return Stop here.

OK.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:281
>> +        g_source_destroy(context.source.get());
> 
> Same here.

OK.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:294
>> +        }
> 
> Same comments here since it's mostly the same than the bool callback case.

OK.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:301
>> +    g_source_destroy(context.source.get());
> 
> Ditto.

OK.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:309
>> +    DeleteOnDestroyType deleteOnDestroy = m_deleteOnDestroy;
> 
> So, since we don't have reset now, because the reset is done by the move and the callback func is passed as an argument, we should make sure this destroy is always called on an already reset(moved) context, right? Could we add an assert here to ensure that?

OK, I'll add an assert for a null m_socket.source.

>> Tools/ChangeLog:11
>> +        * TestWebKitAPI/PlatformGTK.cmake:
> 
> This change is not part of the patch

Right.

>> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:30
>> +        : m_mainLoop(g_main_loop_new(0, TRUE))
> 
> Use nullptr instead of 0 here.

OK.

>> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:67
>> +    context.test.source().schedule("[Test] SecondTask", [&] {
> 
> Ah, ok, because it's the same source, the first one is cancelled. I think it would be easier to understand if you add a comment before the ASSERT or you use another bool variable finishedFirstTask and then you also check it's false.

I'll add the finishFirstTask variable.

>> Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:73
>> +    ASSERT(context.finishedSecondTask);
> 
> You should use the gtest assert macros instead, ASSERT_TRUE/ASSERT_FALSE, etc.

OK.

[Zan Dobersek]

Created attachment 237458 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:75:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:93:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:94:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:95:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:290:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 5 files


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

[Zan Dobersek]

Created attachment 237476 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:75:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:93:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:94:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:95:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:290:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 6 files


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

[Carlos Garcia Campos]

I wonder if we can remove the mutex used in UIProcess/Plugins/gtk/PluginInfoCache.cpp once this lands

[Zan Dobersek]

Comment on attachment 237476 [details]
Patch

Clearing flags on attachment: 237476

Committed r173201: <http://trac.webkit.org/changeset/173201>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Carlos Alberto Lopez Perez]

(In reply to comment #29)
> All reviewed patches have been landed.  Closing bug.

This has caused a regression the test TestWebKitAPI/WebKit2Gtk/TestDownloads. Reported here: https://bugs.webkit.org/show_bug.cgi?id=136530

[WebKit Commit Bot]

Re-opened since this is blocked by bug 136538

[Zan Dobersek]

*** Bug 136523 has been marked as a duplicate of this bug. ***

[Zan Dobersek]

Created attachment 237846 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:97:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:98:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:99:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:100:  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: 5 in 6 files


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

[Zan Dobersek]

Comment on attachment 237846 [details]
Patch

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

GCC 4.9 has some issues with deducing the bool-returning lambda types, hence the build errors. I'll address it. Adding a few more comments addressing concerns that were left in bug #136523.

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

This avoids controlling the main loop runtime via scheduling the destroy callbacks on GMainLoopSource. I don't think controlling that through the destroy callbacks is a good idea, especially when destroy callbacks are being explicitly tested.

But there are a few instances where we quit the loop in the destroy callback. I'd rather switch those cases to using delayedFinish().

[Carlos Garcia Campos]

Comment on attachment 237846 [details]
Patch

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

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

We should also cancel the socket cancellable here I guess.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:95
> +    Context context = WTF::move(m_context);
> +    context.destroySource();

hmm, so I guess that something like:

GMainLoopSource& source = GMainLoopSource::createAndDeleteOnDestroy();
source.schedule();
source.cancel();

would leak the source, no? I agree this is not the normal use of a delete on destroy source, but this case is already covered by current code. 
It seems also that a delete on destroy repeating source that is cancelled between two iterations is leaked too, because the object is only deleted in the callbacks.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:124
> +    m_context = {
> +        adoptGRef(g_idle_source_new()),
> +        adoptGRef(g_cancellable_new()),
> +        nullptr, // socketCancellable
> +        WTF::move(function),
> +        nullptr, // boolCallback
> +        nullptr, // socketCallback
> +        WTF::move(destroyFunction)
> +    };

Is there any reason for this change? or does it just look better for you than the individual assignments? I'm just curious.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:152
> +    // Do not allow scheduling this type of callback on delete-on-destroy GMainLoopSources.
> +    ASSERT(m_deleteOnDestroy == DoNotDeleteOnDestroy);

Why was this? IIRC what is not allowed is re-scheduling a delete on destroy source, because cancelling the current source would destroy the object, and if the source finished the object has already been destroyed.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:159
> +        adoptGRef(socketCancellable),

Why do you need a local variable for this one?

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:278
> +        context.destroySource();

I don't think we want to destroy the source here, since it will be destroyed already when this callback returns, what we want is the destroy callback to be called and the object to be deleted in case of delete on destroy sources. That's why the current destroy() method doesn't call g_source_destroy(), when we need that (when cancelling a source) we destroy the source before calling destroy().

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:290
> +    context.destroySource();

Same here. I agree previous code was not correct either, because in case of a re-schedule we were not calling the destroy callback.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:308
> +        ASSERT(m_status == Scheduled);
> +        m_status = Dispatched;

I don't think this is correct, the status can be Dispatched at this point, since this is a repeating source.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:323
> +        m_status = Ready;

This should be ready only when retval is Stop

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:329
> +            m_status = Scheduled;

A repeating source returning Continue is in Dispatched status all that time, it should only transition to Scheduled from ready state when it's re-scheduled.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:406
> +    g_cancellable_cancel(socketCancellable.get());

It's a bit weird that this is called here, I think this should only be called by cancel()

[Zan Dobersek]

Comment on attachment 237846 [details]
Patch

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

I'll update the patch tomorrow, along with the build fixes for GCC 4.9.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:88
>> +    g_cancellable_cancel(m_cancellable.get());
> 
> We should also cancel the socket cancellable here I guess.

OK.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:95
>> +    context.destroySource();
> 
> hmm, so I guess that something like:
> 
> GMainLoopSource& source = GMainLoopSource::createAndDeleteOnDestroy();
> source.schedule();
> source.cancel();
> 
> would leak the source, no? I agree this is not the normal use of a delete on destroy source, but this case is already covered by current code. 
> It seems also that a delete on destroy repeating source that is cancelled between two iterations is leaked too, because the object is only deleted in the callbacks.

Yeah, correct.

The main problem here is how to delete the GMainLoopSource. The mutex is already locked at this point, and a complete lockdown would occur if you enter the destructor from this point.

As you said, cancelling the delete-on-destroy source shouldn't generally be done, so I added an assertion at the top of this method that should prevent that. The only allowable situation for delete-on-destroy sources is calling cancel() with no callbacks scheduled

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:124
>> +    };
> 
> Is there any reason for this change? or does it just look better for you than the individual assignments? I'm just curious.

It constructs the object in one expression, making it clearer overall.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:152
>> +    ASSERT(m_deleteOnDestroy == DoNotDeleteOnDestroy);
> 
> Why was this? IIRC what is not allowed is re-scheduling a delete on destroy source, because cancelling the current source would destroy the object, and if the source finished the object has already been destroyed.

There's no such scheduling in the current code, and it would be a pain to test it properly, as with other delete-on-destroy sources.

I'll remove this assertion for now and it can be discussed later.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:159
>> +        adoptGRef(socketCancellable),
> 
> Why do you need a local variable for this one?

Because it's also passed to g_socket_create_source().

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:278
>> +        context.destroySource();
> 
> I don't think we want to destroy the source here, since it will be destroyed already when this callback returns, what we want is the destroy callback to be called and the object to be deleted in case of delete on destroy sources. That's why the current destroy() method doesn't call g_source_destroy(), when we need that (when cancelling a source) we destroy the source before calling destroy().

The docs say it's safe to call g_source_destroy() on a GSource multiple times, so I took advantage of that and packed everything that Context should take care of at destruction-time in ::destroySource().

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:308
>> +        m_status = Dispatched;
> 
> I don't think this is correct, the status can be Dispatched at this point, since this is a repeating source.

I see, this would break the ::isDispatched() method. OTOH, not sure if I grepped precisely enough but I don't see that method used anywhere.

I did put some assertions in cancelWithoutLocking() that rely on differentiating between Dispatched and Scheduled during the repeatable dispatch.

I'd rather see isDispatched() removed, if possible. Unless there's something else relying on keeping the status as Dispatched throughout the dispatch of repeatable callbacks.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:323
>> +        m_status = Ready;
> 
> This should be ready only when retval is Stop

That's applied in the if statement below.

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:406
>> +    g_cancellable_cancel(socketCancellable.get());
> 
> It's a bit weird that this is called here, I think this should only be called by cancel()

OK.

[Zan Dobersek]

Created attachment 237936 [details]
Patch

Fixed the build, removed the assertion in the GIOCondition callback, moved the cancellation of socketCancellable into ::cancelWithoutLocking().

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:97:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:98:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:99:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:100:  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]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:191:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:211:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:262:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:318:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:389:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:393:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:470:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 12 in 6 files


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

[Carlos Garcia Campos]

(In reply to comment #37)
> (From update of attachment 237846 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237846&action=review
>
> >> Source/WTF/wtf/gobject/GMainLoopSource.cpp:308
> >> +        m_status = Dispatched;
> > 
> > I don't think this is correct, the status can be Dispatched at this point, since this is a repeating source.
> 
> I see, this would break the ::isDispatched() method. OTOH, not sure if I grepped precisely enough but I don't see that method used anywhere.

Because isDispatched doesn't exist. A source is dispatched once the callback is called, and in case of repeating sources, it's considered dispatched all the time, until it's destroyed. 

> I did put some assertions in cancelWithoutLocking() that rely on differentiating between Dispatched and Scheduled during the repeatable dispatch.

I'm not sure I understand those assertions . . . 

> I'd rather see isDispatched() removed, if possible. Unless there's something else relying on keeping the status as Dispatched throughout the dispatch of repeatable callbacks.

Are you talking about isDispatched() or isScheduled() because isDispatched() doesn't exist and is Scheduled() is very important.

[Carlos Garcia Campos]

Ah, I guess you mean isActive(), no? That's used by GST code to check if the seeking source is active. But we could use isScheduled in that case because, since the seek sources are non repeating ones.

[Carlos Garcia Campos]

Comment on attachment 237936 [details]
Patch

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

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:305
> +        ASSERT(m_status == Scheduled);

I still think this could be Dispatched, we shouldn't change the status of a repeating source while it's active.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:327
> +            m_status = Scheduled;

This should still be Dispatched, I don't think we should change the status, the source is only dispatched when schedule methods are called. We usually check whether a sources is dispatched to know whether the callback has been called already or not

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:353
> +        m_status = Dispatched;

Ditto.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:372
> +        m_status = Ready;

This should only be ready when retval is False.

[Zan Dobersek]

(In reply to comment #41)
> Ah, I guess you mean isActive(), no? That's used by GST code to check if the seeking source is active. But we could use isScheduled in that case because, since the seek sources are non repeating ones.

Can't remember anymore what I was looking at, but neither isActive() or isScheduled() can be removed. isDispatched() doesn't exist, so that's why grepping for it returned nothing ...

[Zan Dobersek]

Created attachment 238298 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:97:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:98:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:99:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:100:  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]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:191:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:211:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:262:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:318:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:389:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:393:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:470:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 12 in 7 files


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

[Zan Dobersek]

Comment on attachment 238298 [details]
Patch

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

Dispatched -> Dispatching. Makes it a bit easier to understand when the callbacks in question are dispatched continuously, and not just once.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:336
> +        if (retval && !m_context.source)
> +            m_context = WTF::move(context);
> +        else if (!retval)
> +            m_status = Ready;

Only sets m_status back to Ready if we'll be shutting down the GMainLoopSource. Otherwise m_status remains Dispatching if the callback will continue to be dispatched, or Scheduled if GMainLoopSource was rescheduled during dispatch.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:388
> +        if (retval && !m_context.source)
> +            m_context = WTF::move(context);
> +        else if (!retval)
> +            m_status = Ready;

Same here.

[Carlos Garcia Campos]

(In reply to comment #46)
> (From update of attachment 238298 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238298&action=review
> 
> Dispatched -> Dispatching. Makes it a bit easier to understand when the callbacks in question are dispatched continuously, and not just once.

Ok, make sense to me, I agree the dispatched thing is a bit confusing for repeating sources, because they are actually dispatched multiple times, but what we want to know with isScheduled/isActive is whether the callback has already been called and whether the ongoing source has finished or not.

> > Source/WTF/wtf/gobject/GMainLoopSource.cpp:336
> > +        if (retval && !m_context.source)
> > +            m_context = WTF::move(context);
> > +        else if (!retval)
> > +            m_status = Ready;
> 
> Only sets m_status back to Ready if we'll be shutting down the GMainLoopSource. Otherwise m_status remains Dispatching if the callback will continue to be dispatched, or Scheduled if GMainLoopSource was rescheduled during dispatch.

That's correct, and the reason why ready was only set in the reset method.

> > Source/WTF/wtf/gobject/GMainLoopSource.cpp:388
> > +        if (retval && !m_context.source)
> > +            m_context = WTF::move(context);
> > +        else if (!retval)
> > +            m_status = Ready;
> 
> Same here.

Right.

[Carlos Garcia Campos]

Comment on attachment 238298 [details]
Patch

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

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:85
> +    // Delete-on-destroy GMainLoopSource objects can only be cancelled when there's callback either scheduled
> +    // or in the middle of dispatch. At that point cancellation will have no effect.
> +    ASSERT(m_deleteOnDestroy != DeleteOnDestroy || (m_status == Ready && !m_context.source));

When a callback is scheduled and before dispatching, m_context.source is a valid pointer, so that cancel destroys the source before the callback is called, but the object is leaked. If cancel is called for a repeating source between two callback invocations, the same happens. That's why in current code cancel() deletes the object.

> Source/cmake/OptionsGTK.cmake:374
> -    "${COMMAND_LINE_TO_BUILD} $@"
> +    "${COMMAND_LINE_TO_BUILD} -- $@"

This looks unrelated.

[Zan Dobersek]

Comment on attachment 238298 [details]
Patch

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

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:85
>> +    ASSERT(m_deleteOnDestroy != DeleteOnDestroy || (m_status == Ready && !m_context.source));
> 
> When a callback is scheduled and before dispatching, m_context.source is a valid pointer, so that cancel destroys the source before the callback is called, but the object is leaked. If cancel is called for a repeating source between two callback invocations, the same happens. That's why in current code cancel() deletes the object.

But this only affects delete-on-destroy GMainLoopSources and those should never be cancelled before or during dispatch, simply because you're not supposed to take the reference to the heap-allocated object and interact with it later, like this:

    GMainLoopSource& source = GMainLoopSource::createAndDeleteOnDestroy();
    source.schedule(...);
    ...
    source.cancel();

You can, but you're not supposed to. There might be some way of preventing this either at run-time with an assertion or ideally at compile-time. No clear idea about this at the moment, though.


This assertion only allows cancellation on a delete-on-destroy GMainLoopSource if there wasn't yet a callback scheduled on it:

    GMainLoopSource& source = GMainLoopSource::createAndDeleteOnDestroy();
    source.cancel();

The assertion also allows the cancel() invocation from the destructor, at which point the callback was both scheduled and dispatched so m_status is Ready and m_context.source is null.

[Zan Dobersek]

Created attachment 238304 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:97:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:98:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:99:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:100:  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]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:191:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:211:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:262:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:318:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:389:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:393:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/gobject/GMainLoopSource.cpp:470:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 12 in 6 files


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

[Carlos Garcia Campos]

(In reply to comment #49)
> (From update of attachment 238298 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238298&action=review
> 
> >> Source/WTF/wtf/gobject/GMainLoopSource.cpp:85
> >> +    ASSERT(m_deleteOnDestroy != DeleteOnDestroy || (m_status == Ready && !m_context.source));
> > 
> > When a callback is scheduled and before dispatching, m_context.source is a valid pointer, so that cancel destroys the source before the callback is called, but the object is leaked. If cancel is called for a repeating source between two callback invocations, the same happens. That's why in current code cancel() deletes the object.
> 
> But this only affects delete-on-destroy GMainLoopSources and those should never be cancelled before or during dispatch, simply because you're not supposed to take the reference to the heap-allocated object and interact with it later, like this:
> 
>     GMainLoopSource& source = GMainLoopSource::createAndDeleteOnDestroy();
>     source.schedule(...);
>     ...
>     source.cancel();
> 
> You can, but you're not supposed to. There might be some way of preventing this either at run-time with an assertion or ideally at compile-time. No clear idea about this at the moment, though.
> 
> 
> This assertion only allows cancellation on a delete-on-destroy GMainLoopSource if there wasn't yet a callback scheduled on it:
> 
>     GMainLoopSource& source = GMainLoopSource::createAndDeleteOnDestroy();
>     source.cancel();
> 
> The assertion also allows the cancel() invocation from the destructor, at which point the callback was both scheduled and dispatched so m_status is Ready and m_context.source is null.

Since schedule is then the only allowed method on a delete on destroy source, what do you think about adding static schedule methods instead? Something like:

GMainLoopSource::scheduleAndDeleteOnDestroy() or even just GMainLoopSource::schedule() and assume the static members create a temp source that is deleted on destroy.

[Carlos Garcia Campos]

Comment on attachment 238304 [details]
Patch

Ok, let's try again, thanks!

[Zan Dobersek]

Comment on attachment 238304 [details]
Patch

Clearing flags on attachment: 238304

Committed r173720: <http://trac.webkit.org/changeset/173720>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.