Summary: | Large files via XHR : out-of-memory crashes | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Siddharth Mathur <s.mathur> | ||||||
Component: | WebCore JavaScript | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED WONTFIX | ||||||||
Severity: | Normal | CC: | ap, barraclough, benjamin, ggaren, hausmann, kling, koshuin, laszlo.gombos, s.mathur | ||||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | S60 Hardware | ||||||||
OS: | S60 3rd edition | ||||||||
Attachments: |
|
Description
Siddharth Mathur
2010-09-28 12:33:46 PDT
Created attachment 69086 [details]
test case
Per Janne K's comments, we should re-test this bug after Bug 43987 is fixed (and cherrypicked into QtWebkit 2.x) Tested trunk with Bug 43987 changes in N8. Downloading 10MB test file finishes and doesn't crash. Problem is that the downloading still takes too much RAM. 10MB download took 90MB RAM when loaded using QtTestBrowser. Downloading 10MB without crash is certainly a big improvement. Thanks Janne! I am marking this as Resolved/Duplicate for now to Bug 43987, even though from a QtWebkit 2.1 point-of-view, the changes are proving hard to cherrypick. (please feel free to reopen) *** This bug has been marked as a duplicate of bug 43987 *** Re-opening. Rope fix done in Bug 43987 didn't fix the OOM issue in Symbian. It just delayed it a bit. The test case here is not the one in internal bug reporting and this test case attached here in this bug works and thats why the confusion. I'll mark this one as obsolete and attach the proper one. Here is what is causing the OOM in symbian. Issue is amplified by the test case itself: The one in internal bug reporting has a progress bar which queries responseText each time readyState is changed and the state is LOADING. The length of the responseText is then used to determine the progress of the download state and displayed to user "xx/100". Now the problem. Each call to responseText allocates new JSString which is not deleted until the garbage collector is run. Meaning after few chunks into the download we run out of memory. WebCore/bindings/js/JSXMLHttpRequestCustom.cpp >JSValue JSXMLHttpRequest::responseText(ExecState* exec) const >{ > return jsOwnedStringOrNull(exec, impl()->responseText()); >} jsOwnedStringOrNull doesn't get deleted until next collector cleanup. Quick and dirty hack would be to use jsStringOrNull instead which calls reportExtraMemoryCost and if there is not enough memory ends up reset()ting the Heap. Other alternative is to call CollectAllGarbage() each time responseText is called. Ofc. the easiest way to deal with this is not to give user progress info ;) On a serious side the javascript code should be modified so that the progress info is changed once or twice a second to minimize memory consumption. Created attachment 79266 [details]
Proper test case
I don't have a host where I could have that test file. So you need to modify URL to get ~5MB test file.
After changing the responseText function to call CollectAllGarbage() we have a new issue. Due to massive allocation and deallocation during download causes the downloading to take long time. example here is 25MB file takes ~1h to complete and as added bonus heap gets completely trashed. So that is a no go. Few Ideas: 1. Each instance of responseText would differ only by string lenght unless detached. JSString associated with the downloaded content would be stored in single location. 2. responseText is null unless data is referenced We would only update the length of the object but not allocate the string unless needed. The advantage over 1. is we can have larger downloads. 3. all JSStrings would be null unless used i.e. lazy allocation of all strings. Would be fun to test :) Maybe such feature exists and I'm not aware of it? > Ofc. the easiest way to deal with this is not to give user progress info ;)
Do you really need to repeatedly query responseText? Would listening for progress event provide the necessary information?
(In reply to comment #8) > > Ofc. the easiest way to deal with this is not to give user progress info ;) > > Do you really need to repeatedly query responseText? Would listening for progress event provide the necessary information? Not required to my understanding. I'm trying to reach out for the guys who wrote the original report. If they say it is ok for them to use progress event instead then I can close this again. Ok, I checked and the test case is speed test which doesn't actually need the data at all so closing this issue. Test case has been altered to not to show the progress and not to show the end result in HTML which is duplicate of 53416 . Progress bar can be achieved by monitoring progress event. |