Skip to content
This repository was archived by the owner on Sep 26, 2020. It is now read-only.

Commit b919270

Browse files
author
leif@vaadin.com
committed
Verify that Timer has not been canceled before firing
* cancelTimeout is not always effective with older versions of IE * Add cancelCounter that is updated when canceling and verified when firing * Fixes Issue 8101 Change-Id: I0ac16598bef878ed756f2fe0dc696166f2a3d504 Review-Link: https://gwt-review.googlesource.com/#/c/2511/ Review by: mdempsky@google.com git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@11611 8db76d5a-ed1c-0410-87a9-c151d255dfc7
1 parent 1e5311e commit b919270

File tree

3 files changed

+146
-7
lines changed

3 files changed

+146
-7
lines changed

user/src/com/google/gwt/user/client/Timer.java

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -57,15 +57,15 @@ private static native void clearTimeout(int id) /*-{
5757
@com.google.gwt.core.client.impl.Impl::clearTimeout(I)(id);
5858
}-*/;
5959

60-
private static native int createInterval(Timer timer, int period) /*-{
60+
private static native int createInterval(Timer timer, int period, int cancelCounter) /*-{
6161
return @com.google.gwt.core.client.impl.Impl::setInterval(Lcom/google/gwt/core/client/JavaScriptObject;I)(
62-
$entry(function() { timer.@com.google.gwt.user.client.Timer::fire()(); }),
62+
$entry(function() { timer.@com.google.gwt.user.client.Timer::fire(I)(cancelCounter); }),
6363
period);
6464
}-*/;
6565

66-
private static native int createTimeout(Timer timer, int delay) /*-{
66+
private static native int createTimeout(Timer timer, int delay, int cancelCounter) /*-{
6767
return @com.google.gwt.core.client.impl.Impl::setTimeout(Lcom/google/gwt/core/client/JavaScriptObject;I)(
68-
$entry(function() { timer.@com.google.gwt.user.client.Timer::fire()(); }),
68+
$entry(function() { timer.@com.google.gwt.user.client.Timer::fire(I)(cancelCounter); }),
6969
delay);
7070
}-*/;
7171

