Bug 68833 - Discard event data not followed by an empty line before eof when parsing an event-stream
: Discard event data not followed by an empty line before eof when parsing an e...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-26 14:24 PDT by Per-Erik Brodin
Modified: 2011-12-21 16:37 PST (History)
3 users (show)

See Also:


Attachments
patch (7.36 KB, patch)
2011-09-26 14:26 PDT, Per-Erik Brodin
no flags Details | Formatted Diff | Diff
updated patch (9.63 KB, patch)
2011-09-27 09:16 PDT, Per-Erik Brodin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per-Erik Brodin 2011-09-26 14:24:26 PDT
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).
Comment 1 Per-Erik Brodin 2011-09-26 14:26:34 PDT
Created attachment 108722 [details]
patch
Comment 2 Alexey Proskuryakov 2011-09-26 14:34:02 PDT
Sounds reasonable.

Were other browsers doing the same as WebKit here? Are they onboard with making a change?
Comment 3 Per-Erik Brodin 2011-09-26 14:46:34 PDT
(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
Comment 4 Alexey Proskuryakov 2011-09-26 14:58:26 PDT
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?
Comment 5 Per-Erik Brodin 2011-09-26 15:16:51 PDT
(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.
Comment 6 Alexey Proskuryakov 2011-09-26 15:53:50 PDT
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?
Comment 7 Per-Erik Brodin 2011-09-27 09:16:06 PDT
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.
Comment 8 Alexey Proskuryakov 2011-09-27 09:47:52 PDT
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.
Comment 9 Per-Erik Brodin 2011-09-27 11:56:50 PDT
(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."
Comment 10 Ian 'Hixie' Hickson 2011-09-27 15:30:53 PDT
(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.)
Comment 11 WebKit Review Bot 2011-12-21 16:37:28 PST
Comment on attachment 108858 [details]
updated patch

Clearing flags on attachment: 108858

Committed r103474: <http://trac.webkit.org/changeset/103474>
Comment 12 WebKit Review Bot 2011-12-21 16:37:33 PST
All reviewed patches have been landed.  Closing bug.