Intermittent crashes in EventSource::close()

[Richard D Klein]

EventSource::close() sometimes crashes when you leave a page on which Server-Sent events are active.  Here's the offending code:

void EventSource::close()
{
    if (m_state == CLOSED)
        return;

    if (m_reconnectTimer.isActive()) {
        m_reconnectTimer.stop();
        unsetPendingActivity(this);
    }

    m_state = CLOSED;
    m_failSilently = true;

    if (m_requestInFlight)
        m_loader->cancel();
}

The crash happens when m_requestInFlight is true, but m_loader is 0.  This should fix it:

    if (m_requestInFlight)
        if (m_loader)
            m_loader->cancel();

[Alexey Proskuryakov]

I'd say that m_requestInFlight should not be true when there is no request in flight!

We'll need to make a reduced case to be landed as a regression test with a fix.

[Richard D Klein]

> I'd say that m_requestInFlight should not be true when there is no request in flight!

Good point.  I should have labelled my "fix" as just a workaround.

[Alexey Proskuryakov]

Richard, does this still happen with recent nightly builds? There have been some semi-related fixes made to EventSource, but without steps to reproduce, it's unclear how to proceed.

[Nate Chapin]

This was also reported at http://code.google.com/p/chromium/issues/detail?id=89155.

It appears this is happening when an EventSource reconnect timer triggers after a navigation away from the current page has begun. In this case, SubresourceLoader::create() will return 0, and EventSource ends up thinking a request is in-flight but has a null m_loader.

I have a fix (don't set "m_requestInFlight = true" in connect() if ThreadableLoader::create() returns 0), but I don't have a good test reduction because it's timing-dependent.

[Nate Chapin]

Created attachment 103815 [details]
crash fix but no test :(

Is there a good way to ensure an internal timer like EventSource::m_reconnecTimer will fire when we want it to?

[Nate Chapin]

Created attachment 103826 [details]
+ test

I had a brain cramp when I posted the crash fix, creating a test was straightforward.

[WebKit Review Bot]

Comment on attachment 103826 [details]
+ test

Attachment 103826 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9380016

New failing tests:
http/tests/eventsource/eventsource-reconnect-during-navigate-crash.html

[Alexey Proskuryakov]

Nate, what is the status of this patch? It's marked for review, but its test is failing.

[Nate Chapin]

Comment on attachment 103826 [details]
+ test


Sorry, I've gotten distracted.  I need to borrow a linux box so I can figure out why the test is failing.

Obsoleting until then.

[Nate Chapin]

Created attachment 105040 [details]
+ test fix

[Alexey Proskuryakov]

Comment on attachment 105040 [details]
+ test fix

View in context: https://bugs.webkit.org/attachment.cgi?id=105040&action=review

r=me. It would be great if you could get rid of EventSender before landing.

> LayoutTests/http/tests/eventsource/eventsource-reconnect-during-navigate-crash.html:16
> +        eventSender.mouseMoveTo(a.offsetLeft + 2, a.offsetTop + 2);
> +        eventSender.mouseDown();
> +        eventSender.mouseUp();

Does no other way to navigate work? I would expect that setting window.location is no different.

It would be nice to have the test work in browser and in ports that don't' support eventSender.

> LayoutTests/http/tests/eventsource/resources/wait-then-notify-done.php:7
> +sleep(1);
> +echo "<script> if (window.layoutTestController) layoutTestController.notifyDone()</script>\n";

Sadness.

[Nate Chapin]

(In reply to comment #11)
> (From update of attachment 105040 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105040&action=review
> 
> r=me. It would be great if you could get rid of EventSender before landing.
> 
> > LayoutTests/http/tests/eventsource/eventsource-reconnect-during-navigate-crash.html:16
> > +        eventSender.mouseMoveTo(a.offsetLeft + 2, a.offsetTop + 2);
> > +        eventSender.mouseDown();
> > +        eventSender.mouseUp();
> 
> Does no other way to navigate work? I would expect that setting window.location is no different.
> 
> It would be nice to have the test work in browser and in ports that don't' support eventSender.

You're right, a window.location navigation reproduces the crash.  Will fix before landing.

> 
> > LayoutTests/http/tests/eventsource/resources/wait-then-notify-done.php:7
> > +sleep(1);
> > +echo "<script> if (window.layoutTestController) layoutTestController.notifyDone()</script>\n";
> 
> Sadness.
 
I agree.  :(

[Nate Chapin]

http://trac.webkit.org/changeset/93726