Bug 49096 – JSCallbackData::invokeCallback triggers layout from a worker thread

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ryosuke Niwa]

We call Document::updateStyleForAllDocuments() in JSCallbackData::invokeCallback without checking that we're in the main thread.

Stack trace:
#0    0x10109fe3f in WebCore::Document::updateStyleForAllDocuments at Document.cpp:1564
#1    0x10148acb1 in WebCore::JSCallbackData::invokeCallback at JSCallbackData.cpp:73
#2    0x1015e87b6 in WebCore::JSSQLTransactionSyncCallback::handleEvent at JSSQLTransactionSyncCallback.cpp:72
#3    0x1019e5520 in WebCore::SQLTransactionSync::execute at SQLTransactionSync.cpp:153
#4    0x101062abb in WebCore::DatabaseSync::runTransaction at DatabaseSync.cpp:152
#5    0x101062b9d in WebCore::DatabaseSync::transaction at DatabaseSync.cpp:134
#6    0x1014bdcf7 in WebCore::jsDatabaseSyncPrototypeFunctionTransaction at JSDatabaseSync.cpp:182
#7    0x3f93858041b8 in ??
#8    0x1001b65bd in JSC::JITCode::execute at JITCode.h:77
#9    0x1001b2648 in JSC::Interpreter::execute at Interpreter.cpp:759
#10    0x100181fa6 in JSC::evaluate at Completion.cpp:62
#11    0x101b43026 in WebCore::WorkerScriptController::evaluate at WorkerScriptController.cpp:128
#12    0x101b4320c in WebCore::WorkerScriptController::evaluate at WorkerScriptController.cpp:107
#13    0x101b44a48 in WebCore::WorkerThread::workerThread at WorkerThread.cpp:134
#14    0x101b44b41 in WebCore::WorkerThread::workerThreadStart at WorkerThread.cpp:117
#15    0x1002c974d in WTF::threadEntryPoint at Threading.cpp:65

[Ryosuke Niwa]

Created attachment 73128 [details]
fixes the bug

[David Levin]

Comment on attachment 73128 [details]
fixes the bug

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

> WebCore/bindings/js/JSCallbackData.cpp:73
> +    if (context->isDocument())

It would be nice to not call this virtual function nearly twice in a row. Consider storing the value in a bool and using in both places.

[Ryosuke Niwa]

Mn... somehow fast/workers/storage/use-same-database-in-page-and-workers.html always crash with the following error:

ERROR: Unable to turn on incremental auto-vacuum for database /var/folders/++/++3qwE++6+0++4RjPqRgNE+-Rj2/-Tmp-/DumpRenderTree-tplq1u/Databases/file__0/000000000000001d.db
(/Volumes/Data/webkit4/WebCore/storage/AbstractDatabase.cpp:251 virtual bool WebCore::AbstractDatabase::performOpenAndVerify(bool, WebCore::ExceptionCode&))
ASSERTION FAILED: !protectedObjectCount()
(/Volumes/Data/webkit4/JavaScriptCore/runtime/Collector.cpp:262 void JSC::Heap::freeBlocks())

Should we fix this problem first?

[Ryosuke Niwa]

(In reply to comment #2)
> (From update of attachment 73128 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73128&action=review
> 
> > WebCore/bindings/js/JSCallbackData.cpp:73
> > +    if (context->isDocument())
> 
> It would be nice to not call this virtual function nearly twice in a row. Consider storing the value in a bool and using in both places.

Will do and land:

-    JSValue result = context->isDocument() 
+    bool contextIsDocument = context->isDocument();
+    JSValue result = contextIsDocument
         ? JSMainThreadExecState::call(exec, function, callType, callData, callback(), args)
         : JSC::call(exec, function, callType, callData, callback(), args);
     globalObject()->globalData().timeoutChecker.stop();
 
-    Document::updateStyleForAllDocuments();
+    if (contextIsDocument)
+        Document::updateStyleForAllDocuments();

[Ryosuke Niwa]

Committed r71455: <http://trac.webkit.org/changeset/71455>