[Blink-WebWorkers] WorkerSharedTimer cancels extra delayed tasks.

[Blink-WebWorkers] WorkerSharedTimer cancels extra delayed tasks.

This CL has the Blink side changes to cancel extra delayed tasks posted
in WorkerSharedTimer. Previously extra tasks were being posted and allowed
to complete if multiple interval timers were scheduled on worker threads.
Now we will cancel any extra delayed task, made redundant due to newly scheduled
lesser timer interval tasks.

BUG=410786
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182930

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183066

[jochen (gone - plz use gerrit), 2014-09-12 14:11:54]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2014-09-12 14:11:55]

Here's how I would do this:

instead of using createSameThreadTask(WorkerSharedTimer::OnTimeout), create a
new class similar to RunDebuggerQueueTask. that class has a bool m_cancelled,
and a method Cancel() that sets this to true.

If m_cancel is True in its performTask(), just do nothing, otherwise invoke the
idle handler

Now you just have to store a pointer to this class, and invoke Cancel on it when
you want to cancel the tasks. The class itself can clear the pointer once it
executed (m_cancel wasn't false)

then you also don't need a weak pointer.

https://codereview.chromium.org/563203002/diff/40001/public/platform/WebThread.h
File public/platform/WebThread.h (right):

https://codereview.chromium.org/563203002/diff/40001/public/platform/WebThrea...
public/platform/WebThread.h:29: #include "base/memory/weak_ptr.h"
you can't use base from blink, there is wtf/WeakPtr.h

however, i don't think you need a weak pointer here at all

[Mayur Kankanwadi, 2014-09-12 14:21:24]

Ok, so instead of going deep till the level of message_loop, we are just letting
the task to be queued in the thread message loop.
And we do the 'cancel'ing at the WorkerThreadTask level.
This was always an option, but I thought doing it at message_loop level was
going to be better. But was equally worried with the weak_ptr, as the ownership
would not be taken by the message loop and then we would leak the tasks.
I will implement this cancel at the WorkerThreadTask level and would put another
patch.
Thanks.


On 2014/09/12 14:11:55, jochen wrote:
> Here's how I would do this:
> 
> instead of using createSameThreadTask(WorkerSharedTimer::OnTimeout), create a
> new class similar to RunDebuggerQueueTask. that class has a bool m_cancelled,
> and a method Cancel() that sets this to true.
> 
> If m_cancel is True in its performTask(), just do nothing, otherwise invoke
the
> idle handler
> 
> Now you just have to store a pointer to this class, and invoke Cancel on it
when
> you want to cancel the tasks. The class itself can clear the pointer once it
> executed (m_cancel wasn't false)
> 
> then you also don't need a weak pointer.
> 
>
https://codereview.chromium.org/563203002/diff/40001/public/platform/WebThread.h
> File public/platform/WebThread.h (right):
> 
>
https://codereview.chromium.org/563203002/diff/40001/public/platform/WebThrea...
> public/platform/WebThread.h:29: #include "base/memory/weak_ptr.h"
> you can't use base from blink, there is wtf/WeakPtr.h
> 
> however, i don't think you need a weak pointer here at all

[Mayur Kankanwadi, 2014-09-12 14:36:41]

jochen: I could just add the bool cancelled to WorkerThreadTask itself. 
Do the check WorkerThreadTask::run and return back without doing anything if the
bool is set to true.
Any specific reason to create a new class similar to RunDebuggerQueueTask?

On 2014/09/12 14:21:24, Mayur Kankanwadi wrote:
> Ok, so instead of going deep till the level of message_loop, we are just
letting
> the task to be queued in the thread message loop.
> And we do the 'cancel'ing at the WorkerThreadTask level.
> This was always an option, but I thought doing it at message_loop level was
> going to be better. But was equally worried with the weak_ptr, as the
ownership
> would not be taken by the message loop and then we would leak the tasks.
> I will implement this cancel at the WorkerThreadTask level and would put
another
> patch.
> Thanks.
> 
> 
> On 2014/09/12 14:11:55, jochen wrote:
> > Here's how I would do this:
> > 
> > instead of using createSameThreadTask(WorkerSharedTimer::OnTimeout), create
a
> > new class similar to RunDebuggerQueueTask. that class has a bool
m_cancelled,
> > and a method Cancel() that sets this to true.
> > 
> > If m_cancel is True in its performTask(), just do nothing, otherwise invoke
> the
> > idle handler
> > 
> > Now you just have to store a pointer to this class, and invoke Cancel on it
> when
> > you want to cancel the tasks. The class itself can clear the pointer once it
> > executed (m_cancel wasn't false)
> > 
> > then you also don't need a weak pointer.
> > 
> >
>
https://codereview.chromium.org/563203002/diff/40001/public/platform/WebThread.h
> > File public/platform/WebThread.h (right):
> > 
> >
>
https://codereview.chromium.org/563203002/diff/40001/public/platform/WebThrea...
> > public/platform/WebThread.h:29: #include "base/memory/weak_ptr.h"
> > you can't use base from blink, there is wtf/WeakPtr.h
> > 
> > however, i don't think you need a weak pointer here at all

[Mayur Kankanwadi, 2014-09-12 14:52:25]

Got it. :) On to creating a new class.
On 2014/09/12 14:36:41, Mayur Kankanwadi wrote:
> jochen: I could just add the bool cancelled to WorkerThreadTask itself. 
> Do the check WorkerThreadTask::run and return back without doing anything if
the
> bool is set to true.
> Any specific reason to create a new class similar to RunDebuggerQueueTask?
> 
> On 2014/09/12 14:21:24, Mayur Kankanwadi wrote:
> > Ok, so instead of going deep till the level of message_loop, we are just
> letting
> > the task to be queued in the thread message loop.
> > And we do the 'cancel'ing at the WorkerThreadTask level.
> > This was always an option, but I thought doing it at message_loop level was
> > going to be better. But was equally worried with the weak_ptr, as the
> ownership
> > would not be taken by the message loop and then we would leak the tasks.
> > I will implement this cancel at the WorkerThreadTask level and would put
> another
> > patch.
> > Thanks.
> > 
> > 
> > On 2014/09/12 14:11:55, jochen wrote:
> > > Here's how I would do this:
> > > 
> > > instead of using createSameThreadTask(WorkerSharedTimer::OnTimeout),
create
> a
> > > new class similar to RunDebuggerQueueTask. that class has a bool
> m_cancelled,
> > > and a method Cancel() that sets this to true.
> > > 
> > > If m_cancel is True in its performTask(), just do nothing, otherwise
invoke
> > the
> > > idle handler
> > > 
> > > Now you just have to store a pointer to this class, and invoke Cancel on
it
> > when
> > > you want to cancel the tasks. The class itself can clear the pointer once
it
> > > executed (m_cancel wasn't false)
> > > 
> > > then you also don't need a weak pointer.
> > > 
> > >
> >
>
https://codereview.chromium.org/563203002/diff/40001/public/platform/WebThread.h
> > > File public/platform/WebThread.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/563203002/diff/40001/public/platform/WebThrea...
> > > public/platform/WebThread.h:29: #include "base/memory/weak_ptr.h"
> > > you can't use base from blink, there is wtf/WeakPtr.h
> > > 
> > > however, i don't think you need a weak pointer here at all

[Mayur Kankanwadi, 2014-09-15 09:02:05]

Hi jochen,
Please review.
--Mayur Kankanwadi.

On 2014/09/12 14:52:25, Mayur Kankanwadi wrote:
> Got it. :) On to creating a new class.
> On 2014/09/12 14:36:41, Mayur Kankanwadi wrote:
> > jochen: I could just add the bool cancelled to WorkerThreadTask itself. 
> > Do the check WorkerThreadTask::run and return back without doing anything if
> the
> > bool is set to true.
> > Any specific reason to create a new class similar to RunDebuggerQueueTask?
> > 
> > On 2014/09/12 14:21:24, Mayur Kankanwadi wrote:
> > > Ok, so instead of going deep till the level of message_loop, we are just
> > letting
> > > the task to be queued in the thread message loop.
> > > And we do the 'cancel'ing at the WorkerThreadTask level.
> > > This was always an option, but I thought doing it at message_loop level
was
> > > going to be better. But was equally worried with the weak_ptr, as the
> > ownership
> > > would not be taken by the message loop and then we would leak the tasks.
> > > I will implement this cancel at the WorkerThreadTask level and would put
> > another
> > > patch.
> > > Thanks.
> > > 
> > > 
> > > On 2014/09/12 14:11:55, jochen wrote:
> > > > Here's how I would do this:
> > > > 
> > > > instead of using createSameThreadTask(WorkerSharedTimer::OnTimeout),
> create
> > a
> > > > new class similar to RunDebuggerQueueTask. that class has a bool
> > m_cancelled,
> > > > and a method Cancel() that sets this to true.
> > > > 
> > > > If m_cancel is True in its performTask(), just do nothing, otherwise
> invoke
> > > the
> > > > idle handler
> > > > 
> > > > Now you just have to store a pointer to this class, and invoke Cancel on
> it
> > > when
> > > > you want to cancel the tasks. The class itself can clear the pointer
once
> it
> > > > executed (m_cancel wasn't false)
> > > > 
> > > > then you also don't need a weak pointer.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/563203002/diff/40001/public/platform/WebThread.h
> > > > File public/platform/WebThread.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/563203002/diff/40001/public/platform/WebThrea...
> > > > public/platform/WebThread.h:29: #include "base/memory/weak_ptr.h"
> > > > you can't use base from blink, there is wtf/WeakPtr.h
> > > > 
> > > > however, i don't think you need a weak pointer here at all

[jochen (gone - plz use gerrit), 2014-09-15 14:29:42]

https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:88: class WorkerThreadTask : public
blink::WebThread::Task {
why do you move this class up here?

https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:131: class WorkerThreadCancelableTask FINAL
: public ExecutionContextTask {
add wtf_make_noncopyable and fast allocated macros

https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:229: static WorkerThreadCancelableTask*
m_lastQueuedTask;
why static? there can be several worker threads

[Mayur Kankanwadi, 2014-09-15 14:38:27]

https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:88: class WorkerThreadTask : public
blink::WebThread::Task {
On 2014/09/15 14:29:41, jochen wrote:
> why do you move this class up here?
Was done to fix earlier compilation errors. Fixed to the right place now.
Acknowledged.

https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:131: class WorkerThreadCancelableTask FINAL
: public ExecutionContextTask {
On 2014/09/15 14:29:42, jochen wrote:
> add wtf_make_noncopyable and fast allocated macros

Done.

https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:229: static WorkerThreadCancelableTask*
m_lastQueuedTask;
On 2014/09/15 14:29:42, jochen wrote:
> why static? there can be several worker threads
Yes, it's unnecessary.
Acknowledged.

[Mayur Kankanwadi, 2014-09-16 09:32:33]

Hi jochen,
Updated & rebased patch with your comments.
Please review.
Regards,
--Mayur Kankanwadi.

On 2014/09/15 14:38:27, Mayur Kankanwadi wrote:
>
https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
> File Source/core/workers/WorkerThread.cpp (right):
> 
>
https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
> Source/core/workers/WorkerThread.cpp:88: class WorkerThreadTask : public
> blink::WebThread::Task {
> On 2014/09/15 14:29:41, jochen wrote:
> > why do you move this class up here?
> Was done to fix earlier compilation errors. Fixed to the right place now.
> Acknowledged.
> 
>
https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
> Source/core/workers/WorkerThread.cpp:131: class WorkerThreadCancelableTask
FINAL
> : public ExecutionContextTask {
> On 2014/09/15 14:29:42, jochen wrote:
> > add wtf_make_noncopyable and fast allocated macros
> 
> Done.
> 
>
https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
> Source/core/workers/WorkerThread.cpp:229: static WorkerThreadCancelableTask*
> m_lastQueuedTask;
> On 2014/09/15 14:29:42, jochen wrote:
> > why static? there can be several worker threads
> Yes, it's unnecessary.
> Acknowledged.

[Mayur Kankanwadi, 2014-09-16 13:38:32]

Hi jochen,
The bots are failing in webkit_tests.
I ran the full suite as well as the two individual failing tests mentioned in
the report, but none are failing here.
How to go about it?
Regards,
Mayur Kankanwadi.

On 2014/09/16 09:32:33, Mayur Kankanwadi wrote:
> Hi jochen,
> Updated & rebased patch with your comments.
> Please review.
> Regards,
> --Mayur Kankanwadi.
> 
> On 2014/09/15 14:38:27, Mayur Kankanwadi wrote:
> >
>
https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
> > File Source/core/workers/WorkerThread.cpp (right):
> > 
> >
>
https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
> > Source/core/workers/WorkerThread.cpp:88: class WorkerThreadTask : public
> > blink::WebThread::Task {
> > On 2014/09/15 14:29:41, jochen wrote:
> > > why do you move this class up here?
> > Was done to fix earlier compilation errors. Fixed to the right place now.
> > Acknowledged.
> > 
> >
>
https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
> > Source/core/workers/WorkerThread.cpp:131: class WorkerThreadCancelableTask
> FINAL
> > : public ExecutionContextTask {
> > On 2014/09/15 14:29:42, jochen wrote:
> > > add wtf_make_noncopyable and fast allocated macros
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
> > Source/core/workers/WorkerThread.cpp:229: static WorkerThreadCancelableTask*
> > m_lastQueuedTask;
> > On 2014/09/15 14:29:42, jochen wrote:
> > > why static? there can be several worker threads
> > Yes, it's unnecessary.
> > Acknowledged.

[Mayur Kankanwadi, 2014-09-16 14:12:38]

Added a fix, this should fix the trybot failures.

On 2014/09/16 13:38:32, Mayur Kankanwadi wrote:
> Hi jochen,
> The bots are failing in webkit_tests.
> I ran the full suite as well as the two individual failing tests mentioned in
> the report, but none are failing here.
> How to go about it?
> Regards,
> Mayur Kankanwadi.
> 
> On 2014/09/16 09:32:33, Mayur Kankanwadi wrote:
> > Hi jochen,
> > Updated & rebased patch with your comments.
> > Please review.
> > Regards,
> > --Mayur Kankanwadi.
> > 
> > On 2014/09/15 14:38:27, Mayur Kankanwadi wrote:
> > >
> >
>
https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
> > > File Source/core/workers/WorkerThread.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
> > > Source/core/workers/WorkerThread.cpp:88: class WorkerThreadTask : public
> > > blink::WebThread::Task {
> > > On 2014/09/15 14:29:41, jochen wrote:
> > > > why do you move this class up here?
> > > Was done to fix earlier compilation errors. Fixed to the right place now.
> > > Acknowledged.
> > > 
> > >
> >
>
https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
> > > Source/core/workers/WorkerThread.cpp:131: class WorkerThreadCancelableTask
> > FINAL
> > > : public ExecutionContextTask {
> > > On 2014/09/15 14:29:42, jochen wrote:
> > > > add wtf_make_noncopyable and fast allocated macros
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/563203002/diff/60001/Source/core/workers/Work...
> > > Source/core/workers/WorkerThread.cpp:229: static
WorkerThreadCancelableTask*
> > > m_lastQueuedTask;
> > > On 2014/09/15 14:29:42, jochen wrote:
> > > > why static? there can be several worker threads
> > > Yes, it's unnecessary.
> > > Acknowledged.

[jochen (gone - plz use gerrit), 2014-09-16 14:17:05]

the crashes look related to your patch

e.g.
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_dbg/...
has a somewhat useful stack trace.

could you also add the test case you posted on the bug as layout test?

https://codereview.chromium.org/563203002/diff/100001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/563203002/diff/100001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:132: virtual void setFireInterval(double
interval)
assume the following two calls happen:

setFireInterval(100);
setFireInterval(200);

we the won't schedule a new task, however, the existing task is still scheduled
for in 100ms. What's missing is to make the existing task do another
postDelayedTask with the remaining delta.

See e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer.c...
for what the main thread does

[Mayur Kankanwadi, 2014-09-16 14:25:30]

Please check comments inline:
On 2014/09/16 14:17:05, jochen wrote:
> the crashes look related to your patch
> 
> e.g.
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_dbg/...
> has a somewhat useful stack trace.
> 
--Added the relevant fix.

> could you also add the test case you posted on the bug as layout test?
--Yes, will do that.

> 
>
https://codereview.chromium.org/563203002/diff/100001/Source/core/workers/Wor...
> File Source/core/workers/WorkerThread.cpp (right):
> 
>
https://codereview.chromium.org/563203002/diff/100001/Source/core/workers/Wor...
> Source/core/workers/WorkerThread.cpp:132: virtual void setFireInterval(double
> interval)
> assume the following two calls happen:
> 
> setFireInterval(100);
> setFireInterval(200);
> 
> we the won't schedule a new task, however, the existing task is still
scheduled
> for in 100ms. What's missing is to make the existing task do another
> postDelayedTask with the remaining delta.
> 
> See e.g.
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer.c...
> for what the main thread does
-- Do you mean that there are two settimeout calls:
setTimeOut(100);
setTimeOut(200);
After the first 100ms timer has fired, the next fire interval of 100ms comes are
an argument.
AFAIK we do not need to add anymore logic to schedule the next interval of 100ms
to complete the 200ms timeout.
TimerBase calculates the nextTimerInterval based on its timerheap and then sends
us the interval to schedule the delayed event.

[Mayur Kankanwadi, 2014-09-17 14:41:05]

Added layout tests. PTAL.

[jochen (gone - plz use gerrit), 2014-09-17 14:47:40]

https://codereview.chromium.org/563203002/diff/140001/LayoutTests/fast/worker...
File LayoutTests/fast/workers/resources/worker-timeout-order.js (right):

https://codereview.chromium.org/563203002/diff/140001/LayoutTests/fast/worker...
LayoutTests/fast/workers/resources/worker-timeout-order.js:9:
setTimeout(function () { postMessage(1); }, 100);
can you use lower timeouts 5, 10, 15? so the test doesn't take unnecessarily
long

might also be a good idea to split this up into two tests (or three and add the
test I mentioned in the comment)

[jochen (gone - plz use gerrit), 2014-09-17 14:48:40]

On 2014/09/16 at 14:25:30, mayurk.vk wrote:
> Please check comments inline:
> On 2014/09/16 14:17:05, jochen wrote:
> > the crashes look related to your patch
> > 
> > e.g.
> >
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_dbg/...
> > has a somewhat useful stack trace.
> > 
> --Added the relevant fix.
> 
> > could you also add the test case you posted on the bug as layout test?
> --Yes, will do that.
> 
> > 
> >
https://codereview.chromium.org/563203002/diff/100001/Source/core/workers/Wor...
> > File Source/core/workers/WorkerThread.cpp (right):
> > 
> >
https://codereview.chromium.org/563203002/diff/100001/Source/core/workers/Wor...
> > Source/core/workers/WorkerThread.cpp:132: virtual void
setFireInterval(double
> > interval)
> > assume the following two calls happen:
> > 
> > setFireInterval(100);
> > setFireInterval(200);
> > 
> > we the won't schedule a new task, however, the existing task is still
scheduled
> > for in 100ms. What's missing is to make the existing task do another
> > postDelayedTask with the remaining delta.
> > 
> > See e.g.
> >
https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer.c...
> > for what the main thread does
> -- Do you mean that there are two settimeout calls:
> setTimeOut(100);
> setTimeOut(200);
> After the first 100ms timer has fired, the next fire interval of 100ms comes
are an argument.
> AFAIK we do not need to add anymore logic to schedule the next interval of
100ms to complete the 200ms timeout.
> TimerBase calculates the nextTimerInterval based on its timerheap and then
sends us the interval to schedule the delayed event.

what happens if you run

var t = setTimeout(func1, 10);
setTimeout(func2, 20);
clearTimeout(t);

[Mayur Kankanwadi, 2014-09-18 14:34:11]

PTAL.

[Mayur Kankanwadi, 2014-09-18 14:34:25]

https://codereview.chromium.org/563203002/diff/140001/LayoutTests/fast/worker...
File LayoutTests/fast/workers/resources/worker-timeout-order.js (right):

https://codereview.chromium.org/563203002/diff/140001/LayoutTests/fast/worker...
LayoutTests/fast/workers/resources/worker-timeout-order.js:9:
setTimeout(function () { postMessage(1); }, 100);
On 2014/09/17 14:47:39, jochen wrote:
> can you use lower timeouts 5, 10, 15? so the test doesn't take unnecessarily
> long
> 
> might also be a good idea to split this up into two tests (or three and add
the
> test I mentioned in the comment)

Done.

[jochen (gone - plz use gerrit), 2014-09-18 18:38:40]

thx for adding the tests.

did you verify when the timeouts happen?

my concern is that after cancelling the first timeout, the second timeout still
fires at the intervall of the first timeout.

[Mayur Kankanwadi, 2014-09-19 07:19:48]

Hi jochen,

I tweaked the worker-timeout-cancel-order.html layout test and in the
workerthread marked our task cancel explicitly.
Assuming the following sequence for the timeout ops:
t1 = setTimeout(1000);
setTimeout(4000);
cancelTimeout(t1);

This roughly translates to the following sequence of events:
T1 = PostDelayedTask(1000)
MarkTaskCanceled(T1)
T2 = PostDelayedTask(3900)
PerformTask(T1); //after 1s, but as its cancelled, nothing executes.
PerformTask(T2); //after 4s

Hope this seals the deal.:)

Regards.


On 2014/09/18 18:38:40, jochen wrote:
> thx for adding the tests.
> 
> did you verify when the timeouts happen?
> 
> my concern is that after cancelling the first timeout, the second timeout
still
> fires at the intervall of the first timeout.

[jochen (gone - plz use gerrit), 2014-09-19 13:37:09]

almost there

https://codereview.chromium.org/563203002/diff/200001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/563203002/diff/200001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:149: if (m_lastQueuedTaskTimer->isActive())
{
ok, if you want to go without posting a task for the time difference, we can
simplify this a bit:

get rid of m_lastQueuedTaskTimer, and just always cancel the currently queued
task

[Mayur Kankanwadi, 2014-09-19 13:53:24]

https://codereview.chromium.org/563203002/diff/200001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/563203002/diff/200001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:149: if (m_lastQueuedTaskTimer->isActive())
{
On 2014/09/19 13:37:09, jochen (slow for reviews) wrote:
> ok, if you want to go without posting a task for the time difference, we can
> simplify this a bit:
> 
> get rid of m_lastQueuedTaskTimer, and just always cancel the currently queued
> task
I am bit confused with this suggestion.
If we discard m_lastQueuedTaskTimer, how would we know if the top of heap timer
interval is greater or less than the currently passed timer interval. As it is
not mandatory that the interval sent across is smaller than that of the
previously queued task.

[jochen (gone - plz use gerrit), 2014-09-22 09:40:52]

On 2014/09/19 at 13:53:24, mayurk.vk wrote:
>
https://codereview.chromium.org/563203002/diff/200001/Source/core/workers/Wor...
> File Source/core/workers/WorkerThread.cpp (right):
> 
>
https://codereview.chromium.org/563203002/diff/200001/Source/core/workers/Wor...
> Source/core/workers/WorkerThread.cpp:149: if
(m_lastQueuedTaskTimer->isActive()) {
> On 2014/09/19 13:37:09, jochen (slow for reviews) wrote:
> > ok, if you want to go without posting a task for the time difference, we can
> > simplify this a bit:
> > 
> > get rid of m_lastQueuedTaskTimer, and just always cancel the currently
queued
> > task
> I am bit confused with this suggestion.
> If we discard m_lastQueuedTaskTimer, how would we know if the top of heap
timer interval is greater or less than the currently passed timer interval. As
it is not mandatory that the interval sent across is smaller than that of the
previously queued task.

if you just post a new delayed task regardless, it doesn't matter, no?

[Mayur Kankanwadi, 2014-09-22 09:47:40]

On 2014/09/22 09:40:52, jochen (slow for reviews) wrote:
> On 2014/09/19 at 13:53:24, mayurk.vk wrote:
> >
>
https://codereview.chromium.org/563203002/diff/200001/Source/core/workers/Wor...
> > File Source/core/workers/WorkerThread.cpp (right):
> > 
> >
>
https://codereview.chromium.org/563203002/diff/200001/Source/core/workers/Wor...
> > Source/core/workers/WorkerThread.cpp:149: if
> (m_lastQueuedTaskTimer->isActive()) {
> > On 2014/09/19 13:37:09, jochen (slow for reviews) wrote:
> > > ok, if you want to go without posting a task for the time difference, we
can
> > > simplify this a bit:
> > > 
> > > get rid of m_lastQueuedTaskTimer, and just always cancel the currently
> queued
> > > task
> > I am bit confused with this suggestion.
> > If we discard m_lastQueuedTaskTimer, how would we know if the top of heap
> timer interval is greater or less than the currently passed timer interval. As
> it is not mandatory that the interval sent across is smaller than that of the
> previously queued task.
> 
> if you just post a new delayed task regardless, it doesn't matter, no?

The new delayed task is posted under two conditions:
1. m_lastQueuedTaskTimer is still active and the new interval passed is less
than the current timer pending interval
2. or if the m_lastQueuedTaskTimer is not active anymore.
A new delayed task is not posted when the new interval passed is greater than
the current timer pending interval.
So for this last condition we still need the m_lastQueuedTaskTimer.

[jochen (gone - plz use gerrit), 2014-09-22 13:56:29]

On 2014/09/22 at 09:47:40, mayurk.vk wrote:
> On 2014/09/22 09:40:52, jochen (slow for reviews) wrote:
> > On 2014/09/19 at 13:53:24, mayurk.vk wrote:
> > >
> >
https://codereview.chromium.org/563203002/diff/200001/Source/core/workers/Wor...
> > > File Source/core/workers/WorkerThread.cpp (right):
> > > 
> > >
> >
https://codereview.chromium.org/563203002/diff/200001/Source/core/workers/Wor...
> > > Source/core/workers/WorkerThread.cpp:149: if
> > (m_lastQueuedTaskTimer->isActive()) {
> > > On 2014/09/19 13:37:09, jochen (slow for reviews) wrote:
> > > > ok, if you want to go without posting a task for the time difference, we
can
> > > > simplify this a bit:
> > > > 
> > > > get rid of m_lastQueuedTaskTimer, and just always cancel the currently
> > queued
> > > > task
> > > I am bit confused with this suggestion.
> > > If we discard m_lastQueuedTaskTimer, how would we know if the top of heap
> > timer interval is greater or less than the currently passed timer interval.
As
> > it is not mandatory that the interval sent across is smaller than that of
the
> > previously queued task.
> > 
> > if you just post a new delayed task regardless, it doesn't matter, no?
> 
> The new delayed task is posted under two conditions:
> 1. m_lastQueuedTaskTimer is still active and the new interval passed is less
than the current timer pending interval
> 2. or if the m_lastQueuedTaskTimer is not active anymore.
> A new delayed task is not posted when the new interval passed is greater than
the current timer pending interval.
> So for this last condition we still need the m_lastQueuedTaskTimer.

that should never happen, no?

[Mayur Kankanwadi, 2014-09-23 14:08:02]

I think this log will help to explain. I have added my comments starting with
--^^

Scenario:
t1 = setTimeout(1000);
setTimeout(2000);
setTimeout(3000);
clearTimeout(t1);

--
[9:41:0923/193745:VERBOSE1:WorkerThread.cpp(142)] setFireInterval:1000
[9:41:0923/193745:VERBOSE1:WorkerThread.cpp(170)] Queueing up new task
[9:41:0923/193745:VERBOSE1:WorkerThread.cpp(171)] 1000 
[9:41:0923/193745:VERBOSE1:WorkerThread.cpp(112)] 0x335928011858:
WorkerThreadCancelableTask 
--^^ Timer and task for 1000ms queued up

[9:41:0923/193745:VERBOSE1:ExecutionContext.cpp(204)] Remove timer : 1 
--^^ clearTimeout(1000) removes the timer, WorkerSharedTimer does not get any
notification now

[9:41:0923/193745:VERBOSE1:WorkerThread.cpp(142)] setFireInterval:1999 n
[21796:21796:0923/193745:INFO:CONSOLE(26)] "lined up 3 timers", source:
http://107.108.71.123/worker_tests/hwww8.js (26)
[9:41:0923/193745:VERBOSE1:WorkerThread.cpp(163)] Timer not active anymore 
--^^ Now is when we can check if the old timer is still active or cleared out
using m_lastQueuedTaskTimer->isActive()
--^^ we will also cancel the task now, as its not active anymore

[9:41:0923/193745:VERBOSE1:WorkerThread.cpp(170)] Queueing up new task
[9:41:0923/193745:VERBOSE1:WorkerThread.cpp(171)] 1999
[9:41:0923/193745:VERBOSE1:WorkerThread.cpp(112)] 0x3359280118c8:
WorkerThreadCancelableTask
--^^ Timer and task for 2000ms queued up

[9:41:0923/193746:VERBOSE1:WorkerThread.cpp(100)] 0x335928011858: performTask
[9:41:0923/193746:VERBOSE1:WorkerThread.cpp(101)] m_taskCanceled : 1
--^^ task for 1000ms executed now, but as its canceled nothing happend

[9:41:0923/193747:VERBOSE1:WorkerThread.cpp(100)] 0x3359280118c8: performTask
[9:41:0923/193747:VERBOSE1:WorkerThread.cpp(101)] m_taskCanceled : 0
--^^ task for 2000ms executed now

[9:41:0923/193747:VERBOSE1:ExecutionContext.cpp(204)] Remove timer : 2
[9:41:0923/193747:VERBOSE1:WorkerThread.cpp(142)] setFireInterval:997
[9:41:0923/193747:VERBOSE1:WorkerThread.cpp(170)] Queueing up new task
[21796:21796:0923/193747:INFO:CONSOLE(24)] "HelloWorker2 - in 2 seconds.",
source: http://107.108.71.123/worker_tests/hwww8.js (24)
[9:41:0923/193747:VERBOSE1:WorkerThread.cpp(171)] 997
[9:41:0923/193747:VERBOSE1:WorkerThread.cpp(112)] 0x335928011938:
WorkerThreadCancelableTask
--^^ Timer and task for 3000ms queued up

[9:41:0923/193748:VERBOSE1:WorkerThread.cpp(100)] 0x335928011938: performTask
[9:41:0923/193748:VERBOSE1:WorkerThread.cpp(101)] m_taskCanceled : 0
--^^ task for 3000ms executed now

[9:41:0923/193748:VERBOSE1:ExecutionContext.cpp(204)] Remove timer : 3
[21796:21796:0923/193748:INFO:CONSOLE(25)] "HelloWorker3 - in 3 seconds.",
source: http://107.108.71.123/worker_tests/hwww8.js (25)
---------------------------------------------------------------------------
Hope this helps.

[jochen (gone - plz use gerrit), 2014-09-26 18:45:08]

hum, the example doesn't include a situation where a fire interval that is
bigger than the currently active timer is passed in.

1) interval = 1000, no active timer => active timer = 1000
2) cancel timer => no active timer
3) interval = 1999, no active timer => active timer = 1999
4) timer executes => no active timer
5) interval = 997, no active timer => active timer = 997
6) timer executes

so whenever the interval was set, there wasn't an active timer in your example?

[Mayur Kankanwadi, 2014-09-30 10:59:58]

On 2014/09/26 18:45:08, jochen (slow for reviews) wrote:
> hum, the example doesn't include a situation where a fire interval that is
> bigger than the currently active timer is passed in.
> 
> 1) interval = 1000, no active timer => active timer = 1000
> 2) cancel timer => no active timer
> 3) interval = 1999, no active timer => active timer = 1999
> 4) timer executes => no active timer
> 5) interval = 997, no active timer => active timer = 997
> 6) timer executes
> 
> so whenever the interval was set, there wasn't an active timer in your
example?
Hi jochen,
I think I got the logic correct this time.
I missed out on the "correct" implementation big time, totally missing even the
correct direction signs posted by you.
Hope, this did not exasperate you much.
I have uploaded the updated patch for review.
PTAL.
Thanks for the patience.

[jochen (gone - plz use gerrit), 2014-09-30 13:16:36]

lgtm, thanks!

[Mayur Kankanwadi, 2014-09-30 13:31:41]

The CQ bit was checked by mayurk.vk@samsung.com

[commit-bot: I haz the power, 2014-09-30 13:32:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/563203002/240001

[commit-bot: I haz the power, 2014-09-30 15:06:48]

Committed patchset #13 (id:240001) as 182930

[Zhenyao Mo, 2014-09-30 21:10:07]

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/617023003/ by zmo@chromium.org.

The reason for reverting is: I suspect this CL caused
fast/workers/worker-close.html to crash on Linux ASAN build.  Revert to
confirm..

[Mayur Kankanwadi, 2014-10-01 13:52:56]

On 2014/09/30 21:10:07, Zhenyao Mo wrote:
> A revert of this CL (patchset #13 id:240001) has been created in
> https://codereview.chromium.org/617023003/ by mailto:zmo@chromium.org.
> 
> The reason for reverting is: I suspect this CL caused
> fast/workers/worker-close.html to crash on Linux ASAN build.  Revert to
> confirm..

Hi jochen,
The Linux ASAN crash report indicated that the
WorkerThreadCancelableTask::cancelTask() call 
in WorkerSharedTimer::stop was using a stale pointer.
After analysing the test case, the following is my take on the scenario:
--
Some tasks are being queued from within a timeout. 
Then WorkerGlobalScope::close is called.
This should in turn not let the timer to complete.
--
Now in the patch in WorkerSharedTimer::stop the
WorkerThreadCancelableTask::cancelTask()
is unnecessary. It will not serve any purpose as once close()
is called, none of the tasks are executed, so even the
WorkerThreadCancelableTask
will not execute.
So removing that stop should ideally fix the ASAN crash.
I have uploaded the changed patch.
PTAL.
Thanks.

[jochen (gone - plz use gerrit), 2014-10-01 14:01:51]

still lgtm

[Mayur Kankanwadi, 2014-10-01 14:10:13]

The CQ bit was checked by mayurk.vk@samsung.com

[commit-bot: I haz the power, 2014-10-01 14:11:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/563203002/260001

[Mayur Kankanwadi, 2014-10-01 14:29:43]

On 2014/10/01 14:01:51, jochen wrote:
> still lgtm

Thanks.

[commit-bot: I haz the power, 2014-10-01 15:28:35]

Committed patchset #14 (id:260001) as 183066