WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
99801
Web Inspector: [JSC] implement WorkerScriptDebugServer
https://bugs.webkit.org/show_bug.cgi?id=99801
Summary
Web Inspector: [JSC] implement WorkerScriptDebugServer
Peter Wang
Reported
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.
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Peter Wang
Comment 1
2012-10-19 02:02:51 PDT
Created
attachment 169578
[details]
Patch
Yury Semikhatsky
Comment 2
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
Peter Wang
Comment 3
2012-10-19 03:14:33 PDT
Created
attachment 169586
[details]
The test case why we need to skip "sourceParsed" in "Parser.h"
Peter Wang
Comment 4
2012-10-19 03:15:35 PDT
Created
attachment 169587
[details]
Snapshot of of the worker inspector problem caused by "sourceParsed" in "Parser.h"
Peter Wang
Comment 5
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.
Peter Wang
Comment 6
2012-10-23 20:48:19 PDT
Created
attachment 170302
[details]
Patch
Peter Wang
Comment 7
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.
Peter Wang
Comment 8
2012-11-01 03:23:19 PDT
Created
attachment 171804
[details]
Patch
Peter Wang
Comment 9
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".
Yury Semikhatsky
Comment 10
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.
Peter Wang
Comment 11
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.
Charles Wei
Comment 12
2012-11-02 03:18:07 PDT
Comment on
attachment 171804
[details]
Patch commit.
WebKit Review Bot
Comment 13
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
>
WebKit Review Bot
Comment 14
2012-11-02 03:25:10 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug