Bug 95341 – Web Inspector: The JS code injected by worker inspector shouldn't be evaluated through JSMainThreadExecState

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Peter Wang]

The implementation of "JSC::JSInjectedScriptManager" and "JSC::ScriptFunctionCall" have a defect, the injected JS is alwyas evaluated through "JSMainThreadExecState". It will cause failed assert, like this:

    at /home/torch-admin/project/upstream/Source/WebCore/bindings/js/JSMainThreadExecState.h:84
84	        ASSERT(isMainThread());
(gdb) bt 
#0  0x00007ffff51f4fcd in WebCore::JSMainThreadExecState::JSMainThreadExecState (this=0x7fff91eb71f0, exec=0x7fff90a4f888)
    at /home/torch-admin/project/upstream/Source/WebCore/bindings/js/JSMainThreadExecState.h:84
#1  0x00007ffff5233242 in WebCore::JSMainThreadExecState::evaluate (exec=0x7fff90a4f888, chain=0x7fff90a2ffc0, source=..., 
    thisValue=..., exception=0x7fff91eb7350)
    at /home/torch-admin/project/upstream/Source/WebCore/bindings/js/JSMainThreadExecState.h:75
#2  0x00007ffff5233400 in WebCore::InjectedScriptManager::createInjectedScript (this=0x7fff88009db0, source=..., 
    scriptState=0x7fff90a4f888, id=1) at /home/torch-admin/project/upstream/Source/WebCore/bindings/js/JSInjectedScriptManager.cpp:64
#3  0x00007ffff578801b in WebCore::InjectedScriptManager::injectScript (this=0x7fff88009db0, source=..., scriptState=0x7fff90a4f888)
    at /home/torch-admin/project/upstream/Source/WebCore/inspector/InjectedScriptManager.cpp:170
#4  0x00007ffff578823d in WebCore::InjectedScriptManager::injectedScriptFor (this=0x7fff88009db0, inspectedScriptState=0x7fff90a4f888)
    at /home/torch-admin/project/upstream/Source/WebCore/inspector/InjectedScriptManager.cpp:185
#5  0x00007ffff5861a1a in WebCore::WorkerRuntimeAgent::injectedScriptForEval (this=0x7fff88009f10, error=0x7fff91eb7810, 
    executionContextId=0x0) at /home/torch-admin/project/upstream/Source/WebCore/inspector/WorkerRuntimeAgent.cpp:64
#6  0x00007ffff582f833 in WebCore::InspectorRuntimeAgent::evaluate (this=0x7fff88009f10, errorString=0x7fff91eb7810, expression=..., 
    objectGroup=0x0, includeCommandLineAPI=0x0, doNotPauseOnExceptionsAndMuteConsole=0x7fff91eb789c, executionContextId=0x0, 
    returnByValue=0x7fff91eb789f, result=..., wasThrown=0x7fff91eb7890)
    at /home/torch-admin/project/upstream/Source/WebCore/inspector/InspectorRuntimeAgent.cpp:88
#7  0x00007ffff6215ca8 in WebCore::InspectorBackendDispatcherImpl::Runtime_evaluate (this=0x7fff880240d0, callId=23, 
    requestMessageObject=0x7fff88024190) at generated/InspectorBackendDispatcher.cpp:1357
#8  0x00007ffff623daad in WebCore::InspectorBackendDispatcherImpl::dispatch (this=0x7fff880240d0, message=...)
    at generated/InspectorBackendDispatcher.cpp:5485
#9  0x00007ffff5860e1a in WebCore::WorkerInspectorController::dispatchMessageFromFrontend (this=0x7fff88009bd0, message=...)
    at /home/torch-admin/project/upstream/Source/WebCore/inspector/WorkerInspectorController.cpp:188
#10 0x00007ffff5e0e3e5 in WebCore::dispatchOnInspectorBackendTask (context=0x7fff880008e0, message=...)
    at /home/torch-admin/project/upstream/Source/WebCore/workers/WorkerMessagingProxy.cpp:420
#11 0x00007ffff5890942 in WebCore::CrossThreadTask1<WTF::String, WTF::String const&>::performTask (this=0x1b65c60, 
    context=0x7fff880008e0) at /home/torch-admin/project/upstream/Source/WebCore/dom/CrossThreadTask.h:81
