Bug 68833 – Discard event data not followed by an empty line before eof when parsing an event-stream

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Per-Erik Brodin]

Update event-stream parsing to reflect recent spec change: http://html5.org/tools/web-apps-tracker?from=6370&to=6371

This may affect some existing deployments (three of our current tests failed after the change).

[Per-Erik Brodin]

Created attachment 108722 [details]
patch

[Alexey Proskuryakov]

Sounds reasonable.

Were other browsers doing the same as WebKit here? Are they onboard with making a change?

[Per-Erik Brodin]

(In reply to comment #2)
> Were other browsers doing the same as WebKit here? Are they onboard with making a change?

The change was requested by Mozilla and annevk replied that the ABNF in the spec would have to be updated as well, so presumably both Mozilla and Opera will make the change. See http://lists.w3.org/Archives/Public/public-webapps/2011JulSep/0097.html

[Alexey Proskuryakov]

Comment on attachment 108722 [details]
patch

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

In other words, no explicit endorsement from IE. Still, seems fine to make this change.

> Source/WebCore/page/EventSource.cpp:247
> +        m_eventId = String();

We don't have a function that discards everything already?

> Source/WebCore/page/EventSource.h:118
> +        String m_eventId;
>          String m_lastEventId;

I'm not thrilled about having two eventId member variables. How are they different? Maybe "last" is actually "previous"? Can we encode this bit of state somehow else, without seemingly duplicating data?

[Per-Erik Brodin]

(In reply to comment #4)
> (From update of attachment 108722 [details])
> > Source/WebCore/page/EventSource.cpp:247
> > +        m_eventId = String();
> 
> We don't have a function that discards everything already?

No, since two newlines were added to m_receiveBuf before the event stream parser always ran to the end of it and did all necessary discarding.

> > Source/WebCore/page/EventSource.h:118
> > +        String m_eventId;
> >          String m_lastEventId;
> 
> I'm not thrilled about having two eventId member variables. How are they different? Maybe "last" is actually "previous"? Can we encode this bit of state somehow else, without seemingly duplicating data?

I'm not thrilled about it either. Both m_eventId and m_lastEventId represent the last 'id' field received. The difference is that m_eventId stores the value temporarily while parsing and the value isn't transferred to m_lastEventId until an event is dispatch to avoid overwriting m_lastEventId in case the 'id' field should be discarded. I couldn't come up with a better way to solve it than having two eventId members.

[Alexey Proskuryakov]

Comment on attachment 108722 [details]
patch

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

I see, that explains why we need two member variables.

Would "m_currentlyParsedEventId" and "m_latestCompleteEventId" work? You could also null out m_currentlyParsedEventId after transferring its value to m_latestCompleteEventId.

> Source/WebCore/page/EventSource.cpp:319
> +            if (!m_eventId.isNull())
> +                m_lastEventId = m_eventId;

What will break if this if() check is removed?

[Per-Erik Brodin]

Created attachment 108858 [details]
updated patch

(In reply to comment #6)
> (From update of attachment 108722 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108722&action=review
> 
> I see, that explains why we need two member variables.
> 
> Would "m_currentlyParsedEventId" and "m_latestCompleteEventId" work? You could also null out m_currentlyParsedEventId after transferring its value to m_latestCompleteEventId.

I would really like to leave m_lastEventId as it is since that's a name used in the spec and the lastEventId property on MessageEvent and the Last-Event-ID request header are set from it, but I've renamed m_eventId to m_currentlyParsedEventId to better reflect how it's used.
 
> > Source/WebCore/page/EventSource.cpp:319
> > +            if (!m_eventId.isNull())
> > +                m_lastEventId = m_eventId;
> 
> What will break if this if() check is removed?

If the EventSource reconnects and data is discarded, and the first event after reconnecting doesn't contain an 'id' field, then m_lastEventId will be incorrectly overwritten without that if() check. I've slightly updated the eventsource-reconnect test so that this case is covered by the tests.

[Alexey Proskuryakov]

Comment on attachment 108858 [details]
updated patch

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

> Source/WebCore/page/EventSource.cpp:318
> +            if (!m_currentlyParsedEventId.isNull()) {

So, we're supposed to keep sending the same old last event id if subsequent events don't have one? Weird.

> Source/WebCore/page/EventSource.h:118
>          String m_lastEventId;

I now agree that keeping the name makes sense.

[Per-Erik Brodin]

(In reply to comment #8)
> So, we're supposed to keep sending the same old last event id if subsequent events don't have one? Weird.

This is according to the specification:
"If an event doesn't have an "id" field, but an earlier event did set the event source's last event ID string, then the event's lastEventId field will be set to the value of whatever the last seen "id" field was."

[Ian 'Hixie' Hickson]

(The idea is that if the server sends periodic state messages followed by deltas, when you reconnect you want to give the server the ID of the last state message, not the deltas. Or some such.)

[WebKit Review Bot]

Comment on attachment 108858 [details]
updated patch

Clearing flags on attachment: 108858

Committed r103474: <http://trac.webkit.org/changeset/103474>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.