Bug 93519 – Web Inspector: implement WorkerScriptDebugServer

• Component: WebCore JavaScript
• Status: RESOLVED DUPLICATE

[Hanna]

WorkerScriptDebugServer is declared but not implemented in jsc, without it the worker inspector frontend is faulty

[Hanna]

*** Bug 92652 has been marked as a duplicate of this bug. ***

[Peter Wang]

The bug 95334 and 95341 are the existing defects that cause the worker inspector for JSC unable to work, so I put them in the independent bug record to make it easier to be reviewed. 
The patch for implementation of "WorkerScriptDebugServer" will be uploaded soon.

[Peter Wang]

Created attachment 161450 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 161450 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.h:1039
> +    if (debugger && !debugger->isWorkerDebugger() && !ParsedNode::scopeIsFunction)

How does inspector get notified about parsed scripts in case of worker if is is skipped here?

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:447
> +void ScriptDebugServer::blockToWaitInstruction(bool& stop)

I'd call it runEventLoopWhilePaused

> Source/WebCore/bindings/js/ScriptDebugServer.h:117
> +    virtual void blockToWaitInstruction(bool& stop);

m_doneProcessingDebuggerEvents is a protected field, no need to pass a reference to it as an argument.

> Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp:71
> +    m_workerContext->script()->attachDebugger(this);

This should happen only when first listener is added. We should probably simplify the API as I am not aware of any cases when we may have more than one listener.

> Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp:81
> +    m_workerContext->script()->detachDebugger(this);

This should only occur when there are no listeners.

> Source/WebCore/bindings/js/WorkerScriptDebugServer.h:48
> +    bool isWorkerDebugger() { return true; }

This method should be marked virtual.

[Peter Wang]

(In reply to comment #4)
> (From update of attachment 161450 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161450&action=review
> 
> > Source/JavaScriptCore/parser/Parser.h:1039
> > +    if (debugger && !debugger->isWorkerDebugger() && !ParsedNode::scopeIsFunction)
> 
> How does inspector get notified about parsed scripts in case of worker if is is skipped here?
The "JSC::WorkerScriptDebugServer::addListener" invokes "recompileAllJSFunctions" which will make "JSC::Debugger::sourceParsed" to be called.
Actually, if we don't skip here, worker inspector will have a problem in some case(refer to the attachement).
> > Source/WebCore/bindings/js/ScriptDebugServer.cpp:447
> > +void ScriptDebugServer::blockToWaitInstruction(bool& stop)
> 
> I'd call it runEventLoopWhilePaused
ok.
> > Source/WebCore/bindings/js/ScriptDebugServer.h:117
> > +    virtual void blockToWaitInstruction(bool& stop);
> 
> m_doneProcessingDebuggerEvents is a protected field, no need to pass a reference to it as an argument.
ok, thx. 
> > Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp:71
> > +    m_workerContext->script()->attachDebugger(this);
> 
> This should happen only when first listener is added. We should probably simplify the API as I am not aware of any cases when we may have more than one listener.
yes, but JSC really needs interface "addListener" to inform of recompilation. 
> > Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp:81
> > +    m_workerContext->script()->detachDebugger(this);
> 
> This should only occur when there are no listeners.
ok 
> > Source/WebCore/bindings/js/WorkerScriptDebugServer.h:48
> > +    bool isWorkerDebugger() { return true; }
> 
> This method should be marked virtual.
ok.

[Peter Wang]

Created attachment 165974 [details]
The test case why we need to skip "sourceParsed" in "Parser.h"

[Peter Wang]

Created attachment 165975 [details]
Snapshot of of the worker inspector problem caused by "sourceParsed" in "Parser.h"

[Peter Wang]

Created attachment 165978 [details]
Patch

[Peter Wang]

However, the subject of this bug is confused, so I open a new bug#99801 to make it clear that what I'm going to do.
So this bug should be closed.

[Andreas Kling]

Comment on attachment 165978 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Joseph Pecoraro]

*** This bug has been marked as a duplicate of bug 164136 ***