[EFL] correct sharedTimer value in SharedTimerEfl.cpp

[Henry Song]

Correct _sharedTimer value after timerFunction() called in SharedTimerEfl.cpp.  Existing implementation set _sharedTimer = 0.  Because it is set to 0, when stopSharedTimer() is called, ecore_timer_del() will not actually delete the timer.  Thus, the "deleted" timer will be called again in next loop.

[Henry Song]

Created attachment 96593 [details]
[EFL] correct _sharedTimer value in timer function in SharedTimerEfl.cpp

[Henry Song]

Created attachment 96594 [details]
[EFL] correct _sharedTimer value in timer function in SharedTimerEfl.cpp

[Lucas De Marchi]

Comment on attachment 96594 [details]
[EFL] correct _sharedTimer value in timer function in SharedTimerEfl.cpp

You mismerged the changelog.

[Henry Song]

Created attachment 96600 [details]
[EFL] correct _sharedTimer value in timer function in SharedTimerEfl.cpp

[Lucas De Marchi]

Comment on attachment 96600 [details]
[EFL] correct _sharedTimer value in timer function in SharedTimerEfl.cpp

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

It seems like the wrong fix. timerEvent() returns ECORE_CALLBACK_CANCEL when it sets _sharedTimer. If you do not set it to 0, later on when stopSharedTimer() is called, it will remove a non-existent timer.

> Source/WebCore/ChangeLog.new:6
> +        [EFL] correct _sharedTimer value after in timer function
> +		htts://bugs/webkit.org/show_bug.cgi?id=62380

Extra space here.

[Leandro Pereira]

