Bug 113434 – Crash when calling QWebFrame::evaluateJavaScript

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Stephen]

In Qt 5.0.1, when I use QWebFrame::evaluateJavaScript to execute some script within a QWebFrame, it's very likely to crash at the same place in MarkedAllocator.cpp.


void* MarkedAllocator::allocateSlowCase(size_t bytes)
{
    ASSERT(m_heap->globalData()->apiLock().currentThreadIsHoldingLock());
#if COLLECT_ON_EVERY_ALLOCATION
    m_heap->collectAllGarbage();
    ASSERT(m_heap->m_operationInProgress == NoOperation);
#endif
...

}

The debug assertion  ASSERT(m_heap->globalData()->apiLock().currentThreadIsHoldingLock()) was fired right after the entrance of this function. 

I did find a similar bug with the blackberry port that's already resolved here: https://bugs.webkit.org/show_bug.cgi?id=100504 . I added the following line "JSC::JSLockHolder lock(exec)" into QVariant QWebFrameAdapter::evaluateJavaScript(const QString &scriptSource) in QWebFrameAdapter.cpp:

QVariant QWebFrameAdapter::evaluateJavaScript(const QString &scriptSource)
{
    ScriptController* proxy = frame->script();
    QVariant rc;
    if (proxy) {
        int distance = 0;
        JSC::JSValue v = frame->script()->executeScript(ScriptSourceCode(scriptSource)).jsValue();
        JSC::ExecState* exec = proxy->globalObject(mainThreadNormalWorld())->globalExec();
        JSC::JSLockHolder lock(exec);    //This line fixes the bug.
        JSValueRef* ignoredException = 0;
        rc = JSC::Bindings::convertValueToQVariant(toRef(exec), toRef(exec, v), QMetaType::Void, &distance, ignoredException);
    }
    return rc;
}

It seems to work and stops the crash. However, I am not an inside developer and don't really know what I am doing. Can someone look into this bug and provide a real fix?

This is a serious bug since it will crash any applications who attempts to run their own scripts within web pages.

[Henry]

I can confirm this happens also on the 5.0.2 version of qtwebkit.  You can easily trigger the bug by using page with frames and running the evalulateJavascript function at the beginning of those frames loads.  I'll put together a testcase when I have extra time.

[Allan Sandfeld Jensen]

Created attachment 199825 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 199825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=199825&action=review

> Source/WebKit/qt/Api/qwebelement.cpp:765
> +    JSC::JSLockHolder lock(state);
>      JSValueRef* ignoredException = 0;
>      return JSC::Bindings::convertValueToQVariant(toRef(state), toRef(state, evaluationResult), QMetaType::Void, &distance, ignoredException);

Would there be issues if you put the lock inside convertValueToQVariant?

[Allan Sandfeld Jensen]

(In reply to comment #3)
> (From update of attachment 199825 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199825&action=review
> 
> > Source/WebKit/qt/Api/qwebelement.cpp:765
> > +    JSC::JSLockHolder lock(state);
> >      JSValueRef* ignoredException = 0;
> >      return JSC::Bindings::convertValueToQVariant(toRef(state), toRef(state, evaluationResult), QMetaType::Void, &distance, ignoredException);
> 
> Would there be issues if you put the lock inside convertValueToQVariant?

It would somehow have to be certain to be in the outer most function because convertValueToQVariant is called recursively.

[Allan Sandfeld Jensen]

Closer investigation tells me convertValueToQVariant should do the locking, but on a much finer grained basis. We already do locking automatically everywhere the JSAPI is used. The problem must be the few places we use methods not provided by the JSAPI , and do not first lock JSC.

[Allan Sandfeld Jensen]

Created attachment 200310 [details]
Patch

[Allan Sandfeld Jensen]

Btw, the assert is almost impossible to trigger on 64bit, but would probably happen reliably on 32bit in any JSC::toRef call.

[WebKit Commit Bot]

Comment on attachment 200310 [details]
Patch

Clearing flags on attachment: 200310

Committed r149521: <http://trac.webkit.org/changeset/149521>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen]

Unfortunately, when I apply the 2nd patch on the source of qt 5.0.2, the crash continues to happen. I set breakpoints in the patched locations inside qt_runtime.cpp but my application doesn't run through those points.

The first patch works fine for me.

[Allan Sandfeld Jensen]

(In reply to comment #10)
> Unfortunately, when I apply the 2nd patch on the source of qt 5.0.2, the crash continues to happen. I set breakpoints in the patched locations inside qt_runtime.cpp but my application doesn't run through those points.
> 
> The first patch works fine for me.

Do you have a backtrace of where the assertion is triggered?

[Stephen]

Here is the stack trace. Basically, I was executing my own custom script right before starting a new navigation (inside QWebPage::acceptNavigationRequest).



 	ntdll.dll!_ZwRaiseException@12()  + 0x12 bytes	
 	ntdll.dll!_ZwRaiseException@12()  + 0x12 bytes	
>	Qt5WebKitd.dll!JSC::MarkedAllocator::allocateSlowCase(unsigned int bytes=16)  Line 73 + 0x43 bytes	C++
 	Qt5WebKitd.dll!JSC::MarkedAllocator::allocate(unsigned int bytes=16)  Line 78 + 0xc bytes	C++
 	Qt5WebKitd.dll!JSC::MarkedSpace::allocateWithoutDestructor(unsigned int bytes=16)  Line 211	C++
 	Qt5WebKitd.dll!JSC::Heap::allocateWithoutDestructor(unsigned int bytes=16)  Line 415	C++
 	Qt5WebKitd.dll!JSC::allocateCell<JSC::JSAPIValueWrapper>(JSC::Heap & heap={...}, unsigned int size=16)  Line 320 + 0xc bytes	C++
 	Qt5WebKitd.dll!JSC::allocateCell<JSC::JSAPIValueWrapper>(JSC::Heap & heap={...})  Line 328 + 0xb bytes	C++
 	Qt5WebKitd.dll!JSC::JSAPIValueWrapper::create(JSC::ExecState * exec=0x0cdff7a8, JSC::JSValue value={...})  Line 49 + 0xe bytes	C++
 	Qt5WebKitd.dll!JSC::jsAPIValueWrapper(JSC::ExecState * exec=0x0cdff7a8, JSC::JSValue value={...})  Line 73 + 0x11 bytes	C++
 	Qt5WebKitd.dll!toRef(JSC::ExecState * exec=0x0cdff7a8, JSC::JSValue v={...})  Line 114 + 0x15 bytes	C++
 	Qt5WebKitd.dll!QWebFrameAdapter::evaluateJavaScript(const QString & scriptSource={...})  Line 206 + 0x1b bytes	C++
 	Qt5WebKitWidgetsd.dll!QWebFrame::evaluateJavaScript(const QString & scriptSource={...})  Line 913 + 0x14 bytes	C++
 	MyApp.exe!WebView::execScriptInFrame(const QString & sScriptCode={...}, QWebFrame * pFrame=0x0b76d470, bool bIncludeChildren=false)  Line 323 + 0x26 bytes	C++
 	MyApp.exe!WebView::execScript(QString sScriptCode={...}, WebView::enmExecuteScriptFrameType eFlags=ES_SPECFRAME, QVariant * pvResult=type = ..., QWebFrame * 
 	MyApp.exe!WebPage::acceptNavigationRequest(QWebFrame * frame=0x13000f98, const QNetworkRequest & request={...}, QWebPage::NavigationType type=NavigationTypeOther)  Line 326	C++
 	Qt5WebKitWidgetsd.dll!QWebPagePrivate::acceptNavigationRequest(QWebFrameAdapter * frameAdapter=0x138275b8, const QNetworkRequest & request={...}, int type=5)  Line 381	C++
 	Qt5WebKitd.dll!WebCore::FrameLoaderClientQt::dispatchDecidePolicyForNavigationAction(void (WebCore::PolicyAction)* function=0x03b0e380, const WebCore::NavigationAction & action={...}, const WebCore::ResourceRequest & request={...}, WTF::PassRefPtr<WebCore::FormState> __formal={...})  Line 1271 + 0x3b bytes	C++
 	Qt5WebKitd.dll!WebCore::PolicyChecker::checkNavigationPolicy(const WebCore::ResourceRequest & request={...}, WebCore::DocumentLoader * loader=0x139278b8, WTF::PassRefPtr<WebCore::FormState> formState={...}, void (void *, const WebCore::ResourceRequest &, WTF::PassRefPtr<WebCore::FormState>, bool)* function=0x0356b750, void * argument=0x1366c698)  Line 90	C++
 	Qt5WebKitd.dll!WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader * loader=0x139278b8, WebCore::FrameLoadType type=FrameLoadTypeRedirectWithLockedBackForwardList, WTF::PassRefPtr<WebCore::FormState> prpFormState={...})  Line 1395	C++
 	Qt5WebKitd.dll!WebCore::FrameLoader::loadWithNavigationAction(const WebCore::ResourceRequest & request={...}, const WebCore::NavigationAction & action={...}, bool lockHistory=false, WebCore::FrameLoadType type=FrameLoadTypeRedirectWithLockedBackForwardList, WTF::PassRefPtr<WebCore::FormState> formState={...})  Line 1298	C++
 	Qt5WebKitd.dll!WebCore::FrameLoader::loadURL(const WebCore::KURL & newURL={...}, const WTF::String & referrer={...}, const WTF::String & frameName={...}, bool lockHistory=false, WebCore::FrameLoadType newLoadType=FrameLoadTypeRedirectWithLockedBackForwardList, WTF::PassRefPtr<WebCore::Event> event={...}, WTF::PassRefPtr<WebCore::FormState> prpFormState={...})  Line 1233	C++
 	Qt5WebKitd.dll!WebCore::FrameLoader::loadFrameRequest(const WebCore::FrameLoadRequest & request={...}, bool lockHistory=false, bool lockBackForwardList=true, WTF::PassRefPtr<WebCore::Event> event={...}, WTF::PassRefPtr<WebCore::FormState> formState={...}, WebCore::ShouldSendReferrer shouldSendReferrer=MaybeSendReferrer)  Line 1166	C++
 	Qt5WebKitd.dll!WebCore::FrameLoader::urlSelected(const WebCore::FrameLoadRequest & passedRequest={...}, WTF::PassRefPtr<WebCore::Event> triggeringEvent={...}, bool lockHistory=false, bool lockBackForwardList=true, WebCore::ShouldSendReferrer shouldSendReferrer=MaybeSendReferrer, WebCore::ShouldReplaceDocumentIfJavaScriptURL shouldReplaceDocumentIfJavaScriptURL=ReplaceDocumentIfJavaScriptURL)  Line 318	C++
 	Qt5WebKitd.dll!WebCore::FrameLoader::changeLocation(WebCore::SecurityOrigin * securityOrigin=0x0edfec88, const WebCore::KURL & url={...}, const WTF::String & referrer={...}, bool lockHistory=false, bool lockBackForwardList=true, bool refresh=false)  Line 286 + 0x69 bytes	C++
 	Qt5WebKitd.dll!WebCore::ScheduledURLNavigation::fire(WebCore::Frame * frame=0x1366c648)  Line 109 + 0x4e bytes	C++
 	Qt5WebKitd.dll!WebCore::NavigationScheduler::timerFired(WebCore::Timer<WebCore::NavigationScheduler> * __formal=0x1366c968)  Line 419	C++
 	Qt5WebKitd.dll!WebCore::Timer<WebCore::RenderProgress>::fired()  Line 106 + 0x19 bytes	C++
 	Qt5WebKitd.dll!WebCore::ThreadTimers::sharedTimerFiredInternal()  Line 119	C++
 	Qt5WebKitd.dll!WebCore::ThreadTimers::sharedTimerFired()  Line 94	C++
 	Qt5WebKitd.dll!WebCore::SharedTimerQt::timerEvent(QTimerEvent * ev=0x00d8cd60)  Line 114	C++
 	Qt5Cored.dll!QObject::event(QEvent * e=0x00d8cd60)  Line 1052	C++
 	Qt5Widgetsd.dll!QApplicationPrivate::notify_helper(QObject * receiver=0x0d712400, QEvent * e=0x00d8cd60)  Line 3398 + 0x11 bytes	C++
 	Qt5Widgetsd.dll!QApplication::notify(QObject * receiver=0x0d712400, QEvent * e=0x00d8cd60)  Line 2829 + 0x10 bytes	C++
 	Qt5Cored.dll!QCoreApplication::notifyInternal(QObject * receiver=0x0d712400, QEvent * event=0x00d8cd60)  Line 767 + 0x15 bytes	C++
 	Qt5Cored.dll!QCoreApplication::sendEvent(QObject * receiver=0x0d712400, QEvent * event=0x00d8cd60)  Line 203 + 0x39 bytes	C++
 	Qt5Cored.dll!QEventDispatcherWin32::event(QEvent * e=0x137daad8)  Line 1107 + 0x10 bytes	C++
 	Qt5Widgetsd.dll!QApplicationPrivate::notify_helper(QObject * receiver=0x0b89f310, QEvent * e=0x137daad8)  Line 3398 + 0x11 bytes	C++
 	Qt5Widgetsd.dll!QApplication::notify(QObject * receiver=0x0b89f310, QEvent * e=0x137daad8)  Line 2829 + 0x10 bytes	C++
 	Qt5Cored.dll!QCoreApplication::notifyInternal(QObject * receiver=0x0b89f310, QEvent * event=0x137daad8)  Line 767 + 0x15 bytes	C++
 	Qt5Cored.dll!QCoreApplication::sendEvent(QObject * receiver=0x0b89f310, QEvent * event=0x137daad8)  Line 203 + 0x39 bytes	C++
 	Qt5Cored.dll!QCoreApplicationPrivate::sendPostedEvents(QObject * receiver=0x00000000, int event_type=0, QThreadData * data=0x0b89db88)  Line 1368 + 0x12 bytes	C++
 	Qt5Cored.dll!QCoreApplication::sendPostedEvents(QObject * receiver=0x00000000, int event_type=0)  Line 1228 + 0x11 bytes	C++
 	Qt5Guid.dll!QWindowSystemInterface::sendWindowSystemEvents(QFlags<enum QEventLoop::ProcessEventsFlag> flags={...})  Line 515 + 0xa bytes	C++
 	qwindowsd.dll!QWindowsGuiEventDispatcher::sendPostedEvents()  Line 86 + 0xd bytes	C++
 	Qt5Cored.dll!qt_internal_proc(HWND__ * hwnd=0x002f088a, unsigned int message=1025, unsigned int wp=0, long lp=0)  Line 423	C++
 	user32.dll!_InternalCallWinProc@20()  + 0x23 bytes	
 	user32.dll!_UserCallWinProcCheckWow@32()  + 0xb7 bytes	
 	user32.dll!_DispatchMessageWorker@8()  + 0xed bytes	
 	user32.dll!_DispatchMessageW@4()  + 0xf bytes	
 	Qt5Cored.dll!QEventDispatcherWin32::processEvents(QFlags<enum QEventLoop::ProcessEventsFlag> flags={...})  Line 744	C++
 	qwindowsd.dll!QWindowsGuiEventDispatcher::processEvents(QFlags<enum QEventLoop::ProcessEventsFlag> flags={...})  Line 78 + 0xd bytes	C++
 	Qt5Cored.dll!QEventLoop::processEvents(QFlags<enum QEventLoop::ProcessEventsFlag> flags={...})  Line 137	C++
 	Qt5Cored.dll!QEventLoop::exec(QFlags<enum QEventLoop::ProcessEventsFlag> flags={...})  Line 212 + 0x26 bytes	C++
 	Qt5Cored.dll!QCoreApplication::exec()  Line 1020 + 0x15 bytes	C++
 	Qt5Guid.dll!QGuiApplication::exec()  Line 1184	C++
 	Qt5Widgetsd.dll!QApplication::exec()  Line 2674	C++
 	MyApp.exe!main(int argc=1, char * * argv=0x0b765c10)  Line 94 + 0x6 bytes	C++
 	MyApp.exe!WinMain(HINSTANCE__ * instance=0x00240000, HINSTANCE__ * prevInstance=0x00000000, char * __formal=0x011d5742, int cmdShow=10)  Line 131 + 0x12 bytes	C++
 	MyApp.exe!__tmainCRTStartup()  Line 547 + 0x2c bytes	C
 	MyApp.exe!WinMainCRTStartup()  Line 371	C
 	kernel32.dll!@BaseThreadInitThunk@12()  + 0x12 bytes	
 	ntdll.dll!___RtlUserThreadStart@8()  + 0x27 bytes	
 	ntdll.dll!__RtlUserThreadStart@8()  + 0x1b bytes

[Allan Sandfeld Jensen]

(In reply to comment #12)
> Here is the stack trace. Basically, I was executing my own custom script right before starting a new navigation (inside QWebPage::acceptNavigationRequest).
> 
Ahh, that explains it. The problem is toRef(exec, v) which requires the lock held.

[Allan Sandfeld Jensen]

Reopen since we still have unprotected toRef() calls.

[Allan Sandfeld Jensen]

We have a really unfortunate mix of JSValue, JSValueRef and WebCore::ScriptValue here, that makes the conversions messy.

[Allan Sandfeld Jensen]

Created attachment 200894 [details]
Patch

Protect conversion of JSValue to JSValueRef

[WebKit Commit Bot]

Comment on attachment 200894 [details]
Patch

Clearing flags on attachment: 200894

Committed r149671: <http://trac.webkit.org/changeset/149671>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen]

Just to make sure, with the new patch, the old patch to qt_runtime.cpp is no longer necessary, right?

[Allan Sandfeld Jensen]

(In reply to comment #19)
> Just to make sure, with the new patch, the old patch to qt_runtime.cpp is no longer necessary, right?

No, they are both necessary.

[Stephen]

Then why the previous qt_runtime.cpp patch is marked as "obsolete" among the list of attachments?

[Allan Sandfeld Jensen]

(In reply to comment #21)
> Then why the previous qt_runtime.cpp patch is marked as "obsolete" among the list of attachments?

Only because marking the old patch obsolote is the default behavior of the script I used to upload the patch.