Bug 113434

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.

Created attachment 199825 [details] Patch

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?

(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.

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.

Created attachment 200310 [details] Patch

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

Comment on attachment 200310 [details] Patch Clearing flags on attachment: 200310 Committed r149521: <http://trac.webkit.org/changeset/149521>

All reviewed patches have been landed. Closing bug.

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.

(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?

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

(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.

Reopen since we still have unprotected toRef() calls.

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

Created attachment 200894 [details] Patch Protect conversion of JSValue to JSValueRef

Comment on attachment 200894 [details] Patch Clearing flags on attachment: 200894 Committed r149671: <http://trac.webkit.org/changeset/149671>

All reviewed patches have been landed. Closing bug.

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

(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.

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

(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.