Bug 131673 – Web Inspector: Move InspectorProfilerAgent to JavaScriptCore

• Component: Web Inspector
• Status: RESOLVED FIXED

[Timothy Hatcher]

To make the legacy profiler work with JSContext inspection, the InspectorProfilerAgent needs to move lower in the stack.

[Radar WebKit Bug Importer]

<rdar://problem/16618200>

[Timothy Hatcher]

Created attachment 229362 [details]
Patch

[Build Bot]

Comment on attachment 229362 [details]
Patch

Attachment 229362 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5227490024882176

New failing tests:
fast/dom/gc-attribute-node.html

[Build Bot]

Created attachment 229364 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 229362 [details]
Patch

Attachment 229362 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4866719214469120

New failing tests:
fast/dom/gc-attribute-node.html

[Build Bot]

Created attachment 229365 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Timothy Hatcher]

Test failure is unrelated to my patch. Does not fail on my machine either.

[Timothy Hatcher]

Created attachment 229372 [details]
Patch

[Timothy Hatcher]

That should fix the windows build.

[Timothy Hatcher]

The new Windows failures seem unrelated.

[Build Bot]

Comment on attachment 229372 [details]
Patch

Attachment 229372 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4670146077523968

New failing tests:
fast/dom/gc-attribute-node.html

[Build Bot]

Created attachment 229376 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 229372 [details]
Patch

Attachment 229372 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6229872041000960

New failing tests:
fast/dom/gc-attribute-node.html

[Build Bot]

Created attachment 229387 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Joseph Pecoraro]

Comment on attachment 229372 [details]
Patch

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

Looks good to me! r=me

> Source/JavaScriptCore/inspector/agents/InspectorProfilerAgent.cpp:224
> +void InspectorProfilerAgent::didCreateFrontendAndBackend(InspectorFrontendChannel* frontendChannel, InspectorBackendDispatcher* backendDispatcher)
> +{
> +    m_frontendDispatcher = std::make_unique<InspectorProfilerFrontendDispatcher>(frontendChannel);
> +    m_backendDispatcher = InspectorProfilerBackendDispatcher::create(backendDispatcher, this);
> +}
> +
> +void InspectorProfilerAgent::willDestroyFrontendAndBackend(InspectorDisconnectReason reason)
> +{
> +    m_frontendDispatcher = nullptr;
> +    m_backendDispatcher.clear();
> +
> +    reset();
> +
> +    disable(reason == InspectorDisconnectReason::InspectedTargetDestroyed ? SkipRecompile : Recompile);
> +}

Nit: I like to have these methods up near the constructor so they are easy to find.

> Source/JavaScriptCore/inspector/agents/JSGlobalObjectProfilerAgent.cpp:37
> +JSGlobalObjectProfilerAgent::JSGlobalObjectProfilerAgent(JSC::JSGlobalObject& globalObject)

Nit: You can drop the "JSC::" here.

> Source/JavaScriptCore/inspector/agents/JSGlobalObjectProfilerAgent.h:32
> +#include <wtf/PassOwnPtr.h>

Nit: I do not think this include is needed.

> Source/JavaScriptCore/inspector/agents/JSGlobalObjectProfilerAgent.h:45
> +    virtual JSC::ExecState* profilingGlobalExecState() const override final;

Nit: Final is already on the class, we can remove it from here.

> Source/WebCore/inspector/InspectorController.cpp:76
> +#include <inspector/agents/InspectorProfilerAgent.h>

Nit: unnecessary include.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:962
> +    return nullptr;

Maybe this should return "String()" instead of nullptr.

> Source/WebCore/inspector/PageProfilerAgent.h:45
> +    virtual JSC::ExecState* profilingGlobalExecState() const override final;

Nit: Lets put final on the class instead of this method.

> Source/WebCore/inspector/PageProfilerAgent.h:47
> +    Page* m_inspectedPage;

Nit: Page&?

> Source/WebCore/inspector/WebProfilerAgent.h:36
> +typedef String ErrorString;

Nit: Unnecessary forward declaration.

> Source/WebCore/inspector/WorkerInspectorController.cpp:99
> +    profilerAgent->setScriptDebugServer(&debuggerAgent->scriptDebugServer());

Oops. Though I'm not sure if we actually need this in a worker agent. Better to have it then not.

> Source/WebCore/inspector/WorkerProfilerAgent.h:45
> +    virtual JSC::ExecState* profilingGlobalExecState() const override final;

