Bug 92342 - Web Inspector [JSC]: Support multi instance for PageScriptDebugServer of JSC
Summary: Web Inspector [JSC]: Support multi instance for PageScriptDebugServer of JSC
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:
 
Reported: 2012-07-25 23:54 PDT by Peter Wang
Modified: 2014-12-11 09:51 PST (History)
21 users (show)

See Also:


Attachments
Patch (7.38 KB, patch)
2012-07-26 03:31 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (7.46 KB, patch)
2012-07-26 04:04 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (8.11 KB, patch)
2012-07-26 04:29 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (13.02 KB, patch)
2012-07-27 03:16 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (13.08 KB, patch)
2012-07-29 18:34 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (17.05 KB, patch)
2012-07-29 19:11 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (17.06 KB, patch)
2012-07-29 20:16 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (17.07 KB, patch)
2012-07-29 23:48 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (17.59 KB, patch)
2012-07-30 01:45 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (18.12 KB, patch)
2012-07-30 04:25 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (17.93 KB, patch)
2012-07-30 05:13 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (25.57 KB, patch)
2012-08-06 05:08 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (15.00 KB, patch)
2012-10-12 04:11 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-07-25 23:54:53 PDT
PageScriptDebugServer is singleton now. That's right for V8, since PageScriptDebugServer is a dispatcher for V8, it doesn't record any debug status for a special page or JSGlobalObject (frame). 
But for JSC things are different, the derived chain of PageScriptDebugServer is JSC::Debugger -> WebCore::ScriptDebugServer -> WebCore::PageScriptDebugServer, the ScriptDebugServer records status for one JSGlobalObject (refer to member variables "m_pauseOnNextStatement" and "m_paused"). It causes a problem that if there more than on pages within one process only one can be inspected. 
In high level, the InspectorController and PageDebugAgent are one-one mapped to page. So there is a possibility to let JSC's PageScriptDebugServer support multi instance.
Comment 1 Peter Wang 2012-07-26 03:31:39 PDT
Created attachment 154602 [details]
Patch
Comment 2 Early Warning System Bot 2012-07-26 03:51:58 PDT
Comment on attachment 154602 [details]
Patch

Attachment 154602 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13353647
Comment 3 Peter Wang 2012-07-26 04:04:57 PDT
Created attachment 154606 [details]
Patch
Comment 4 WebKit Review Bot 2012-07-26 04:19:34 PDT
Comment on attachment 154606 [details]
Patch

Attachment 154606 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13349679
Comment 5 Peter Wang 2012-07-26 04:29:54 PDT
Created attachment 154609 [details]
Patch
Comment 6 Yury Semikhatsky 2012-07-26 05:50:58 PDT
Comment on attachment 154609 [details]
Patch

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

> Source/WebCore/bindings/js/PageScriptDebugServer.cpp:68
> +    if (!page)

How can it happen?

> Source/WebCore/bindings/js/PageScriptDebugServer.cpp:86
> +    m_pageDebugServerMap.add(page, serverForPage);

Pleas move this line into PageScriptDebugServer constructor.

> Source/WebCore/bindings/js/PageScriptDebugServer.cpp:198
> +    deleteInstance(page);

I'd replace this with delete this; and get rid of PageScriptDebugServer::deleteInstance

> Source/WebCore/bindings/v8/PageScriptDebugServer.cpp:67
> +PageScriptDebugServer& PageScriptDebugServer::shared(Page*)

Please rename this method to something like forPage as now there is no shared instance.

> Source/WebCore/inspector/PageDebuggerAgent.cpp:72
> +    if (PageScriptDebugServer::isMultiInstance())

