Bug 95341 - Web Inspector: The JS code injected by worker inspector shouldn't be evaluated through JSMainThreadExecState
Summary: Web Inspector: The JS code injected by worker inspector shouldn't be evaluat...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 93519
  Show dependency treegraph
 
Reported: 2012-08-29 06:30 PDT by Peter Wang
Modified: 2012-10-08 14:43 PDT (History)
14 users (show)

See Also:


Attachments
Patch (5.61 KB, patch)
2012-08-29 06:38 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
how to reproduce this bug (24.75 KB, image/png)
2012-09-24 03:24 PDT, Peter Wang
no flags Details
Patch (4.28 KB, patch)
2012-09-24 03:38 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (3.19 KB, patch)
2012-09-24 20:02 PDT, Peter Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Wang 2012-08-29 06:30:12 PDT
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
......
Comment 1 Peter Wang 2012-08-29 06:38:06 PDT
Created attachment 161213 [details]
Patch
Comment 2 Adam Klein 2012-09-04 10:05:27 PDT
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+.
Comment 3 Pavel Feldman 2012-09-21 02:54:53 PDT
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
Comment 4 Peter Wang 2012-09-24 03:23:18 PDT
(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.
Comment 5 Peter Wang 2012-09-24 03:24:02 PDT
Created attachment 165341 [details]
how to reproduce this bug
Comment 6 Andrey Adaikin 2012-09-24 03:35:38 PDT
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?
Comment 7 Peter Wang 2012-09-24 03:38:37 PDT
Created attachment 165344 [details]
Patch
Comment 8 Peter Wang 2012-09-24 03:40:00 PDT
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.
Comment 9 Peter Wang 2012-09-24 04:44:36 PDT
(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.
Comment 10 Peter Wang 2012-09-24 05:06:59 PDT
(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.
Comment 11 Yury Semikhatsky 2012-09-24 08:26:58 PDT
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.
Comment 12 Peter Wang 2012-09-24 20:02:28 PDT
Created attachment 165502 [details]
Patch
Comment 13 Charles Wei 2012-09-25 01:18:29 PDT
Comment on attachment 165502 [details]
Patch

commit
Comment 14 WebKit Review Bot 2012-09-25 01:39:22 PDT
Comment on attachment 165502 [details]
Patch

Clearing flags on attachment: 165502

Committed r129476: <http://trac.webkit.org/changeset/129476>
Comment 15 WebKit Review Bot 2012-09-25 01:39:26 PDT
All reviewed patches have been landed.  Closing bug.