Bug 92342 – Web Inspector [JSC]: Support multi instance for PageScriptDebugServer of JSC

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

[Peter Wang]

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.

[Peter Wang]

Created attachment 154602 [details]
Patch

[Early Warning System Bot]

Comment on attachment 154602 [details]
Patch

Attachment 154602 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13353647

[Peter Wang]

Created attachment 154606 [details]
Patch

[WebKit Review Bot]

Comment on attachment 154606 [details]
Patch

Attachment 154606 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13349679

[Peter Wang]

Created attachment 154609 [details]
Patch

[Yury Semikhatsky]

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.

[Peter Wang]

(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.

[Peter Wang]

Created attachment 154892 [details]
Patch

[WebKit Review Bot]

Comment on attachment 154892 [details]
Patch

Attachment 154892 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13373341

[Build Bot]

Comment on attachment 154892 [details]
Patch

Attachment 154892 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13368549

[Peter Wang]

Created attachment 155193 [details]
Patch

[WebKit Review Bot]

Comment on attachment 155193 [details]
Patch

Attachment 155193 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13395155

[Peter Wang]

Created attachment 155196 [details]
Patch

[Build Bot]

Comment on attachment 155196 [details]
Patch

Attachment 155196 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13392202

[Peter Wang]

Created attachment 155200 [details]
Patch

[Build Bot]

Comment on attachment 155200 [details]
Patch

Attachment 155200 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13380968

[Peter Wang]

Created attachment 155214 [details]
Patch

[Yury Semikhatsky]

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.

[Peter Wang]

(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.

[Peter Wang]

Created attachment 155233 [details]
Patch

[Build Bot]

Comment on attachment 155233 [details]
Patch

Attachment 155233 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13392285

[Yury Semikhatsky]

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.

[Peter Wang]

(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.

[Peter Wang]

Created attachment 155252 [details]
Patch

[Build Bot]

Comment on attachment 155252 [details]
Patch

Attachment 155252 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13384544

[Peter Wang]

Created attachment 155267 [details]
Patch

[Peter Wang]

(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.

[Yury Semikhatsky]

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?

[Peter Wang]

(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.

[Peter Wang]

Created attachment 156665 [details]
Patch

[Peter Wang]

(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".

[Pavel Feldman]

@yurys, are you looking at this?

[Peter Wang]

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.

[Yury Semikhatsky]

(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.

[Peter Wang]

Created attachment 168389 [details]
Patch

[Peter Wang]

(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.

[Pavel Feldman]

Comment on attachment 168389 [details]
Patch

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