No need in this check just ignore the Page* argument in case of v8 and delete isMultiInstance.
Comment 7 Peter Wang 2012-07-27 03:07:53 PDT
(In reply to comment #6)
> (From update of attachment 154609 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154609&action=review
> 
> > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:68
> > +    if (!page)
> 
> How can it happen?
For InspectorProfilerAgent, I planned just changed the behavior of InspectorDebugAgent firstly to be on safe side. But it seems confused, I'll correct it in new patch.  
> > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:86
> > +    m_pageDebugServerMap.add(page, serverForPage);
> 
> Pleas move this line into PageScriptDebugServer constructor.
ok. Thx, it's better.
> > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:198
> > +    deleteInstance(page);
> 
> I'd replace this with delete this; and get rid of PageScriptDebugServer::deleteInstance
ok. Thx, it's better.
> > Source/WebCore/bindings/v8/PageScriptDebugServer.cpp:67
> > +PageScriptDebugServer& PageScriptDebugServer::shared(Page*)
> 
> Please rename this method to something like forPage as now there is no shared instance.
> 
ok.
> > Source/WebCore/inspector/PageDebuggerAgent.cpp:72
> > +    if (PageScriptDebugServer::isMultiInstance())
> 
> No need in this check just ignore the Page* argument in case of v8 and delete isMultiInstance.
ok.
Comment 8 Peter Wang 2012-07-27 03:16:20 PDT
Created attachment 154892 [details]
Patch
Comment 9 WebKit Review Bot 2012-07-27 04:58:47 PDT
Comment on attachment 154892 [details]
Patch

Attachment 154892 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13373341
Comment 10 Build Bot 2012-07-27 08:28:09 PDT
Comment on attachment 154892 [details]
Patch

Attachment 154892 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13368549
Comment 11 Peter Wang 2012-07-29 18:34:23 PDT
Created attachment 155193 [details]
Patch
Comment 12 WebKit Review Bot 2012-07-29 18:42:41 PDT
Comment on attachment 155193 [details]
Patch

Attachment 155193 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13395155
Comment 13 Peter Wang 2012-07-29 19:11:52 PDT
Created attachment 155196 [details]
Patch
Comment 14 Build Bot 2012-07-29 19:23:47 PDT
Comment on attachment 155196 [details]
Patch

Attachment 155196 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13392202
Comment 15 Peter Wang 2012-07-29 20:16:00 PDT
Created attachment 155200 [details]
Patch
Comment 16 Build Bot 2012-07-29 22:28:45 PDT
Comment on attachment 155200 [details]
Patch

Attachment 155200 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13380968
Comment 17 Peter Wang 2012-07-29 23:48:53 PDT
Created attachment 155214 [details]
Patch
Comment 18 Yury Semikhatsky 2012-07-29 23:51:16 PDT
Comment on attachment 155200 [details]
Patch

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

> Source/WebCore/bindings/js/PageScriptDebugServer.cpp:64
> +PageScriptDebugServer& PageScriptDebugServer::server(Page* page)

Remove this method as PageScriptDebugServer::instance does the same things.

> Source/WebCore/bindings/js/PageScriptDebugServer.cpp:161
> +    ASSERT(page && page == m_page);

What about the comment above about page being null in some cases?

> Source/WebCore/bindings/js/PageScriptDebugServer.h:53
> +    static void addListener(ScriptDebugListener*, Page*);

add/removeListener can be instance methods and we could keep the list of listeners on a corresponding PageScriptDebugServer, no need to have additional static map.
Comment 19 Peter Wang 2012-07-30 00:29:59 PDT
(In reply to comment #18)
> (From update of attachment 155200 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155200&action=review
> 
> > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:64
> > +PageScriptDebugServer& PageScriptDebugServer::server(Page* page)
> 
> Remove this method as PageScriptDebugServer::instance does the same things.

Ok. Maybe I can remove "instance()", "server()" is left for invoked by high level invoking like PageDebugAgent::scriptDebugServer since V8 has the same interface.
 
> > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:161
> > +    ASSERT(page && page == m_page);
> 
> What about the comment above about page being null in some cases?

In ideal situation, if PageScriptDebugServer exists, the "m_page" shouldn't be null and the "page" read by ScriptDebugServer from callFrame should be same.
But if there is some accident takes place in released version, I want PageScriptDebugServer to do nothing just return.
Do you think it's appropriate?  

> > Source/WebCore/bindings/js/PageScriptDebugServer.h:53
> > +    static void addListener(ScriptDebugListener*, Page*);
> 
> add/removeListener can be instance methods and we could keep the list of listeners on a corresponding PageScriptDebugServer, no need to have additional static map.
Yes, thanks. Now for JSC::PageScriptDebugServer the argument "Page" can also be ignored.
Comment 20 Peter Wang 2012-07-30 01:45:50 PDT
Created attachment 155233 [details]
Patch
Comment 21 Build Bot 2012-07-30 01:58:20 PDT
Comment on attachment 155233 [details]
Patch

Attachment 155233 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13392285
Comment 22 Yury Semikhatsky 2012-07-30 02:25:00 PDT
Comment on attachment 155233 [details]
Patch

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

> Source/WebCore/bindings/js/PageScriptDebugServer.h:59
> +    PageScriptDebugServer(Page* = 0);

The constructor should be marked "explicit" also why is the Page parameter 0 by default while it should never be null?

> Source/WebCore/bindings/js/PageScriptDebugServer.h:76
> +    static PageDebugServerMap m_pageDebugServerMap;

m_pageDebugServerMap -> s_pageDebugServerMap as it is a static field.

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:106
> +        return PageScriptDebugServer::server(0);

Worker*Agent must not depend on Page*Server. There should be a separate map WorkerContext -> WorkerScriptDebugServer.

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:-105
> -            PageScriptDebugServer::shared().continueProgram();

You don't need to rename this getter for V8 bindings as the patch doesn't affect their functionality.
Comment 23 Peter Wang 2012-07-30 02:53:54 PDT
(In reply to comment #22)
> (From update of attachment 155233 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155233&action=review
> 
> > Source/WebCore/bindings/js/PageScriptDebugServer.h:59
> > +    PageScriptDebugServer(Page* = 0);
> 
> The constructor should be marked "explicit" 
Sure. Thx.
> also why is the Page parameter 0 by default while it should never be null?
Sorry. It comes from old patch. 
> > Source/WebCore/bindings/js/PageScriptDebugServer.h:76
> > +    static PageDebugServerMap m_pageDebugServerMap;
> 
> m_pageDebugServerMap -> s_pageDebugServerMap as it is a static field.
ok.
> > Source/WebCore/inspector/InspectorProfilerAgent.cpp:106
> > +        return PageScriptDebugServer::server(0);
> 
> Worker*Agent must not depend on Page*Server. There should be a separate map WorkerContext -> WorkerScriptDebugServer.
I see, thank you. 
> > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:-105
> > -            PageScriptDebugServer::shared().continueProgram();
> 
> You don't need to rename this getter for V8 bindings as the patch doesn't affect their functionality.
Sorry, I didn't understand this comment very well. 
I renamed "JSC::PageScriptDebugServer::shared" to "server", and I had to rename "V8::PageScriptDebugServer::shared" to "server", because if not, high level function like PageDebuggerAgent::scriptDebugServer have to recognize now is V8 or JSC. So I also modified WebDevToolsAgentImpl.cpp to make it pass compiling.
Comment 24 Peter Wang 2012-07-30 04:25:04 PDT
Created attachment 155252 [details]
Patch
Comment 25 Build Bot 2012-07-30 04:31:56 PDT
Comment on attachment 155252 [details]
Patch

Attachment 155252 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13384544
Comment 26 Peter Wang 2012-07-30 05:13:48 PDT
Created attachment 155267 [details]
Patch
Comment 27 Peter Wang 2012-07-30 18:49:32 PDT
(In reply to comment #26)
> Created an attachment (id=155267) [details]
> Patch

I put the "s_pageDebugServerMap" into function "pageServerMap" to avoid break the rules of mac porting. As the comments #14 #16 #21 #25 show, mac porting seems doesn't permit to define global variables and takes static member variable as global variable also.
Comment 28 Yury Semikhatsky 2012-08-01 07:43:45 PDT
Comment on attachment 155267 [details]
Patch

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

> Source/WebCore/bindings/js/PageScriptDebugServer.cpp:105
>  void PageScriptDebugServer::removeListener(ScriptDebugListener* listener, Page* page)

We can make v8's implementation also keep a pointer to the Page and remove the second argument from the method. Can be done in another patch.

> Source/WebCore/bindings/js/PageScriptDebugServer.cpp:143
> +    if (page && page == m_page)

I don't think we need page == m_page check as PageScriptDebugServer listens to only one page.

> Source/WebCore/bindings/js/PageScriptDebugServer.cpp:152
> +    if (page && page == m_page)

Ditto.

> Source/WebCore/bindings/js/PageScriptDebugServer.h:52
> +    static PageDebugServerMap& pageServerMap();

This one should be private.

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:107
> +        ScriptDebugServer* server = new WorkerScriptDebugServer(m_workerContext);

It will create WorkerScriptDebugServer that is never deleted.

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:235
> +    scriptDebugServer().recompileAllJSFunctionsSoon();

Can we avoid creating ScriptDebugServer just to recompile js functions?
Comment 29 Peter Wang 2012-08-06 04:53:49 PDT
(In reply to comment #28)
> (From update of attachment 155267 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155267&action=review
> 
> > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:105
> >  void PageScriptDebugServer::removeListener(ScriptDebugListener* listener, Page* page)
> 
> We can make v8's implementation also keep a pointer to the Page and remove the second argument from the method. Can be done in another patch.
ok. 
> > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:143
> > +    if (page && page == m_page)
> 
> I don't think we need page == m_page check as PageScriptDebugServer listens to only one page.
> 
> > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:152
> > +    if (page && page == m_page)
> 
> ok.
> 
> > Source/WebCore/bindings/js/PageScriptDebugServer.h:52
> > +    static PageDebugServerMap& pageServerMap();
> 
> This one should be private.
> 
> > Source/WebCore/inspector/InspectorProfilerAgent.cpp:107
> > +        ScriptDebugServer* server = new WorkerScriptDebugServer(m_workerContext);
> 
> It will create WorkerScriptDebugServer that is never deleted.
It's here just for pass compiling. In my new patch I removed  "scriptDebugServer()" from "InspectorProfilerAgent", so we don't need it anymore.
 
> > Source/WebCore/inspector/InspectorProfilerAgent.cpp:235
> > +    scriptDebugServer().recompileAllJSFunctionsSoon();
> 
> Can we avoid creating ScriptDebugServer just to recompile js functions?
Thx. It's a great idea. I will have a try.
Comment 30 Peter Wang 2012-08-06 05:08:08 PDT
Created attachment 156665 [details]
Patch
Comment 31 Peter Wang 2012-08-06 05:15:23 PDT
(In reply to comment #30)
> Created an attachment (id=156665) [details]
> Patch

According to comments #28, in this patch I removed "m_recompileTimer" from "JSC::ScriptDebugServer", so that we don't need to create a "ScriptDebugServer" instance for "InspectorProfileAgent".
Comment 32 Pavel Feldman 2012-09-21 02:49:35 PDT
@yurys, are you looking at this?
Comment 33 Peter Wang 2012-09-24 05:33:07 PDT
The related code of this patch has changed a lot recently :(, I'll check if this bug is worth to exist. If yes, I'll upload a new patch that can work with new code.
Comment 34 Yury Semikhatsky 2012-09-24 08:22:54 PDT
(In reply to comment #33)
> The related code of this patch has changed a lot recently :(, I'll check if this bug is worth to exist. If yes, I'll upload a new patch that can work with new code.

Clearing r? until the new version.
Comment 35 Peter Wang 2012-10-12 04:11:50 PDT
Created attachment 168389 [details]
Patch
Comment 36 Peter Wang 2012-10-12 04:19:36 PDT
(In reply to comment #29)
> (In reply to comment #28)
> > (From update of attachment 155267 [details] [details])
......
> > Can we avoid creating ScriptDebugServer just to recompile js functions?
> Thx. It's a great idea. I will have a try.
I investigate it again recently, we cannot do that, because all JS code (of different pages) share the Global Data space, so when we are recompiling we have to know which part needs to be recompiled.
Comment 37 Pavel Feldman 2013-03-19 09:54:11 PDT
Comment on attachment 168389 [details]
Patch

Clearing r? - it has been there for ages, probably obsolete.