Bug 61863 – EventSource loader should not buffer data

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

I filed a bug about memory leak in the EventSource streaming on Chromium 5 months ago. Since I see no progress on it, I am filing it here. Hopefully with better luck. Thanks.

Steps to duplicate are below:
http://code.google.com/p/chromium/issues/detail?id=68160

[Alexey Proskuryakov]

Could you please post actual steps to reproduce? Making everyone who looks at this bug click through to some other site for actual description is not very efficient.

Created attachment 95609 [details]
files to reproduce leak

This is yet another problem with EventSource. 
My browser based application connects to the Java server to receive financial data. The volume of data may be high with users receiving hundreds thousands updates from the server throughout the day. I currently use XHR to push data from the server to the client, however it requires periodic disconnect and I would like to replace it with something that does not create a memory leak/ does not require disconnects (unfortunately multipart/x-mixed-replace is not supported in Chrome).

I was surprised to find out that EventSource has a memory leak. I wrote a simple Java Server/ JavaScript client program to demonstrate the issue.

What steps will reproduce the problem?
1. Run the server in Tomcat (use the latest Tomcat 7). You will need to add Java file test.WebFrameworkServletEventSource. Add StreamingEventSourceTest.html as a web resource.
2. Open http://localhost/Test/web/StreamingEventSourceTest.html in Chrome and press "test". It should connect to the server and generate about 300 messages per second.
3. Now open task manager and watch memory for Chrome. It steadily grows at about 100K per second. It will allocate hundreds of thousands megabytes of memory if left to run for hours.

What is the expected result?
Did not expect memory to grow at all.

What happens instead?
Nasty memory leak.

[Adam Bergkvist]

Created attachment 96258 [details]
Proposed patch

[WebKit Review Bot]

Attachment 96258 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Bergkvist]

Created attachment 96262 [details]
Updated patch

Removed a tab from the ChangeLog.

I would like to change the bug title to "EventSource loader should not buffer data" since this is a buffering issue rather than a memory leak, but I don't have the necessary permissions.

Adam, thanks for fixing it! If only EventSource allowed cross domain access, it would have been the best streaming mechanism.

[Adam Bergkvist]

Alexey, could you please take a look at this patch? The fix is just one line but the patch has been in the review queue since early June.

[Alexey Proskuryakov]

I think that this patch will break platforms that use CFNETWORK_DATA_ARRAY_CALLBACK. Specifically, ResourceLoader::didReceiveDataArray() bails out if m_shouldBufferData is false.

This is probably a bug in didReceiveDataArray(), but it needs to be fixed prior to landing this.

I don't quite understand what the "should buffer data" option is about, and why it would cause huge leaks in the first place. CC'ing some folks who have more recent experience with this code.

[Pratik Solanki]

I don't recall why I added the early bailout for m_shouldBufferData in ResourceLoader::didReceiveDataArray(). Most likely I did that because of what ResourceLoader::addData() does. ResourceLoader::didReceiveData() calls addData() and since that had the check, I kept the same check in didReceiveDataArray(). I don't know how EventSource works, but if the patch is correct when CFNETWORK_DATA_ARRAY_CALLBACK is not enabled, then it should be correct when we do enable that flag.

[Alexey Proskuryakov]

So I think what we should do is:
1. Fix ResourceLoader::didReceiveDataArray() to not bail out (in a separate patch).
2. Land this EventSource patch as is.
3. Consider renaming shouldBufferData, since it's arguably not about buffering. I certainly don't think about storing the full response body in ResourceLoader when I see "shouldBufferData". Buffers usually have a fixed size.

[Alexey Proskuryakov]

Comment on attachment 96262 [details]
Updated patch

r+/cq-, this depends on another fix we need to make.

[Pratik Solanki]

(In reply to comment #11)
> So I think what we should do is:
> 1. Fix ResourceLoader::didReceiveDataArray() to not bail out (in a separate patch).

Filed bug 65926 to track this.

[Alexey Proskuryakov]

Comment on attachment 96262 [details]
Updated patch

Blocking bug fixed, fine to land now.

[WebKit Review Bot]

Comment on attachment 96262 [details]
Updated patch

Clearing flags on attachment: 96262

Committed r92991: <http://trac.webkit.org/changeset/92991>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.