Web Inspector: [JSC] implement WorkerScriptDebugServer

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

[Peter Wang]

There is a bug#93519 about it already. However, the subject of bug#93519 is a little confused, so I open a new bug to make it clear.

[Peter Wang]

Created attachment 169578 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 169578 [details]
Patch

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

Inspector part of the change looks good to me except a few minor things. I am pretty sure we should be able to treat page and worker debuggers similarly in Source/JavaScriptCore/debugger but I'm not an expert in JSC so you need someone working on JSC to review that part.

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

I still don't understand why we should treat parse events differently in case of workers. We need someone familiar with JSC code to look at this part.

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:370
> +        dispatchDidParseSource(*listeners, sourceProvider, isWorkerDebugger() ? false : isContentScript(exec));

Please make isContentScript a virtual method on ScriptDebugServer and provide appropriate implementations for Page/WorkerScriptDebug instead of introducing isWorkerDebugger.

> Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp:58
> +    // If JavaScript stack is not empty postpone recompilation.

It seems that we can safely assume the JS stack is empty if recompileAllJSFunctions is invoked by Timer but this is a different issue.

> Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp:73
> +    recompileAllJSFunctions(0);

recompileAllJSFunctionsSoon

[Peter Wang]

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

[Peter Wang]

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

[Peter Wang]

