Bug 61863 - EventSource loader should not buffer data
Summary: EventSource loader should not buffer data
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Critical
Assignee: Nobody
URL:
Keywords:
Depends on: 65926
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-01 08:17 PDT by goberman
Modified: 2011-08-12 13:59 PDT (History)
7 users (show)

See Also:


Attachments
files to reproduce leak (1.87 KB, application/zip)
2011-06-01 08:41 PDT, goberman
no flags Details
Proposed patch (3.68 KB, patch)
2011-06-07 10:36 PDT, Adam Bergkvist
no flags Details | Formatted Diff | Diff
Updated patch (3.69 KB, patch)
2011-06-07 10:50 PDT, Adam Bergkvist
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description goberman 2011-06-01 08:17:51 PDT
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
Comment 1 Alexey Proskuryakov 2011-06-01 08:35:17 PDT
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.
Comment 2 goberman 2011-06-01 08:41:30 PDT
Created attachment 95609 [details]
files to reproduce leak
Comment 3 goberman 2011-06-01 08:42:26 PDT
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.
Comment 4 Adam Bergkvist 2011-06-07 10:36:47 PDT
Created attachment 96258 [details]
Proposed patch
Comment 5 WebKit Review Bot 2011-06-07 10:42:26 PDT
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.
Comment 6 Adam Bergkvist 2011-06-07 10:50:48 PDT
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.
Comment 7 goberman 2011-06-08 02:32:06 PDT
Adam, thanks for fixing it! If only EventSource allowed cross domain access, it would have been the best streaming mechanism.
Comment 8 Adam Bergkvist 2011-08-08 08:35:03 PDT
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.
Comment 9 Alexey Proskuryakov 2011-08-08 13:02:02 PDT
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.
Comment 10 Pratik Solanki 2011-08-08 14:43:05 PDT
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.
Comment 11 Alexey Proskuryakov 2011-08-08 15:06:19 PDT
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.
Comment 12 Alexey Proskuryakov 2011-08-08 15:07:07 PDT
Comment on attachment 96262 [details]
Updated patch

r+/cq-, this depends on another fix we need to make.
Comment 13 Pratik Solanki 2011-08-09 10:09:12 PDT
(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.
Comment 14 Alexey Proskuryakov 2011-08-12 13:40:41 PDT
Comment on attachment 96262 [details]
Updated patch

Blocking bug fixed, fine to land now.
Comment 15 WebKit Review Bot 2011-08-12 13:59:05 PDT
Comment on attachment 96262 [details]
Updated patch

Clearing flags on attachment: 96262

Committed r92991: <http://trac.webkit.org/changeset/92991>
Comment 16 WebKit Review Bot 2011-08-12 13:59:11 PDT
All reviewed patches have been landed.  Closing bug.