- Revision
- 94242
- Author
- a...@apple.com
- Date
- 2011-08-31 16:01:42 -0700 (Wed, 31 Aug 2011)
Log Message
http/tests/eventsource/workers/eventsource-simple.html is a flaky crash because of
eventsource-status-error-iframe-crash.html
https://bugs.webkit.org/show_bug.cgi?id=61523
Reviewed by Nate Chapin.
Source/WebCore:
The problem here was that canceling EventSource during frame removal erroneously resulted
in event dispatch, and event handler re-entered frame destruction code.
* page/EventSource.h: Renamed endRequest() to networkRequestEnded(), because this method
doesn't end request. It implements "reestablish the connection" or "fail the connection"
algotithms from the spec, depending on current state.
Removed m_failSilently, since we can make this decision with existing data, and want to
fail silently by default (e.g. when detaching a frame cancels all loads).
* page/EventSource.cpp:
(WebCore::EventSource::EventSource): Don't initialize m_failSilently.
(WebCore::EventSource::~EventSource): Assert taht we are in a correct state.
(WebCore::EventSource::connect): Ditto.
(WebCore::EventSource::networkRequestEnded): Moved errorevent dispatch elsewhere.
(WebCore::EventSource::scheduleReconnect): Error event should always be queued when
reconnecting; firing it synchronously after starting m_reconnectTimer implements that.
(WebCore::EventSource::reconnectTimerFired): Assert that state is correct (the timer is
stopped if EventSource is stopped while waiting on the timer).
(WebCore::EventSource::close): Don't set m_state before calling cancel() - it will indirectly
call didFail(), which asserts that EventSource is not stopped yet.
(WebCore::EventSource::didReceiveResponse): Explicitly dispatch an error event, since it
is no longer dispatched when canceling, and canceling is the only way to stop a ThreadableLoader.
Removed a special case for 2xx responses, since it's no longer in the spec.
(WebCore::EventSource::didReceiveData): Assert that state is correct.
(WebCore::EventSource::didFinishLoading): Don't set state to CONNECTING after parsing remaining
response bytes - that may well result in dispatching an event whose handler calls close().
(WebCore::EventSource::didFail): It's simple now - we always reconnect unless the request
got canceled.
(WebCore::EventSource::didFailRedirectCheck): Dispatch error event explicitly, as we are
not going to attempt reconnecting.
LayoutTests:
* http/tests/eventsource/eventsource-status-code-states-expected.txt:
* http/tests/eventsource/eventsource-status-code-states.html:
2xx responses are no longer different from any other failures.
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (94241 => 94242)
--- trunk/LayoutTests/ChangeLog 2011-08-31 22:44:42 UTC (rev 94241)
+++ trunk/LayoutTests/ChangeLog 2011-08-31 23:01:42 UTC (rev 94242)
@@ -1,3 +1,15 @@
+2011-08-31 Alexey Proskuryakov <a...@apple.com>
+
+ http/tests/eventsource/workers/eventsource-simple.html is a flaky crash because of
+ eventsource-status-error-iframe-crash.html
+ https://bugs.webkit.org/show_bug.cgi?id=61523
+
+ Reviewed by Nate Chapin.
+
+ * http/tests/eventsource/eventsource-status-code-states-expected.txt:
+ * http/tests/eventsource/eventsource-status-code-states.html:
+ 2xx responses are no longer different from any other failures.
+
2011-08-31 David Levin <le...@chromium.org>
[chromium] Fix expectations due to r94213.
Modified: trunk/LayoutTests/http/tests/eventsource/eventsource-status-code-states-expected.txt (94241 => 94242)
--- trunk/LayoutTests/http/tests/eventsource/eventsource-status-code-states-expected.txt 2011-08-31 22:44:42 UTC (rev 94241)
+++ trunk/LayoutTests/http/tests/eventsource/eventsource-status-code-states-expected.txt 2011-08-31 23:01:42 UTC (rev 94242)
@@ -1,9 +1,9 @@
Test EventSource states for different status codes. Should print a series of PASS messages followed by DONE.
PASS: status code 200 resulted in states CONNECTING, OPEN, OPEN, CONNECTING, CLOSED
-PASS: status code 204 resulted in states CONNECTING, CONNECTING, CLOSED
-PASS: status code 205 resulted in states CONNECTING, CONNECTING, CLOSED
-PASS: status code 202 resulted in states CONNECTING, CONNECTING, CLOSED
+PASS: status code 204 resulted in states CONNECTING, CLOSED, CLOSED
+PASS: status code 205 resulted in states CONNECTING, CLOSED, CLOSED
+PASS: status code 202 resulted in states CONNECTING, CLOSED, CLOSED
PASS: status code 301 resulted in states CONNECTING, OPEN, OPEN, CONNECTING, CLOSED
PASS: status code 302 resulted in states CONNECTING, OPEN, OPEN, CONNECTING, CLOSED
PASS: status code 303 resulted in states CONNECTING, OPEN, OPEN, CONNECTING, CLOSED
Modified: trunk/LayoutTests/http/tests/eventsource/eventsource-status-code-states.html (94241 => 94242)
--- trunk/LayoutTests/http/tests/eventsource/eventsource-status-code-states.html 2011-08-31 22:44:42 UTC (rev 94241)
+++ trunk/LayoutTests/http/tests/eventsource/eventsource-status-code-states.html 2011-08-31 23:01:42 UTC (rev 94242)
@@ -21,9 +21,9 @@
eval("var " + stateNames[i] + " = " + i);
var tests = [{"code": 200, "expectedStates": [CONNECTING, OPEN, OPEN, CONNECTING, CLOSED]},
- {"code": 204, "expectedStates": [CONNECTING,,, CONNECTING, CLOSED]},
- {"code": 205, "expectedStates": [CONNECTING,,, CONNECTING, CLOSED]},
- {"code": 202, "expectedStates": [CONNECTING,,, CONNECTING, CLOSED]}, // other 2xx
+ {"code": 204, "expectedStates": [CONNECTING,,, CLOSED, CLOSED]},
+ {"code": 205, "expectedStates": [CONNECTING,,, CLOSED, CLOSED]},
+ {"code": 202, "expectedStates": [CONNECTING,,, CLOSED, CLOSED]}, // other 2xx
{"code": 301, "expectedStates": [CONNECTING, OPEN, OPEN, CONNECTING, CLOSED]},
{"code": 302, "expectedStates": [CONNECTING, OPEN, OPEN, CONNECTING, CLOSED]},
{"code": 303, "expectedStates": [CONNECTING, OPEN, OPEN, CONNECTING, CLOSED]},
Modified: trunk/Source/WebCore/ChangeLog (94241 => 94242)
--- trunk/Source/WebCore/ChangeLog 2011-08-31 22:44:42 UTC (rev 94241)
+++ trunk/Source/WebCore/ChangeLog 2011-08-31 23:01:42 UTC (rev 94242)
@@ -1,3 +1,42 @@
+2011-08-31 Alexey Proskuryakov <a...@apple.com>
+
+ http/tests/eventsource/workers/eventsource-simple.html is a flaky crash because of
+ eventsource-status-error-iframe-crash.html
+ https://bugs.webkit.org/show_bug.cgi?id=61523
+
+ Reviewed by Nate Chapin.
+
+ The problem here was that canceling EventSource during frame removal erroneously resulted
+ in event dispatch, and event handler re-entered frame destruction code.
+
+ * page/EventSource.h: Renamed endRequest() to networkRequestEnded(), because this method
+ doesn't end request. It implements "reestablish the connection" or "fail the connection"
+ algotithms from the spec, depending on current state.
+ Removed m_failSilently, since we can make this decision with existing data, and want to
+ fail silently by default (e.g. when detaching a frame cancels all loads).
+
+ * page/EventSource.cpp:
+ (WebCore::EventSource::EventSource): Don't initialize m_failSilently.
+ (WebCore::EventSource::~EventSource): Assert taht we are in a correct state.
+ (WebCore::EventSource::connect): Ditto.
+ (WebCore::EventSource::networkRequestEnded): Moved errorevent dispatch elsewhere.
+ (WebCore::EventSource::scheduleReconnect): Error event should always be queued when
+ reconnecting; firing it synchronously after starting m_reconnectTimer implements that.
+ (WebCore::EventSource::reconnectTimerFired): Assert that state is correct (the timer is
+ stopped if EventSource is stopped while waiting on the timer).
+ (WebCore::EventSource::close): Don't set m_state before calling cancel() - it will indirectly
+ call didFail(), which asserts that EventSource is not stopped yet.
+ (WebCore::EventSource::didReceiveResponse): Explicitly dispatch an error event, since it
+ is no longer dispatched when canceling, and canceling is the only way to stop a ThreadableLoader.
+ Removed a special case for 2xx responses, since it's no longer in the spec.
+ (WebCore::EventSource::didReceiveData): Assert that state is correct.
+ (WebCore::EventSource::didFinishLoading): Don't set state to CONNECTING after parsing remaining
+ response bytes - that may well result in dispatching an event whose handler calls close().
+ (WebCore::EventSource::didFail): It's simple now - we always reconnect unless the request
+ got canceled.
+ (WebCore::EventSource::didFailRedirectCheck): Dispatch error event explicitly, as we are
+ not going to attempt reconnecting.
+
2011-08-31 Sheriff Bot <webkit.review....@gmail.com>
Unreviewed, rolling out r94116.
Modified: trunk/Source/WebCore/page/EventSource.cpp (94241 => 94242)
--- trunk/Source/WebCore/page/EventSource.cpp 2011-08-31 22:44:42 UTC (rev 94241)
+++ trunk/Source/WebCore/page/EventSource.cpp 2011-08-31 23:01:42 UTC (rev 94242)
@@ -65,7 +65,6 @@
, m_decoder(TextResourceDecoder::create("text/plain", "UTF-8"))
, m_reconnectTimer(this, &EventSource::reconnectTimerFired)
, m_discardTrailingNewline(false)
- , m_failSilently(false)
, m_requestInFlight(false)
, m_reconnectDelay(defaultReconnectDelay)
, m_origin(context->securityOrigin()->toString())
@@ -101,10 +100,15 @@
EventSource::~EventSource()
{
+ ASSERT(m_state == CLOSED);
+ ASSERT(!m_requestInFlight);
}
void EventSource::connect()
{
+ ASSERT(m_state == CONNECTING);
+ ASSERT(!m_requestInFlight);
+
ResourceRequest request(m_url);
request.setHTTPMethod("GET");
request.setHTTPHeaderField("Accept", "text/event-stream");
@@ -124,16 +128,13 @@
m_requestInFlight = true;
}
-void EventSource::endRequest()
+void EventSource::networkRequestEnded()
{
if (!m_requestInFlight)
return;
m_requestInFlight = false;
- if (!m_failSilently)
- dispatchEvent(Event::create(eventNames().errorEvent, false, false));
-
if (m_state != CLOSED)
scheduleReconnect();
else
@@ -144,6 +145,7 @@
{
m_state = CONNECTING;
m_reconnectTimer.startOneShot(m_reconnectDelay / 1000);
+ dispatchEvent(Event::create(eventNames().errorEvent, false, false));
}
void EventSource::reconnectTimerFired(Timer<EventSource>*)
@@ -163,19 +165,21 @@
void EventSource::close()
{
- if (m_state == CLOSED)
+ if (m_state == CLOSED) {
+ ASSERT(!m_requestInFlight);
return;
+ }
+ // Stop trying to reconnect if EventSource was explicitly closed or if ActiveDOMObject::stop() was called.
if (m_reconnectTimer.isActive()) {
m_reconnectTimer.stop();
unsetPendingActivity(this);
}
- m_state = CLOSED;
- m_failSilently = true;
-
if (m_requestInFlight)
m_loader->cancel();
+
+ m_state = CLOSED;
}
ScriptExecutionContext* EventSource::scriptExecutionContext() const
@@ -185,13 +189,15 @@
void EventSource::didReceiveResponse(unsigned long, const ResourceResponse& response)
{
+ ASSERT(m_state == CONNECTING);
+ ASSERT(m_requestInFlight);
+
int statusCode = response.httpStatusCode();
bool mimeTypeIsValid = response.mimeType() == "text/event-stream";
bool responseIsValid = statusCode == 200 && mimeTypeIsValid;
if (responseIsValid) {
const String& charset = response.textEncodingName();
- // If we have a charset, the only allowed value is UTF-8 (case-insensitive). This should match
- // the updated EventSource standard.
+ // If we have a charset, the only allowed value is UTF-8 (case-insensitive).
responseIsValid = charset.isEmpty() || equalIgnoringCase(charset, "UTF-8");
if (!responseIsValid) {
String message = "EventSource's response has a charset (\"";
@@ -215,40 +221,51 @@
m_state = OPEN;
dispatchEvent(Event::create(eventNames().openEvent, false, false));
} else {
- if (statusCode <= 200 || statusCode > 299)
- m_state = CLOSED;
m_loader->cancel();
+ dispatchEvent(Event::create(eventNames().errorEvent, false, false));
}
}
void EventSource::didReceiveData(const char* data, int length)
{
+ ASSERT(m_state == OPEN);
+ ASSERT(m_requestInFlight);
+
append(m_receiveBuf, m_decoder->decode(data, length));
parseEventStream();
}
void EventSource::didFinishLoading(unsigned long, double)
{
+ ASSERT(m_state == OPEN);
+ ASSERT(m_requestInFlight);
+
if (m_receiveBuf.size() > 0 || m_data.size() > 0) {
append(m_receiveBuf, "\n\n");
parseEventStream();
}
- m_state = CONNECTING;
- endRequest();
+ networkRequestEnded();
}
void EventSource::didFail(const ResourceError& error)
{
- int canceled = error.isCancellation();
- if (((m_state == CONNECTING) && !canceled) || ((m_state == OPEN) && canceled))
+ ASSERT(m_state != CLOSED);
+ ASSERT(m_requestInFlight);
+
+ if (error.isCancellation())
m_state = CLOSED;
- endRequest();
+ networkRequestEnded();
}
void EventSource::didFailRedirectCheck()
{
- m_state = CLOSED;
+ ASSERT(m_state == CONNECTING);
+ ASSERT(m_requestInFlight);
+
m_loader->cancel();
+
+ ASSERT(m_state == CLOSED);
+ dispatchEvent(Event::create(eventNames().errorEvent, false, false));
}
void EventSource::parseEventStream()
Modified: trunk/Source/WebCore/page/EventSource.h (94241 => 94242)
--- trunk/Source/WebCore/page/EventSource.h 2011-08-31 22:44:42 UTC (rev 94241)
+++ trunk/Source/WebCore/page/EventSource.h 2011-08-31 23:01:42 UTC (rev 94242)
@@ -97,7 +97,7 @@
virtual void didFailRedirectCheck();
void connect();
- void endRequest();
+ void networkRequestEnded();
void scheduleReconnect();
void reconnectTimerFired(Timer<EventSource>*);
void parseEventStream();
@@ -112,7 +112,6 @@
Timer<EventSource> m_reconnectTimer;
Vector<UChar> m_receiveBuf;
bool m_discardTrailingNewline;
- bool m_failSilently;
bool m_requestInFlight;
String m_eventName;
