Bug 99801 - Web Inspector: [JSC] implement WorkerScriptDebugServer
Summary: Web Inspector: [JSC] implement WorkerScriptDebugServer
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: 100347
  Show dependency treegraph
 
Reported: 2012-10-18 20:46 PDT by Peter Wang
Modified: 2012-11-02 03:25 PDT (History)
17 users (show)

See Also:


Attachments
Patch (13.70 KB, patch)
2012-10-19 02:02 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
The test case why we need to skip "sourceParsed" in "Parser.h" (568 bytes, application/x-gzip)
2012-10-19 03:14 PDT, Peter Wang
no flags Details
Snapshot of of the worker inspector problem caused by "sourceParsed" in "Parser.h" (34.75 KB, image/png)
2012-10-19 03:15 PDT, Peter Wang
no flags Details
Patch (11.31 KB, patch)
2012-10-23 20:48 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (11.53 KB, patch)
2012-11-01 03:23 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-10-18 20:46:15 PDT
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.
Comment 1 Peter Wang 2012-10-19 02:02:51 PDT
Created attachment 169578 [details]
Patch
Comment 2 Yury Semikhatsky 2012-10-19 02:46:40 PDT
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
Comment 3 Peter Wang 2012-10-19 03:14:33 PDT
Created attachment 169586 [details]
 The test case why we need to skip "sourceParsed" in "Parser.h"
Comment 4 Peter Wang 2012-10-19 03:15:35 PDT
Created attachment 169587 [details]
 Snapshot of of the worker inspector problem caused by "sourceParsed" in "Parser.h"
Comment 5 Peter Wang 2012-10-19 03:44:15 PDT
(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.
Comment 6 Peter Wang 2012-10-23 20:48:19 PDT
Created attachment 170302 [details]
Patch
Comment 7 Peter Wang 2012-10-23 21:01:23 PDT
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.
Comment 8 Peter Wang 2012-11-01 03:23:19 PDT
Created attachment 171804 [details]
Patch
Comment 9 Peter Wang 2012-11-01 03:24:29 PDT
(In reply to comment #8)
> Created an attachment (id=171804) [details]
> Patch

rebase and import a modification to support "Pause on start".
Comment 10 Yury Semikhatsky 2012-11-02 00:59:27 PDT
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.
Comment 11 Peter Wang 2012-11-02 01:38:56 PDT
(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 12 Charles Wei 2012-11-02 03:18:07 PDT
Comment on attachment 171804 [details]
Patch

commit.
Comment 13 WebKit Review Bot 2012-11-02 03:25:06 PDT
Comment on attachment 171804 [details]
Patch

Clearing flags on attachment: 171804

Committed r133281: <http://trac.webkit.org/changeset/133281>
Comment 14 WebKit Review Bot 2012-11-02 03:25:10 PDT
All reviewed patches have been landed.  Closing bug.