Bug 62380 - [EFL] correct sharedTimer value in SharedTimerEfl.cpp
Summary: [EFL] correct sharedTimer value in SharedTimerEfl.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-09 08:30 PDT by Henry Song
Modified: 2011-06-24 12:49 PDT (History)
12 users (show)

See Also:


Attachments
[EFL] correct _sharedTimer value in timer function in SharedTimerEfl.cpp (136.47 KB, patch)
2011-06-09 08:47 PDT, Henry Song
no flags Details | Formatted Diff | Diff
[EFL] correct _sharedTimer value in timer function in SharedTimerEfl.cpp (136.47 KB, patch)
2011-06-09 08:59 PDT, Henry Song
lucas.de.marchi: review-
lucas.de.marchi: commit-queue-
Details | Formatted Diff | Diff
[EFL] correct _sharedTimer value in timer function in SharedTimerEfl.cpp (981 bytes, patch)
2011-06-09 10:13 PDT, Henry Song
no flags Details | Formatted Diff | Diff
remove _sharedTimer = 0 in timerEvent() (1.26 KB, patch)
2011-06-09 14:27 PDT, Henry Song
no flags Details | Formatted Diff | Diff
remove _sharedTimer = 0 in timerEvent() (1.27 KB, patch)
2011-06-09 14:36 PDT, Henry Song
lucas.de.marchi: review-
lucas.de.marchi: commit-queue-
Details | Formatted Diff | Diff
remove _sharedTimer = 0 in timerEvent() (1.32 KB, patch)
2011-06-09 15:15 PDT, Henry Song
no flags Details | Formatted Diff | Diff
remove _sharedTimer = 0 in timerEvent() (2.07 KB, patch)
2011-06-09 18:16 PDT, Henry Song
no flags Details | Formatted Diff | Diff
correct _sharedTimer value in SharedTimerEfl.cpp (2.00 KB, patch)
2011-06-13 11:57 PDT, Henry Song
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Henry Song 2011-06-09 08:30:26 PDT
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.
Comment 1 Henry Song 2011-06-09 08:47:17 PDT
Created attachment 96593 [details]
[EFL] correct _sharedTimer value in timer function in SharedTimerEfl.cpp
Comment 2 Henry Song 2011-06-09 08:59:04 PDT
Created attachment 96594 [details]
[EFL] correct _sharedTimer value in timer function in SharedTimerEfl.cpp
Comment 3 Lucas De Marchi 2011-06-09 09:55:10 PDT
Comment on attachment 96594 [details]
[EFL] correct _sharedTimer value in timer function in SharedTimerEfl.cpp

You mismerged the changelog.
Comment 4 Henry Song 2011-06-09 10:13:25 PDT
Created attachment 96600 [details]
[EFL] correct _sharedTimer value in timer function in SharedTimerEfl.cpp
Comment 5 Lucas De Marchi 2011-06-09 10:43:08 PDT
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.
Comment 6 Leandro Pereira 2011-06-09 10:47:48 PDT
(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).
Comment 7 Leandro Pereira 2011-06-09 10:49:08 PDT
(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.
Comment 8 Henry Song 2011-06-09 14:27:55 PDT
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.
Comment 9 WebKit Review Bot 2011-06-09 14:29:22 PDT
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.
Comment 10 Henry Song 2011-06-09 14:36:57 PDT
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.
Comment 11 Lucas De Marchi 2011-06-09 15:09:30 PDT
(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.
Comment 12 Henry Song 2011-06-09 15:15:01 PDT
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.
Comment 13 Lucas De Marchi 2011-06-09 15:46:29 PDT
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.
Comment 14 Gyuyoung Kim 2011-06-09 17:00:05 PDT
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
Comment 15 Henry Song 2011-06-09 18:16:36 PDT
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.
Comment 16 Lucas De Marchi 2011-06-10 06:11:59 PDT
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.
Comment 17 Henry Song 2011-06-10 07:45:12 PDT
Yes, that covers all cases. Thanks
Comment 18 Henry Song 2011-06-13 11:57:22 PDT
Created attachment 96985 [details]
correct _sharedTimer value in SharedTimerEfl.cpp
Comment 19 Lucas De Marchi 2011-06-14 16:33:02 PDT
Comment on attachment 96985 [details]
correct _sharedTimer value in SharedTimerEfl.cpp

Informal r+
Comment 20 Ryuan Choi 2011-06-15 06:10:52 PDT
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.
Comment 21 Martin Robinson 2011-06-24 07:20:09 PDT
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?
Comment 22 Lucas De Marchi 2011-06-24 09:28:29 PDT
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.
Comment 23 Martin Robinson 2011-06-24 09:58:52 PDT
(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.
Comment 24 Henry Song 2011-06-24 10:17:13 PDT
_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.
Comment 25 WebKit Review Bot 2011-06-24 12:49:40 PDT
Comment on attachment 96985 [details]
correct _sharedTimer value in SharedTimerEfl.cpp

Clearing flags on attachment: 96985

Committed r89697: <http://trac.webkit.org/changeset/89697>
Comment 26 WebKit Review Bot 2011-06-24 12:49:47 PDT
All reviewed patches have been landed.  Closing bug.