Nit: final on class instead of method.

> Source/WebCore/page/PageConsole.cpp:170
>      if (!InspectorInstrumentation::profilerEnabled(&m_page))
>          return;
>  

This seems unnecessary. Since the InspectorInstrumentation::startProfiling won't do anything unless the profiler is enabled.

> Source/WebCore/page/PageConsole.cpp:179
> +    RefPtr<JSC::Profile> profile = InspectorInstrumentation::stopProfiling(&m_page, exec, title);

Likewise.

[Timothy Hatcher]

Created attachment 229677 [details]
Patch

[Build Bot]

Comment on attachment 229677 [details]
Patch

Attachment 229677 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5359042188804096

New failing tests:
fast/dom/gc-attribute-node.html

[Build Bot]

Created attachment 229690 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Timothy Hatcher]

Created attachment 229694 [details]
Patch

[Timothy Hatcher]

Created attachment 229697 [details]
Patch

[Build Bot]

Comment on attachment 229697 [details]
Patch

Attachment 229697 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6506334841208832

New failing tests:
fast/dom/gc-attribute-node.html

[Build Bot]

Created attachment 229702 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 229697 [details]
Patch

Attachment 229697 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4743044607770624

New failing tests:
fast/dom/gc-attribute-node.html

[Build Bot]

Created attachment 229703 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 229697 [details]
Patch

Attachment 229697 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5995484938240000

New failing tests:
fast/dom/gc-attribute-node.html

[Build Bot]

Created attachment 229708 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 229697 [details]
Patch

Attachment 229697 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5764422190497792

New failing tests:
fast/dom/gc-attribute-node.html

[Build Bot]

Created attachment 229712 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Timothy Hatcher]

Any idea why fast/dom/gc-attribute-node.html would fail with this patch?

[Timothy Hatcher]

It seem flaky. It fails sometimes and not others.

[WebKit Commit Bot]

Comment on attachment 229697 [details]
Patch

Clearing flags on attachment: 229697

Committed r167530: <http://trac.webkit.org/changeset/167530>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #31)
> (From update of attachment 229697 [details])
> Clearing flags on attachment: 229697
> 
> Committed r167530: <http://trac.webkit.org/changeset/167530>

It broke the build on the Apple Windows debug bot.

[Joseph Pecoraro]

Comment on attachment 229697 [details]
Patch

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

> Source/WebCore/inspector/PageDebuggerAgent.cpp:-71
> -void PageDebuggerAgent::enable()
> -{
> -    WebDebuggerAgent::enable();
> -    m_instrumentingAgents->setPageDebuggerAgent(this);
> -}
> -
> -void PageDebuggerAgent::disable(bool isBeingDestroyed)
> -{
> -    WebDebuggerAgent::disable(isBeingDestroyed);
> -    m_instrumentingAgents->setPageDebuggerAgent(nullptr);
> -}

Why did these get removed? Now nothing sets the instrumenting agents pageDebuggerAgent. We rely on that so that on page changes we do handling for clearing the global object.

[Timothy Hatcher]

Comment on attachment 229697 [details]
Patch

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

>> Source/WebCore/inspector/PageDebuggerAgent.cpp:-71
>> -}
> 
> Why did these get removed? Now nothing sets the instrumenting agents pageDebuggerAgent. We rely on that so that on page changes we do handling for clearing the global object.

I thought it wasn't needed because I saw WebDebuggerAgent doing "the same thing". But WebDebuggerAgent calls setInspectorDebuggerAgent and this calls setPageDebuggerAgent. As far as I can tell, pageDebuggerAgent is needed to avoid a cast or other type check. I'll restore this code.

[Timothy Hatcher]

Comment on attachment 229697 [details]
Patch

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

>>> Source/WebCore/inspector/PageDebuggerAgent.cpp:-71
>>> -}
>> 
>> Why did these get removed? Now nothing sets the instrumenting agents pageDebuggerAgent. We rely on that so that on page changes we do handling for clearing the global object.
> 
> I thought it wasn't needed because I saw WebDebuggerAgent doing "the same thing". But WebDebuggerAgent calls setInspectorDebuggerAgent and this calls setPageDebuggerAgent. As far as I can tell, pageDebuggerAgent is needed to avoid a cast or other type check. I'll restore this code.

Patch to add them back: https://bugs.webkit.org/attachment.cgi?id=230121&action=review