Bug 46746

Summary: Large files via XHR : out-of-memory crashes
Product: WebKit Reporter: Siddharth Mathur <s.mathur>
Component: WebCore JavaScriptAssignee: Nobody <webkit-unassigned>
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   
Description Flags
test case
Proper test case none

Description Siddharth Mathur 2010-09-28 12:33:46 PDT
When the attached code snippet it used to download a large file (say .js or .txt) via XHR, a null pointer crash results due to heap exhaustion in QtWebkit-Symbian application even when a large max heap size configured. 

Room for improvement ? 
a) reduce need for multiple small appends() in ScriptString/StringBuilder 
b) in case of allocation failure, gracefully indicate an XHR error condition  

63 User::Panic() W:\sf\os\kernelhwsrv\kernel\eka\euser\us_func.cpp:741
62 WTF::fastMalloc()
61 WTF::VectorBufferBase<wchar_t>::allocateBuffer()
W:\sf\mw\qt\src\3rdparty\webkit\JavaScriptCore\wtf\Vector.h:286 0x2c27408c    
60 WTF::VectorBuffer<wchar_t, 64>::allocateBuffer()
W:\sf\mw\qt\src\3rdparty\webkit\JavaScriptCore\wtf\Vector.h:416 0x2c98c132    
59 WTF::Vector<wchar_t, 64>::reserveCapacity()
W:\sf\mw\qt\src\3rdparty\webkit\JavaScriptCore\wtf\Vector.h:854 0x2c98c0ce    
58 WTF::Vector<wchar_t, 64>::expandCapacity()
W:\sf\mw\qt\src\3rdparty\webkit\JavaScriptCore\wtf\Vector.h:771 0x2c98c1f9    
57 WTF::Vector<wchar_t, 64>::expandCapacity()
W:\sf\mw\qt\src\3rdparty\webkit\JavaScriptCore\wtf\Vector.h:778 0x2c98c02b    
56 WTF::Vector<wchar_t, 64>::append<wchar_t>()
W:\sf\mw\qt\src\3rdparty\webkit\JavaScriptCore\wtf\Vector.h:913 0x2cbe8d3a    
55 JSC::StringBuilder::append()
54 WebCore::ScriptString::operator +=()
53 WebCore::XMLHttpRequest::didReceiveData()
W:\sf\mw\qt\src\3rdparty\webkit\WebCore\xml\XMLHttpRequest.cpp:945 0x2cbe894b   
52 WebCore::DocumentThreadableLoader::didReceiveData()
51 WebCore::SubresourceLoader::didReceiveData()
50 WebCore::ResourceLoader::didReceiveData()
49 WebCore::QNetworkReplyHandler::forwardData()
48 WebCore::QNetworkReplyHandler::qt_metacall()
47 QMetaObject::metacall() W:\sf\mw\qt\src\corelib\kernel\qmetaobject.cpp:237
46 QMetaObject::activate() W:\sf\mw\qt\src\corelib\kernel\qobject.cpp:3272
45 QIODevice::readyRead()
W:\sf\mw\qt\src\corelib\tmp\moc\debug_shared\moc_qiodevice.cpp:91 0x149814c2    
44 MyApp::MyAppNetworkReply::handleReadyRead()
X:\cMyApp\MyApp\widgetcore\network\MyAppnetworkreply.cpp:98 0x483e885b    
43 MyApp::MyAppNetworkReply::qt_metacall()
42 QMetaObject::metacall() W:\sf\mw\qt\src\corelib\kernel\qmetaobject.cpp:237
41 QMetaObject::activate() W:\sf\mw\qt\src\corelib\kernel\qobject.cpp:3272
40 QIODevice::readyRead()
W:\sf\mw\qt\src\corelib\tmp\moc\debug_shared\moc_qiodevice.cpp:91 0x149814c2    
39 QNetworkReplyImplPrivate::appendDownstreamDataSignalEmissions()
W:\sf\mw\qt\src\network\access\qnetworkreplyimpl.cpp:559 0x16401c27    
38 QNetworkReplyImplPrivate::appendDownstreamData()
W:\sf\mw\qt\src\network\access\qnetworkreplyimpl.cpp:540 0x16401ac9    
37 QNetworkAccessBackend::writeDownstreamData()
W:\sf\mw\qt\src\network\access\qnetworkaccessbackend.cpp:248 0x163e90af    
36 QNetworkAccessHttpBackend::readFromHttp()
W:\sf\mw\qt\src\network\access\qnetworkaccesshttpbackend.cpp:740 0x163f29df    
35 QNetworkAccessHttpBackend::replyReadyRead()
W:\sf\mw\qt\src\network\access\qnetworkaccesshttpbackend.cpp:722 0x163f28e9    
34 QNetworkAccessHttpBackend::qt_metacall()
33 QMetaObject::metacall() W:\sf\mw\qt\src\corelib\kernel\qmetaobject.cpp:237
32 QMetaObject::activate() W:\sf\mw\qt\src\corelib\kernel\qobject.cpp:3272
31 QHttpNetworkReply::readyRead()
30 QHttpNetworkConnectionChannel::_q_receiveReply()
W:\sf\mw\qt\src\network\access\qhttpnetworkconnectionchannel.cpp:425 0x163d9896 
29 QHttpNetworkConnectionChannel::_q_readyRead()
W:\sf\mw\qt\src\network\access\qhttpnetworkconnectionchannel.cpp:874 0x163dba94 
28 QHttpNetworkConnectionChannel::qt_metacall()
27 QMetaObject::metacall() W:\sf\mw\qt\src\corelib\kernel\qmetaobject.cpp:237
26 QMetaObject::activate() W:\sf\mw\qt\src\corelib\kernel\qobject.cpp:3272
25 QIODevice::readyRead()
W:\sf\mw\qt\src\corelib\tmp\moc\debug_shared\moc_qiodevice.cpp:91 0x149814c2    
24 QAbstractSocketPrivate::canReadNotification()
W:\sf\mw\qt\src\network\socket\qabstractsocket.cpp:639 0x16440860    
23 QAbstractSocketPrivate::readNotification()
W:\sf\mw\qt\src\network\socket\qabstractsocket_p.h:77 0x16445579    
22 QAbstractSocketEngine::readNotification()
W:\sf\mw\qt\src\network\socket\qabstractsocketengine.cpp:155 0x16430de6    
21 QReadNotifier::event()
W:\sf\mw\qt\src\network\socket\qnativesocketengine.cpp:1104 0x1643387a    
20 QApplicationPrivate::notify_helper()
W:\sf\mw\qt\src\gui\kernel\qapplication.cpp:4392 0x14e4848c    
19 QApplication::notify() W:\sf\mw\qt\src\gui\kernel\qapplication.cpp:3794
18 QCoreApplication::notifyInternal()
W:\sf\mw\qt\src\corelib\kernel\qcoreapplication.cpp:732 0x14916f0b    
17 QCoreApplication::sendEvent()
W:\sf\mw\qt\src\corelib\kernel\qcoreapplication.h:215 0x1488a433    
16 QEventDispatcherSymbian::socketFired()
W:\sf\mw\qt\src\corelib\kernel\qeventdispatcher_symbian.cpp:899 0x1494d914    
15 QSocketActiveObject::RunL()
W:\sf\mw\qt\src\corelib\kernel\qeventdispatcher_symbian.cpp:632 0x1494cbc1    
14 CActiveScheduler::RunIfReady()
W:\sf\os\kernelhwsrv\kernel\eka\euser\cbase\ub_act.cpp:1281 0x60002800    
13 QEventDispatcherSymbian::processEvents()
W:\sf\mw\qt\src\corelib\kernel\qeventdispatcher_symbian.cpp:831 0x1494d4cd    
12 QEventDispatcherS60::processEvents()
W:\sf\mw\qt\src\gui\kernel\qeventdispatcher_s60.cpp:74 0x14ebc425    
11 QEventLoop::processEvents()
W:\sf\mw\qt\src\corelib\kernel\qeventloop.cpp:149 0x14913fc0    
10 QEventLoop::exec() W:\sf\mw\qt\src\corelib\kernel\qeventloop.cpp:201
9 QCoreApplication::exec()
W:\sf\mw\qt\src\corelib\kernel\qcoreapplication.cpp:1009 0x14917543    
8 QApplication::exec() W:\sf\mw\qt\src\gui\kernel\qapplication.cpp:3668
7 main() X:\cMyApp\app\widget\MyAppWidgetUI\src\MyAppWidgetMain.cpp:104
0x4de525ad Collapse All Comments
Comment 1 Siddharth Mathur 2010-09-28 12:34:42 PDT
Created attachment 69086 [details]
test case
Comment 2 Siddharth Mathur 2010-10-15 07:42:05 PDT
Per Janne K's comments, we should re-test this bug after Bug 43987 is fixed (and cherrypicked into QtWebkit 2.x)
Comment 3 Janne Koskinen 2010-10-28 02:14:48 PDT
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.
Comment 4 Siddharth Mathur 2010-10-28 06:15:09 PDT
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 ***
Comment 5 Janne Koskinen 2011-01-18 05:11:50 PST
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.

>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.
Comment 6 Janne Koskinen 2011-01-18 05:19:51 PST
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.
Comment 7 Janne Koskinen 2011-01-20 04:37:37 PST
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?
Comment 8 Alexey Proskuryakov 2011-01-20 08:27:18 PST
> 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?
Comment 9 Janne Koskinen 2011-01-20 09:14:46 PST
(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.
Comment 10 Janne Koskinen 2011-01-31 01:50:56 PST
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.