#12 0x00007ffff5e11415 in WebCore::WorkerRunLoop::Task::performTask (this=0x1b52bc0, runLoop=..., context=0x7fff880008e0)
    at /home/torch-admin/project/upstream/Source/WebCore/workers/WorkerRunLoop.cpp:228
#13 0x00007ffff5e10edf in WebCore::WorkerRunLoop::runInMode (this=0x1aa6170, context=0x7fff880008e0, predicate=..., 
    waitMode=WebCore::WorkerRunLoop::WaitForMessage) at /home/torch-admin/project/upstream/Source/WebCore/workers/WorkerRunLoop.cpp:177
#14 0x00007ffff5e10af4 in WebCore::WorkerRunLoop::run (this=0x1aa6170, context=0x7fff880008e0)
    at /home/torch-admin/project/upstream/Source/WebCore/workers/WorkerRunLoop.cpp:135
#15 0x00007ffff5e1475f in WebCore::WorkerThread::runEventLoop (this=0x1aa6140)
    at /home/torch-admin/project/upstream/Source/WebCore/workers/WorkerThread.cpp:195
......

[Peter Wang]

Created attachment 161213 [details]
Patch

[Adam Klein]

Looks fine to me, but I'm not a reviewer, so someone from the JSC side (e.g., Sam) will be the right person to r+.

[Pavel Feldman]

Comment on attachment 161213 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        No new test case.

Can this be tested?

> Source/WebCore/bindings/js/ScriptFunctionCall.cpp:40
> +#ifndef NDEBUG

Do we have debug-related behavior here?

> Source/WebCore/inspector/InjectedScriptManager.h:71
> +    explicit InjectedScriptManager(InspectedStateAccessCheck, bool = false);

bool isForWorker = false

[Peter Wang]

(In reply to comment #3)
> (From update of attachment 161213 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161213&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        No new test case.
> 
> Can this be tested?

Taking QT debug build as example, without my patch, when we click the link on the worker side-panel(refer to the attached picture), the Browser hits the assert statement. 

> > Source/WebCore/bindings/js/ScriptFunctionCall.cpp:40
> > +#ifndef NDEBUG
> 
> Do we have debug-related behavior here?
It's weird, it's not my code. It'll cleaned in the new patch.

> > Source/WebCore/inspector/InjectedScriptManager.h:71
> > +    explicit InjectedScriptManager(InspectedStateAccessCheck, bool = false);
> 
> bool isForWorker = false
ok.

[Peter Wang]

Created attachment 165341 [details]
how to reproduce this bug

[Andrey Adaikin]

Comment on attachment 161213 [details]
Patch

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

> Source/WebCore/inspector/InjectedScriptManager.h:87
> +protected:

private?

[Peter Wang]

Created attachment 165344 [details]
Patch

[Peter Wang]

JSMainThreadExecState is a wrapper of JSC API, it's seems designed just for main thread but not worker thread, so there are several assert statements in it.  In worker inspector related code, there are several mistakes that invoke interfaces of  JSMainThreadExecState from worker thread.

[Peter Wang]

(In reply to comment #6)
> (From update of attachment 161213 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161213&action=review
> 
> > Source/WebCore/inspector/InjectedScriptManager.h:87
> > +protected:
> 
> private?

yes (I really cannot remember why I declared it as "protected"), it should be "private", thx.

[Peter Wang]

(In reply to comment #7)
> Created an attachment (id=165344) [details]
> Patch

The related code are already changed a lot since the first time I uploaded this patch. I'll make a new patch that can work with current code.

[Yury Semikhatsky]

Comment on attachment 165344 [details]
Patch

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

> Source/WebCore/inspector/InjectedScriptManager.h:87
> +    bool m_isForWorker;

You don't seem to need this field. Please remove.

[Peter Wang]

Created attachment 165502 [details]
Patch

[Charles Wei]

Comment on attachment 165502 [details]
Patch

commit

[WebKit Review Bot]

Comment on attachment 165502 [details]
Patch

Clearing flags on attachment: 165502

Committed r129476: <http://trac.webkit.org/changeset/129476>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.