Changeset 94242 in webkit


Ignore:
Timestamp:
Aug 31, 2011 4:01:42 PM (13 years ago)
Author:
ap@apple.com
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.

Location:
trunk
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r94239 r94242  
     12011-08-31  Alexey Proskuryakov  <ap@apple.com>
     2
     3        http/tests/eventsource/workers/eventsource-simple.html is a flaky crash because of
     4        eventsource-status-error-iframe-crash.html
     5        https://bugs.webkit.org/show_bug.cgi?id=61523
     6
     7        Reviewed by Nate Chapin.
     8
     9        * http/tests/eventsource/eventsource-status-code-states-expected.txt:
     10        * http/tests/eventsource/eventsource-status-code-states.html:
     11        2xx responses are no longer different from any other failures.
     12
    1132011-08-31  David Levin  <levin@chromium.org>
    214

  • trunk/LayoutTests/http/tests/eventsource/eventsource-status-code-states-expected.txt

    r47323 r94242  
    22
    33PASS: status code 200 resulted in states CONNECTING, OPEN, OPEN, CONNECTING, CLOSED
    4 PASS: status code 204 resulted in states CONNECTING, CONNECTING, CLOSED
    5 PASS: status code 205 resulted in states CONNECTING, CONNECTING, CLOSED
    6 PASS: status code 202 resulted in states CONNECTING, CONNECTING, CLOSED
     4PASS: status code 204 resulted in states CONNECTING, CLOSED, CLOSED
     5PASS: status code 205 resulted in states CONNECTING, CLOSED, CLOSED
     6PASS: status code 202 resulted in states CONNECTING, CLOSED, CLOSED
    77PASS: status code 301 resulted in states CONNECTING, OPEN, OPEN, CONNECTING, CLOSED
    88PASS: status code 302 resulted in states CONNECTING, OPEN, OPEN, CONNECTING, CLOSED

  • trunk/LayoutTests/http/tests/eventsource/eventsource-status-code-states.html

    r47323 r94242  
    2222
    2323var tests = [{"code": 200, "expectedStates": [CONNECTING, OPEN, OPEN, CONNECTING, CLOSED]},
    24              {"code": 204, "expectedStates": [CONNECTING,,, CONNECTING, CLOSED]},
    25              {"code": 205, "expectedStates": [CONNECTING,,, CONNECTING, CLOSED]},
    26              {"code": 202, "expectedStates": [CONNECTING,,, CONNECTING, CLOSED]}, // other 2xx
     24             {"code": 204, "expectedStates": [CONNECTING,,, CLOSED, CLOSED]},
     25             {"code": 205, "expectedStates": [CONNECTING,,, CLOSED, CLOSED]},
     26             {"code": 202, "expectedStates": [CONNECTING,,, CLOSED, CLOSED]}, // other 2xx
    2727             {"code": 301, "expectedStates": [CONNECTING, OPEN, OPEN, CONNECTING, CLOSED]},
    2828             {"code": 302, "expectedStates": [CONNECTING, OPEN, OPEN, CONNECTING, CLOSED]},

  • trunk/Source/WebCore/ChangeLog

    r94236 r94242  
     12011-08-31  Alexey Proskuryakov  <ap@apple.com>
     2
     3        http/tests/eventsource/workers/eventsource-simple.html is a flaky crash because of
     4        eventsource-status-error-iframe-crash.html
     5        https://bugs.webkit.org/show_bug.cgi?id=61523
     6
     7        Reviewed by Nate Chapin.
     8
     9        The problem here was that canceling EventSource during frame removal erroneously resulted
     10        in event dispatch, and event handler re-entered frame destruction code.
     11
     12        * page/EventSource.h: Renamed endRequest() to networkRequestEnded(), because this method
     13        doesn't end request. It implements "reestablish the connection" or "fail the connection"
     14        algotithms from the spec, depending on current state.
     15        Removed m_failSilently, since we can make this decision with existing data, and want to
     16        fail silently by default (e.g. when detaching a frame cancels all loads).
     17
     18        * page/EventSource.cpp:
     19        (WebCore::EventSource::EventSource): Don't initialize m_failSilently.
     20        (WebCore::EventSource::~EventSource): Assert taht we are in a correct state.
     21        (WebCore::EventSource::connect): Ditto.
     22        (WebCore::EventSource::networkRequestEnded): Moved errorevent dispatch elsewhere.
     23        (WebCore::EventSource::scheduleReconnect): Error event should always be queued when
     24        reconnecting; firing it synchronously after starting m_reconnectTimer implements that.
     25        (WebCore::EventSource::reconnectTimerFired): Assert that state is correct (the timer is
     26        stopped if EventSource is stopped while waiting on the timer).
     27        (WebCore::EventSource::close): Don't set m_state before calling cancel() - it will indirectly
     28        call didFail(), which asserts that EventSource is not stopped yet.
     29        (WebCore::EventSource::didReceiveResponse): Explicitly dispatch an error event, since it
     30        is no longer dispatched when canceling, and canceling is the only way to stop a ThreadableLoader.
     31        Removed a special case for 2xx responses, since it's no longer in the spec.
     32        (WebCore::EventSource::didReceiveData): Assert that state is correct.
     33        (WebCore::EventSource::didFinishLoading): Don't set state to CONNECTING after parsing remaining
     34        response bytes - that may well result in dispatching an event whose handler calls close().
     35        (WebCore::EventSource::didFail): It's simple now - we always reconnect unless the request
     36        got canceled.
     37        (WebCore::EventSource::didFailRedirectCheck): Dispatch error event explicitly, as we are
     38        not going to attempt reconnecting.
     39
    1402011-08-31  Sheriff Bot  <webkit.review.bot@gmail.com>
    241

  • trunk/Source/WebCore/page/EventSource.cpp

    r93886 r94242  
    6666    , m_reconnectTimer(this, &EventSource::reconnectTimerFired)
    6767    , m_discardTrailingNewline(false)
    68     , m_failSilently(false)
    6968    , m_requestInFlight(false)
    7069    , m_reconnectDelay(defaultReconnectDelay)
    … …  
    102101EventSource::~EventSource()
    103102{
     103    ASSERT(m_state == CLOSED);
     104    ASSERT(!m_requestInFlight);
    104105}
    105106
    106107void EventSource::connect()
    107108{
     109    ASSERT(m_state == CONNECTING);
     110    ASSERT(!m_requestInFlight);
     111
    108112    ResourceRequest request(m_url);
    109113    request.setHTTPMethod("GET");
    … …  
    125129}
    126130
    127 void EventSource::endRequest()
     131void EventSource::networkRequestEnded()
    128132{
    129133    if (!m_requestInFlight)
    … …  
    131135
    132136    m_requestInFlight = false;
    133 
    134     if (!m_failSilently)
    135         dispatchEvent(Event::create(eventNames().errorEvent, false, false));
    136137
    137138    if (m_state != CLOSED)
    … …  
    145146    m_state = CONNECTING;
    146147    m_reconnectTimer.startOneShot(m_reconnectDelay / 1000);
     148    dispatchEvent(Event::create(eventNames().errorEvent, false, false));
    147149}
    148150
    … …  
    164166void EventSource::close()
    165167{
    166     if (m_state == CLOSED)
     168    if (m_state == CLOSED) {
     169        ASSERT(!m_requestInFlight);
    167170        return;
    168 
     171    }
     172
     173    // Stop trying to reconnect if EventSource was explicitly closed or if ActiveDOMObject::stop() was called.
    169174    if (m_reconnectTimer.isActive()) {
    170175        m_reconnectTimer.stop();
    … …  
    172177    }
    173178
    174     m_state = CLOSED;
    175     m_failSilently = true;
    176 
    177179    if (m_requestInFlight)
    178180        m_loader->cancel();
     181
     182    m_state = CLOSED;
    179183}
    180184
    … …  
    186190void EventSource::didReceiveResponse(unsigned long, const ResourceResponse& response)
    187191{
     192    ASSERT(m_state == CONNECTING);
     193    ASSERT(m_requestInFlight);
     194
    188195    int statusCode = response.httpStatusCode();
    189196    bool mimeTypeIsValid = response.mimeType() == "text/event-stream";
    … …  
    191198    if (responseIsValid) {
    192199        const String& charset = response.textEncodingName();
    193         // If we have a charset, the only allowed value is UTF-8 (case-insensitive). This should match
    194         // the updated EventSource standard.
     200        // If we have a charset, the only allowed value is UTF-8 (case-insensitive).
    195201        responseIsValid = charset.isEmpty() || equalIgnoringCase(charset, "UTF-8");
    196202        if (!responseIsValid) {
    … …  
    216222        dispatchEvent(Event::create(eventNames().openEvent, false, false));
    217223    } else {
    218         if (statusCode <= 200 || statusCode > 299)
    219             m_state = CLOSED;
    220224        m_loader->cancel();
     225        dispatchEvent(Event::create(eventNames().errorEvent, false, false));
    221226    }
    222227}
    … …  
    224229void EventSource::didReceiveData(const char* data, int length)
    225230{
     231    ASSERT(m_state == OPEN);
     232    ASSERT(m_requestInFlight);
     233
    226234    append(m_receiveBuf, m_decoder->decode(data, length));
    227235    parseEventStream();
    … …  
    230238void EventSource::didFinishLoading(unsigned long, double)
    231239{
     240    ASSERT(m_state == OPEN);
     241    ASSERT(m_requestInFlight);
     242
    232243    if (m_receiveBuf.size() > 0 || m_data.size() > 0) {
    233244        append(m_receiveBuf, "\n\n");
    234245        parseEventStream();
    235246    }
    236     m_state = CONNECTING;
    237     endRequest();
     247    networkRequestEnded();
    238248}
    239249
    240250void EventSource::didFail(const ResourceError& error)
    241251{
    242     int canceled = error.isCancellation();
    243     if (((m_state == CONNECTING) && !canceled) || ((m_state == OPEN) && canceled))
     252    ASSERT(m_state != CLOSED);
     253    ASSERT(m_requestInFlight);
     254
     255    if (error.isCancellation())
    244256        m_state = CLOSED;
    245     endRequest();
     257    networkRequestEnded();
    246258}
    247259
    248260void EventSource::didFailRedirectCheck()
    249261{
    250     m_state = CLOSED;
     262    ASSERT(m_state == CONNECTING);
     263    ASSERT(m_requestInFlight);
     264
    251265    m_loader->cancel();
     266
     267    ASSERT(m_state == CLOSED);
     268    dispatchEvent(Event::create(eventNames().errorEvent, false, false));
    252269}
    253270

  • trunk/Source/WebCore/page/EventSource.h

    r89036 r94242  
    9898
    9999        void connect();
    100         void endRequest();
     100        void networkRequestEnded();
    101101        void scheduleReconnect();
    102102        void reconnectTimerFired(Timer<EventSource>*);
    … …  
    113113        Vector<UChar> m_receiveBuf;
    114114        bool m_discardTrailingNewline;
    115         bool m_failSilently;
    116115        bool m_requestInFlight;
    117116
Note: See TracChangeset for help on using the changeset viewer.