@@ -85,10 +85,17 @@ public void onClose(CloseEvent<Window> event) {
8585

8686
private int timerId;
8787

88+
/**
89+
* Workaround for broken clearTimeout in IE. Keeps track of whether cancel has been called since
90+
* schedule was called. See https://code.google.com/p/google-web-toolkit/issues/detail?id=8101
91+
*/
92+
private int cancelCounter = 0;
93+
8894
/**
8995
* Cancels this timer.
9096
*/
9197
public void cancel() {
98+
cancelCounter++;
9299
if (isRepeating) {
93100
clearInterval(timerId);
94101
} else {
@@ -115,7 +122,7 @@ public void schedule(int delayMillis) {
115122
}
116123
cancel();
117124
isRepeating = false;
118-
timerId = createTimeout(this, delayMillis);
125+
timerId = createTimeout(this, delayMillis, cancelCounter);
119126
timers.add(this);
120127
}
121128

@@ -131,14 +138,20 @@ public void scheduleRepeating(int periodMillis) {
131138
}
132139
cancel();
133140
isRepeating = true;
134-
timerId = createInterval(this, periodMillis);
141+
timerId = createInterval(this, periodMillis, cancelCounter);
135142
timers.add(this);
136143
}
137144

138145
/*
139146
* Called by native code when this timer fires.
147+
*
148+
* Only call run() if cancelCounter has not changed since the timer was scheduled.
140149
*/
141-
final void fire() {
150+
final void fire(int scheduleCancelCounter) {
151+
if (scheduleCancelCounter != cancelCounter) {
152+
return;
153+
}
154+
142155
// If this is a one-shot timer, remove it from the timer list. This will
143156
// allow it to be garbage collected.
144157
if (!isRepeating) {

user/test/com/google/gwt/user/MiscSuite.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import com.google.gwt.user.client.EventTest;
2626
import com.google.gwt.user.client.GestureEventSinkTest;
2727
import com.google.gwt.user.client.HistoryDisabledTest;
28+
import com.google.gwt.user.client.TimerCancelTest;
2829
import com.google.gwt.user.client.TouchEventSinkTest;
2930
import com.google.gwt.user.client.WindowTest;
3031
import com.google.gwt.user.datepicker.client.CalendarUtilTest;
@@ -53,6 +54,7 @@ public static Test suite() {
5354
suite.addTestSuite(HistoryDisabledTest.class);
5455
suite.addTestSuite(ImageBundleGeneratorTest.class);
5556
suite.addTestSuite(LayoutTest.class);
57+
suite.addTestSuite(TimerCancelTest.class);
5658
suite.addTestSuite(TouchEventSinkTest.class);
5759
suite.addTestSuite(WindowTest.class);
5860
suite.addTestSuite(XMLTest.class);
Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
/*
2+
* Copyright 2013 Google Inc.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
5+
* in compliance with the License. You may obtain a copy of the License at
6+
*
7+
* http://www.apache.org/licenses/LICENSE-2.0
8+
*
9+
* Unless required by applicable law or agreed to in writing, software distributed under the License
10+
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
11+
* or implied. See the License for the specific language governing permissions and limitations under
12+
* the License.
13+
*/
14+
15+
package com.google.gwt.user.client;
16+
17+
import com.google.gwt.core.client.Duration;
18+
import com.google.gwt.junit.client.GWTTestCase;
19+
20+
/**
21+
* Tests {@link Timer#cancel()} functionality.
22+
*/
23+
public class TimerCancelTest extends GWTTestCase {
24+
25+
private static final class CountingTimer extends Timer {
26+
private int timerCount;
27+
@Override
28+
public void run() {
29+
timerCount++;
30+
}
31+
}
32+
33+
@Override
34+
public String getModuleName() {
35+
return "com.google.gwt.user.UserTest";
36+
}
37+
38+
public void testCancelTimer() {
39+
final CountingTimer canceledTimer = new CountingTimer();
40+
41+
Timer cancelingTimer = new Timer() {
42+
@Override
43+
public void run() {
44+
assertEquals(0, canceledTimer.timerCount);
45+
canceledTimer.cancel();
46+
}
47+
};
48+
cancelingTimer.schedule(50);
49+
canceledTimer.schedule(100);
50+
51+
52+
busyWait(200);
53+
54+
delayTestFinish(500);
55+
new Timer() {
56+
@Override
57+
public void run() {
58+
assertEquals(0, canceledTimer.timerCount);
59+
finishTest();
60+
}
61+
}.schedule(300);
62+
}
63+
64+
public void testRestartTimer() {
65+
final CountingTimer restartedTimer = new CountingTimer();
66+
67+
Timer cancelingTimer = new Timer() {
68+
@Override
69+
public void run() {
70+
assertEquals(0, restartedTimer.timerCount);
71+
restartedTimer.cancel();
72+
restartedTimer.schedule(100);
73+
}
74+
};
75+
76+
cancelingTimer.schedule(50);
77+
restartedTimer.schedule(100);
78+
79+
busyWait(200);
80+
81+
delayTestFinish(500);
82+
new Timer() {
83+
@Override
84+
public void run() {
85+
assertEquals(1, restartedTimer.timerCount);
86+
finishTest();
87+
}
88+
}.schedule(400);
89+
}
90+
91+
private static void busyWait(double duration) {
92+
/*
93+
* It seems that IE adds an event to the javascript event loop immediately when a timer expires
94+
* (supposedly from a separate thread). After this has happened, canceling the timer has no
95+
* effect because it is already in the queue and no further checks are done when running the
96+
* event once it reaches the head of the queue.
97+
*
98+
* This means that to trigger the bug, we must ensure the timer has been added to the event loop
99+
* queue before it is canceled. To ensure this happens, we will busy wait until both timers
100+
* should have fired. This means the following happens:
101+
*
102+
* 1) While busy waiting, IE adds events for each timer to the event loop
103+
*
104+
* 2) IE pumps the event loop, running the helper timer that cancels the tested timer. This does
105+
* however not have any effect because the timer is already in the event loop queue.
106+
*
107+
* 3) IE pumps the event loop again and runs the event for the tested timer, without realizing
108+
* that it has been canceled.
109+
*
110+
* Without busy waiting, the tested timer would not yet have been added to the event loop queue
111+
* at the point when the timer is canceled, in which case canceling the timer would work as
112+
* expected.
113+
*
114+
* If the two timers are not scheduled in the same order that they will run, it seems that IE
115+
* does some additional checks that makes the problem disappear.
116+
*/
117+
118+
double start = Duration.currentTimeMillis();
119+
while (Duration.currentTimeMillis() - start <= duration) {
120+
// Busy wait
121+
}
122+
}
123+
124+
}

0 commit comments

Comments
 (0)