(In reply to comment #2)
> (From update of attachment 169578 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169578&action=review
> 
> Inspector part of the change looks good to me except a few minor things. I am pretty sure we should be able to treat page and worker debuggers similarly in Source/JavaScriptCore/debugger but I'm not an expert in JSC so you need someone working on JSC to review that part.
> 
> > Source/JavaScriptCore/parser/Parser.h:998
> > +    if (debugger && !debugger->isWorkerDebugger() && !ParsedNode::scopeIsFunction)
> 
> I still don't understand why we should treat parse events differently in case of workers. We need someone familiar with JSC code to look at this part.
> 
Because when Inspector is attached, the JS code is demanded to be recompiled, and JSC send the code to Inspector ("debugger!=null" means Inspector is attached) by these statements of "Parser.h". If we inspecting a normal web page, JS code just recompiled once, and then the "compiled"(not exactly) code is executed, the related statements of "Parser.h" is never be invoked, so  Inspector has only one copy of code. 
But for worker js with timer, there is a problem.

> > Source/WebCore/bindings/js/ScriptDebugServer.cpp:370
> > +        dispatchDidParseSource(*listeners, sourceProvider, isWorkerDebugger() ? false : isContentScript(exec));
> 
> Please make isContentScript a virtual method on ScriptDebugServer and provide appropriate implementations for Page/WorkerScriptDebug instead of introducing isWorkerDebugger.
> 
ok 
> > Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp:58
> > +    // If JavaScript stack is not empty postpone recompilation.
> 
> It seems that we can safely assume the JS stack is empty if recompileAllJSFunctions is invoked by Timer but this is a different issue.
> 
Yes, it's another issue of JSC. So far, we can just use these code as "PageScriptDebugeServer". Actually, the statement 
"if (JSDOMWindow::commonJSGlobalData()->dynamicGlobalObject)" has problem when there are two pages in one process, and being Inspected in same time.

> > Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp:73
> > +    recompileAllJSFunctions(0);
> 
> recompileAllJSFunctionsSoon
sorry, thx.

[Peter Wang]

Created attachment 170302 [details]
Patch

[Peter Wang]

In the new patch, what I did according to these comments:


(In reply to comment #5)
> (In reply to comment #2)
> > (From update of attachment 169578 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=169578&action=review
> > 
> > Inspector part of the change looks good to me except a few minor things. I am pretty sure we should be able to treat page and worker debuggers similarly in Source/JavaScriptCore/debugger but I'm not an expert in JSC so you need someone working on JSC to review that part.
> > 
> > > Source/JavaScriptCore/parser/Parser.h:998
> > > +    if (debugger && !debugger->isWorkerDebugger() && !ParsedNode::scopeIsFunction)
> > 
> > I still don't understand why we should treat parse events differently in case of workers. We need someone familiar with JSC code to look at this part.
> > 
> Because when Inspector is attached, the JS code is demanded to be recompiled, and JSC send the code to Inspector ("debugger!=null" means Inspector is attached) by these statements of "Parser.h". If we inspecting a normal web page, JS code just recompiled once, and then the "compiled"(not exactly) code is executed, the related statements of "Parser.h" is never be invoked, so  Inspector has only one copy of code. 
> But for worker js with timer, there is a problem.

Based no newest code, good news is now the worker JS code doesn't recompiled every executed, bad news is I cannot find which patch do me the favour.
I'm investigating, at least it doesn't bother us so far.
 
> > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:370
> > > +        dispatchDidParseSource(*listeners, sourceProvider, isWorkerDebugger() ? false : isContentScript(exec));
> > 
> > Please make isContentScript a virtual method on ScriptDebugServer and provide appropriate implementations for Page/WorkerScriptDebug instead of introducing isWorkerDebugger.
> > 
> ok 
I did it as your comment.
> > > Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp:58
> > > +    // If JavaScript stack is not empty postpone recompilation.
> > 
> > It seems that we can safely assume the JS stack is empty if recompileAllJSFunctions is invoked by Timer but this is a different issue.
> > 
> Yes, it's another issue of JSC. So far, we can just use these code as "PageScriptDebugeServer". Actually, the statement 
> "if (JSDOMWindow::commonJSGlobalData()->dynamicGlobalObject)" has problem when there are two pages in one process, and being Inspected in same time.
> 
> > > Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp:73
> > > +    recompileAllJSFunctions(0);
> > 
> > recompileAllJSFunctionsSoon
> sorry, thx.
I checked code again, I really need "recompileAllJSFunctions(0)", it's a protect wrapper. We're not always sure the situation of JSC engine when it's invoked here.

[Peter Wang]

Created attachment 171804 [details]
Patch

[Peter Wang]

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

rebase and import a modification to support "Pause on start".

[Yury Semikhatsky]

Comment on attachment 171804 [details]
Patch

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

Consider adding a test for the worker debugging functionality. Now that we have support for pure protocol tests (kudos to vivekg) it shouldn't be hard to test worker debugging functionality. See for example the test I recently added for workers: LayoutTests/inspector-protocol/debugger-pause-dedicated-worker.html

> Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp:61
> +    recompileAllJSFunctions(0);

recompileAllJSFunctions should only be called when the first listener is being added. It seems that we need to fix this in PageSriptDebugServer as well.

[Peter Wang]

(In reply to comment #10)
> (From update of attachment 171804 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171804&action=review
> 
> Consider adding a test for the worker debugging functionality. Now that we have support for pure protocol tests (kudos to vivekg) it shouldn't be hard to test worker debugging functionality. See for example the test I recently added for workers: LayoutTests/inspector-protocol/debugger-pause-dedicated-worker.html
ok, I'll do it ASAP.

> > Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp:61
> > +    recompileAllJSFunctions(0);
> 
> recompileAllJSFunctions should only be called when the first listener is being added. It seems that we need to fix this in PageSriptDebugServer as well.

Yes, I also find it's a vulnerable part. The problem becomes more obvious when there is a timer in worker: seems inspector cannot distinguish if the JS code is updated or just be executed again by timer. JSC is OK so far just because it doesn’t recompile the js code for timer of worker.  I'm investigating it now.

Thank you for your time, sir.

[Charles Wei]

Comment on attachment 171804 [details]
Patch

commit.

[WebKit Review Bot]

Comment on attachment 171804 [details]
Patch

Clearing flags on attachment: 171804

Committed r133281: <http://trac.webkit.org/changeset/133281>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.