Bug 115058 – Assert in JSC::Heap::unprotect when closing facebook.com web site

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Stephen]

I've a browser application based on qt webkit 5.0.2. I use it to open facebook.com and do a little something such as reading some streaming news or post an update myself. After that, I close the QWebView which hosts the facebook.com website. More often than not, I got a debug assertion at the following location:

bool Heap::unprotect(JSValue k)
{
    ASSERT(k);
    ASSERT(m_globalData->apiLock().currentThreadIsHoldingLock());   <-----Here is the debug assertion raised.

    if (!k.isCell())
        return false;

    return m_protectedValues.remove(k.asCell());
}

I can't reproduce the crash every time. If I open facebook.com and close it immediately, it usually doesn't crash. But after doing viewing and posting on it, it is very likely to crash. 
By the way, I've already applied the patch I mentioned in another bug report: https://bugs.webkit.org/show_bug.cgi?id=113434 . That patch works well for most other websites. But facebook.com seems to be an exception. 

Here is a copy of the stack: 

I've a browser application based on qt webkit 5.0.2. 

 	ntdll.dll!_ZwRaiseException@12()  + 0x12 bytes	
 	ntdll.dll!_ZwRaiseException@12()  + 0x12 bytes	
>	Qt5WebKitd.dll!JSC::Heap::unprotect(JSC::JSValue k={...})  Line 344 + 0x42 bytes	C++
 	Qt5WebKitd.dll!JSC::gcUnprotect(JSC::JSCell * val=0x0ba1fe60)  Line 38	C++
 	Qt5WebKitd.dll!JSC::Bindings::RootObject::invalidate()  Line 132 + 0x10 bytes	C++
 	Qt5WebKitd.dll!WebCore::ScriptController::~ScriptController()  Line 86	C++
 	Qt5WebKitd.dll!WebCore::Frame::~Frame()  Line 228 + 0x5c bytes	C++
 	Qt5WebKitd.dll!WebCore::Frame::`scalar deleting destructor'()  + 0xf bytes	C++
 	Qt5WebKitd.dll!WTF::RefCounted<WebCore::StorageArea>::deref()  Line 202 + 0x38 bytes	C++
 	Qt5WebKitd.dll!WTF::derefIfNotNull<WebCore::FTPDirectoryDocumentParser>(WebCore::FTPDirectoryDocumentParser * ptr=0x0e6d6968)  Line 54	C++
 	Qt5WebKitd.dll!WTF::RefPtr<WebCore::StorageArea>::~RefPtr<WebCore::StorageArea>()  Line 56 + 0x12 bytes	C++
 	Qt5WebKitd.dll!WebCore::Page::~Page()  Line 218 + 0xb5 bytes	C++
 	Qt5WebKitd.dll!WebCore::Page::`scalar deleting destructor'()  + 0xf bytes	C++
 	Qt5WebKitd.dll!QWebPageAdapter::deletePage()  Line 237 + 0x1f bytes	C++
 	Qt5WebKitWidgetsd.dll!QWebPagePrivate::~QWebPagePrivate()  Line 238	C++
 	Qt5WebKitWidgetsd.dll!QWebPagePrivate::`scalar deleting destructor'()  + 0xf bytes	C++
 	Qt5WebKitWidgetsd.dll!QWebPage::~QWebPage()  Line 1368 + 0x23 bytes	C++
 	MyApp.exe!WebPage::~WebPage()  Line 54 + 0x40 bytes	C++
 	MyApp.exe!WebPage::`scalar deleting destructor'()  + 0xf bytes	C++
 	Qt5WebKitWidgetsd.dll!QWebViewPrivate::detachCurrentPage()  Line 236 + 0x24 bytes	C++
 	Qt5WebKitWidgetsd.dll!QWebViewPrivate::~QWebViewPrivate()  Line 64	C++
 	Qt5WebKitWidgetsd.dll!QWebViewPrivate::`scalar deleting destructor'()  + 0xf bytes	C++
 	Qt5WebKitWidgetsd.dll!QWebView::~QWebView()  Line 199 + 0x23 bytes	C++
 	MyApp.exe!WebView::~WebView()  Line 180 + 0xef bytes	C++
 	MyApp.exe!WebView::`scalar deleting destructor'()  + 0xf bytes	C++
 	Qt5Cored.dll!QObjectPrivate::deleteChildren()  Line 1764 + 0x24 bytes	C++
 	Qt5Widgetsd.dll!QWidget::~QWidget()  Line 1475	C++
 	Qt5Widgetsd.dll!QMdiSubWindow::~QMdiSubWindow()  Line 2289 + 0x8 bytes	C++
 	MyApp.exe!MdiSubWindow::~MdiSubWindow()  + 0x10 bytes	C++
 	MyApp.exe!MdiSubWindow::`scalar deleting destructor'()  + 0xf bytes	C++
 	Qt5Cored.dll!qDeleteInEventHandler(QObject * o=0x0e6dba28)  Line 4093 + 0x21 bytes	C++
 	Qt5Cored.dll!QObject::event(QEvent * e=0x0e6c6888)  Line 1061 + 0xc bytes	C++
 	Qt5Widgetsd.dll!QWidget::event(QEvent * event=0x0e6c6888)  Line 8250 + 0x10 bytes	C++
 	Qt5Widgetsd.dll!QMdiSubWindow::event(QEvent * event=0x0e6c6888)  Line 2917	C++
 	Qt5Widgetsd.dll!QApplicationPrivate::notify_helper(QObject * receiver=0x0e6dba28, QEvent * e=0x0e6c6888)  Line 3398 + 0x11 bytes	C++
 	Qt5Widgetsd.dll!QApplication::notify(QObject * receiver=0x0e6dba28, QEvent * e=0x0e6c6888)  Line 3363 + 0x10 bytes	C++
 	Qt5Cored.dll!QCoreApplication::notifyInternal(QObject * receiver=0x0e6dba28, QEvent * event=0x0e6c6888)  Line 767 + 0x15 bytes	C++
 	Qt5Cored.dll!QCoreApplication::sendEvent(QObject * receiver=0x0e6dba28, QEvent * event=0x0e6c6888)  Line 203 + 0x39 bytes	C++
 	Qt5Cored.dll!QCoreApplicationPrivate::sendPostedEvents(QObject * receiver=0x00000000, int event_type=0, QThreadData * data=0x0b47db88)  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=0x00090cbc, unsigned int message=275, unsigned int wp=4294967294, 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=0x09a35bb0)  Line 94 + 0x6 bytes	C++
 	MyApp.exe!WinMain(HINSTANCE__ * instance=0x01080000, HINSTANCE__ * prevInstance=0x00000000, char * __formal=0x003c5742, 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

[Geoffrey Garen]

The way to fix this is to put a JSLock inside ScriptController::~ScriptController.

[Stephen]

Do you have a patch to share for it?

By the way, I've found this similar bug report https://bugs.webkit.org/show_bug.cgi?id=89809 . Among the changeset 121098, the modificiation done to Heap.cpp inside Heap::protect and Heap::unprotect is particullarly interesting ( check http://trac.webkit.org/changeset/121098/trunk/Source/JavaScriptCore/heap/Heap.cpp ). 

The one condition assert was modified into a two condition assert in this patch as shown below:

ASSERT(JSLock::currentThreadIsHoldingLock() || !m_globalData->isSharedInstance());

Somehow, the 2nd conditon is removed again in the trunk version. If I add back the 2nd condition, it does stop the crash. This is just for your information. I don't really know what I am doing at all. 


(In reply to comment #1)
> The way to fix this is to put a JSLock inside ScriptController::~ScriptController.

[Allan Sandfeld Jensen]

Created attachment 199829 [details]
Patch

Patched as ggaren suggested

[WebKit Commit Bot]

Comment on attachment 199829 [details]
Patch

Clearing flags on attachment: 199829

Committed r149188: <http://trac.webkit.org/changeset/149188>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen]

Thanks for the patch. Unfortunately, I am not able to directly apply this patch on qtwebkit 5.0.2. JSDOMWindowBase::commonVM() is not defined in qtwebkit 5.0.2.

[Allan Sandfeld Jensen]

(In reply to comment #6)
> Thanks for the patch. Unfortunately, I am not able to directly apply this patch on qtwebkit 5.0.2. JSDOMWindowBase::commonVM() is not defined in qtwebkit 5.0.2.

It is called JSDOMWindowBase::commonJSGlobalData() in the Qt 5.0/5.1 branch. See https://codereview.qt-project.org/#change,55088