(In reply to comment #5)
> 
> > Source/WebCore/ChangeLog.new:6
> > +        [EFL] correct _sharedTimer value after in timer function
> > +		htts://bugs/webkit.org/show_bug.cgi?id=62380
> 
> Extra space here.

URL is also wrong. Please use webkit-patch utility to format your patch, it should take care of this (and other things as well).

[Leandro Pereira]

(In reply to comment #4)
> Created an attachment (id=96600) [details]
> [EFL] correct _sharedTimer value in timer function in SharedTimerEfl.cpp

Please mark other patches as obsolete as soon as you upload one that supersedes it. This will ensure it won't be picked up by slow queues in vain.

[Henry Song]

Created attachment 96645 [details]
remove _sharedTimer = 0 in timerEvent()

Both stopSharedTimer() and addNewTimer() can be called within timerFunction() in timerEvent().  If addNewTimer() is called, the static _sharedTimer object is replaced with new timer object.  If set it to 0 after timerFunction() returns, the new timer object is lost and cannot be deleted in later stopSharedTimer().  This is similar to how it is implemented in SharedTimerGtk.cpp for GTK port.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 1 files


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

[Henry Song]

Created attachment 96646 [details]
remove _sharedTimer = 0 in timerEvent()

Both stopSharedTimer() and addNewTimer() can be called within timerFunction() in timerEvent().  If addNewTimer() is called, the static _sharedTimer object is replaced with new timer object.  If set it to 0 after timerFunction() returns, the new timer object is lost and cannot be deleted in later stopSharedTimer().  This is similar to how it is implemented in SharedTimerGtk.cpp for GTK port.

[Lucas De Marchi]

(In reply to comment #9)
> Attachment 96645 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> 
> Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
> Total errors found: 1 in 1 files
> 
ChangeLog cannot contain tab character. It's probably a mis-configuration of your editor.

Moreover try not to pass 80 chars in a single line in the ChangeLog. It makes it hard to read.

[Henry Song]

Created attachment 96651 [details]
remove _sharedTimer = 0 in timerEvent()

Both stopSharedTimer() and addNewTimer() can be called within timerFunction() in timerEvent().  If addNewTimer() is called, the static _sharedTimer object is replaced with new timer object.  If set it to 0 after timerFunction() returns, the new timer object is lost and cannot be deleted in later stopSharedTimer().  This is similar to how it is implemented in SharedTimerGtk.cpp for GTK port.

[Lucas De Marchi]

Comment on attachment 96646 [details]
remove _sharedTimer = 0 in timerEvent()

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

> Source/WebCore/ChangeLog:6
> +        SharedTimerEfl.cpp set _sharedTimer = 0 is incorrect in timerEvent, stopSharedTimer can be call by both stopSharedTimer() or by addNewTimer() withint timerFunction(). When addNewTimer(), the global _sharedTimer is replaced by the new timer object. Setting it to 0 loses the new timer object within ecore_timer without deleting it.
> +        https://bugs.webkit.org/show_bug.cgi?id=62380

Sorry, I didn't understand what you mean by "stopSharedTimer can be call by both stopSharedTimer() or by addNewTimer() withint timerFunction()".

Reading this file, what I understand is:

 * If WebCore added a new timer by calling WebCore::addNewTimer(), the old timer will be deleted by calling WebCore::stopSharedTimer(), which in turn deletes the timer inside ecore calling ecore_timer_del()

 * If the timer is triggered before WebCore calls WebCore::stopSharedTimer(), then the registered function is called and the timer is deleted inside ecore by returning ECORE_CALLBACK_CANCEL


The only reason this might be wrong (and then your patch partially right) is if the timer is supposed to keep triggering until the WebCore::stopSharedTimer() is called. In that case you have to fix the return value too, which should be EINA_TRUE.

[Gyuyoung Kim]

Comment on attachment 96651 [details]
remove _sharedTimer = 0 in timerEvent()

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

> Source/WebCore/ChangeLog:4
> +

Add bug title/url to here.

[EFL] correct sharedTimer value in SharedTimerEfl.cpp
https://bugs.webkit.org/show_bug.cgi?id=62380

[Henry Song]

Created attachment 96682 [details]
remove _sharedTimer = 0 in timerEvent()

SharedTimerEfl.cpp set _sharedTimer = 0 is incorrect in timerEvent(), we have observed addNewTimer() can be triggered by either inside timerFunction() or others that are not within timerFunction().  We have observed the following case:
        1. add a new Timer within timerFunction()
        2. timerFunction returns, set _sharedTimer = 0
        3. addNewTimer() is triggered again within webkit, don't know
           it triggers this.
        4. because at this moment, _sharedTimer == 0, the previous
           registered timer (in step 1) did not get removed.
        5. Now in ecore_timer, there are two timers with same callbacks
        6. From now on, timerFunction() will be called twice back-to-back
           in each ecore_timer loop.
To correct this, we should not set _sharedTimer = 0 after timerFunction() so that when addNewTimer() triggered, it has a valid sharedTimer to delete.  timerEvent(), however, will still return ECORE_CALLBACK_CANCEL to allow ecore_timer to remove current timer.

[Lucas De Marchi]

Comment on attachment 96682 [details]
remove _sharedTimer = 0 in timerEvent()

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

> Source/WebCore/ChangeLog:12
> +        SharedTimerEfl.cpp set _sharedTimer = 0 is incorrect in 
> +        timerEvent(), we have observed addNewTimer() can be triggered 
> +        by either inside timerFunction() or others that are not within
> +        timerFunction().  We have observed the following case:
> +        1. add a new Timer within timerFunction()

You are right. If we can setup a new timer from within the callback, this will indeed be bogus.

However your patch creates another bug:

 1. addNewTimer() is called by WebCore;
 2 timerEvent() is triggered by Ecore. With your patch _sharedTimer is not set to 0;
 3. addNewTimer() is called by WebCore;
   -> stopSharedTimer() is called. Since _sharedTimer is not 0, it deletes a non-existent timer


The solution for this problem is setting _sharedTimer to 0 before calling timerFunction, like below:


diff --git a/Source/WebCore/platform/efl/SharedTimerEfl.cpp b/Source/WebCore/platform/efl/SharedTimerEfl.cpp
index 2534c60..a8a6ac6 100644
--- a/Source/WebCore/platform/efl/SharedTimerEfl.cpp
+++ b/Source/WebCore/platform/efl/SharedTimerEfl.cpp
@@ -47,11 +47,11 @@ void setSharedTimerFiredFunction(void (*func)())
 
 static Eina_Bool timerEvent(void*)
 {
+    _sharedTimer = 0;
+
     if (_timerFunction)
         _timerFunction();
 
-    _sharedTimer = 0;
-
     return ECORE_CALLBACK_CANCEL;
 }
 


Is that ok? I think this covers all the use-cases.

[Henry Song]

Yes, that covers all cases. Thanks

[Henry Song]

Created attachment 96985 [details]
correct _sharedTimer value in SharedTimerEfl.cpp

[Lucas De Marchi]

Comment on attachment 96985 [details]
correct _sharedTimer value in SharedTimerEfl.cpp

Informal r+

[Ryuan Choi]

could you review this?

This will fix sluggish animation in jsgamebench, one of benchmark tools, which try to add timer frequently and repeatedly using javascript.
Thanks in advance.

[Martin Robinson]

Comment on attachment 96985 [details]
correct _sharedTimer value in SharedTimerEfl.cpp

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

> Source/WebCore/platform/efl/SharedTimerEfl.cpp:-53
> +    _sharedTimer = 0;
>      if (_timerFunction)
>          _timerFunction();
>  
> -    _sharedTimer = 0;

If you set stopSharedTimer to zero here when does ecore_timer_del ever happen on the old timer? Don't you risk "leaking" a timer?

[Lucas De Marchi]

Comment on attachment 96985 [details]
correct _sharedTimer value in SharedTimerEfl.cpp

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

>> Source/WebCore/platform/efl/SharedTimerEfl.cpp:-53
>> -    _sharedTimer = 0;
> 
> If you set stopSharedTimer to zero here when does ecore_timer_del ever happen on the old timer? Don't you risk "leaking" a timer?

What do you mean by "set stopSharedTimer to zero"? It's function, not a pointer.

[Martin Robinson]

(In reply to comment #22)
> (From update of attachment 96985 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96985&action=review
> 
> >> Source/WebCore/platform/efl/SharedTimerEfl.cpp:-53
> >> -    _sharedTimer = 0;
> > 
> > If you set stopSharedTimer to zero here when does ecore_timer_del ever happen on the old timer? Don't you risk "leaking" a timer?
> 
> What do you mean by "set stopSharedTimer to zero"? It's function, not a pointer.

Sorry, I meant _sharedTimer.

[Henry Song]

_sharedTimer is set to zero only in timeEvent() callback.  The current _sharedTimer within timeEvent() is removed regardless it is set zero to 0 or not.  We set it to zero before _timerFunction() is called.  This is because within _timerFunction(), it is possible to add new timer via addNewTimer(), in which case, _sharedTimer value is changed to new timer.  This patch actually fixes timer leaking.

[WebKit Review Bot]

Comment on attachment 96985 [details]
correct _sharedTimer value in SharedTimerEfl.cpp

Clearing flags on attachment: 96985

Committed r89697: <http://trac.webkit.org/changeset/89697>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.