Bug 131673 - Web Inspector: Move InspectorProfilerAgent to JavaScriptCore
Summary: Web Inspector: Move InspectorProfilerAgent to JavaScriptCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-04-15 01:22 PDT by Timothy Hatcher
Modified: 2014-04-24 17:10 PDT (History)
16 users (show)

See Also:


Attachments
Patch (165.28 KB, patch)
2014-04-15 01:37 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (460.40 KB, application/zip)
2014-04-15 02:47 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (463.99 KB, application/zip)
2014-04-15 03:53 PDT, Build Bot
no flags Details
Patch (166.65 KB, patch)
2014-04-15 09:24 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (461.44 KB, application/zip)
2014-04-15 10:35 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (464.86 KB, application/zip)
2014-04-15 11:44 PDT, Build Bot
no flags Details
Patch (166.29 KB, patch)
2014-04-18 14:57 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (490.19 KB, application/zip)
2014-04-18 17:27 PDT, Build Bot
no flags Details
Patch (168.03 KB, patch)
2014-04-18 17:45 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (168.23 KB, patch)
2014-04-18 18:21 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (462.13 KB, application/zip)
2014-04-18 19:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (571.23 KB, application/zip)
2014-04-18 19:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (465.77 KB, application/zip)
2014-04-18 20:35 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (518.05 KB, application/zip)
2014-04-18 21:06 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2014-04-15 01:22:25 PDT
To make the legacy profiler work with JSContext inspection, the InspectorProfilerAgent needs to move lower in the stack.
Comment 1 Radar WebKit Bug Importer 2014-04-15 01:22:50 PDT
<rdar://problem/16618200>
Comment 2 Timothy Hatcher 2014-04-15 01:37:12 PDT
Created attachment 229362 [details]
Patch
Comment 3 Build Bot 2014-04-15 02:47:16 PDT
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
Comment 4 Build Bot 2014-04-15 02:47:20 PDT
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
Comment 5 Build Bot 2014-04-15 03:53:25 PDT
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
Comment 6 Build Bot 2014-04-15 03:53:33 PDT
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
Comment 7 Timothy Hatcher 2014-04-15 09:16:19 PDT
Test failure is unrelated to my patch. Does not fail on my machine either.
Comment 8 Timothy Hatcher 2014-04-15 09:24:59 PDT
Created attachment 229372 [details]
Patch
Comment 9 Timothy Hatcher 2014-04-15 09:25:23 PDT
That should fix the windows build.
Comment 10 Timothy Hatcher 2014-04-15 10:32:04 PDT
The new Windows failures seem unrelated.
Comment 11 Build Bot 2014-04-15 10:35:30 PDT
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
Comment 12 Build Bot 2014-04-15 10:35:35 PDT
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
Comment 13 Build Bot 2014-04-15 11:44:44 PDT
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
Comment 14 Build Bot 2014-04-15 11:44:51 PDT
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
Comment 15 Joseph Pecoraro 2014-04-16 18:05:44 PDT
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.
Comment 16 Timothy Hatcher 2014-04-18 14:57:28 PDT
Created attachment 229677 [details]
Patch
Comment 17 Build Bot 2014-04-18 17:27:19 PDT
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
Comment 18 Build Bot 2014-04-18 17:27:25 PDT
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
Comment 19 Timothy Hatcher 2014-04-18 17:45:06 PDT
Created attachment 229694 [details]
Patch
Comment 20 Timothy Hatcher 2014-04-18 18:21:54 PDT
Created attachment 229697 [details]
Patch
Comment 21 Build Bot 2014-04-18 19:28:42 PDT
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
Comment 22 Build Bot 2014-04-18 19:28:48 PDT
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
Comment 23 Build Bot 2014-04-18 19:31:26 PDT
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
Comment 24 Build Bot 2014-04-18 19:31:33 PDT
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
Comment 25 Build Bot 2014-04-18 20:35:20 PDT
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
Comment 26 Build Bot 2014-04-18 20:35:27 PDT
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
Comment 27 Build Bot 2014-04-18 21:06:47 PDT
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
Comment 28 Build Bot 2014-04-18 21:06:58 PDT
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
Comment 29 Timothy Hatcher 2014-04-18 21:51:38 PDT
Any idea why fast/dom/gc-attribute-node.html would fail with this patch?
Comment 30 Timothy Hatcher 2014-04-18 21:53:39 PDT
It seem flaky. It fails sometimes and not others.
Comment 31 WebKit Commit Bot 2014-04-18 23:06:02 PDT
Comment on attachment 229697 [details]
Patch

Clearing flags on attachment: 229697

Committed r167530: <http://trac.webkit.org/changeset/167530>
Comment 32 WebKit Commit Bot 2014-04-18 23:06:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 33 Csaba Osztrogonác 2014-04-20 07:22:25 PDT
(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.
Comment 34 Joseph Pecoraro 2014-04-24 14:10:04 PDT
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.
Comment 35 Timothy Hatcher 2014-04-24 17:07:08 PDT
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.
Comment 36 Timothy Hatcher 2014-04-24 17:10:51 PDT
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