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.
Created attachment 169578 [details] Patch
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
Created attachment 169586 [details] The test case why we need to skip "sourceParsed" in "Parser.h"
Created attachment 169587 [details] Snapshot of of the worker inspector problem caused by "sourceParsed" in "Parser.h"
(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.
Created attachment 170302 [details] Patch
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.
Created attachment 171804 [details] Patch
(In reply to comment #8) > Created an attachment (id=171804) [details] > Patch rebase and import a modification to support "Pause on start".
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.
(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.
Comment on attachment 171804 [details] Patch commit.
Comment on attachment 171804 [details] Patch Clearing flags on attachment: 171804 Committed r133281: <http://trac.webkit.org/changeset/133281>
All reviewed patches have been landed. Closing bug.