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
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-09-26 14:24 PST by
Modified: 2011-12-21 16:37 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-09-26 14:24:26 PST
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 From 2011-09-26 14:26:34 PST -------
Created an attachment (id=108722) [details]
patch
------- Comment #2 From 2011-09-26 14:34:02 PST -------
Sounds reasonable.

Were other browsers doing the same as WebKit here? Are they onboard with making a change?
------- Comment #3 From 2011-09-26 14:46:34 PST -------
(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 From 2011-09-26 14:58:26 PST -------
(From update of attachment 108722 [details])
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 From 2011-09-26 15:16:51 PST -------
(In reply to comment #4)
> (From update of attachment 108722 [details] [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 From 2011-09-26 15:53:50 PST -------
(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.

> 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 From 2011-09-27 09:16:06 PST -------
Created an attachment (id=108858) [details]
updated patch

(In reply to comment #6)
> (From update of attachment 108722 [details] [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 From 2011-09-27 09:47:52 PST -------
(From update of attachment 108858 [details])
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 From 2011-09-27 11:56:50 PST -------
(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 From 2011-09-27 15:30:53 PST -------
(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 From 2011-12-21 16:37:28 PST -------
(From update of attachment 108858 [details])
Clearing flags on attachment: 108858

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