Bug 134643 – Web Inspector: Profiler disappeared

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

The profile which used to live in the timeline tab is now gone completely. Starting profiles programmatically is also impossible; console.profile() returns undefined

[Radar WebKit Bug Importer]

<rdar://problem/17566028>

[Timothy Hatcher]

Profiles are now part of the Timeline and are captured when the timeline is recording. To see the profile data, click on the JavaScript and Events timeline which will show a data grid of all the recorded profiles.

Indeed, console.profile() is not working and I am working on making it record into the the timeline.

[Timothy Hatcher]

Created attachment 235068 [details]
Patch

[Build Bot]

Comment on attachment 235068 [details]
Patch

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

New failing tests:
inspector-protocol/profiler/console-profileEnd-parameterless.html
inspector-protocol/profiler/console-profile.html

[Build Bot]

Created attachment 235072 [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 235068 [details]
Patch

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

New failing tests:
inspector-protocol/profiler/console-profileEnd-parameterless.html
media/W3C/video/networkState/networkState_during_loadstart.html
media/track/track-long-word-container-sizing.html
inspector-protocol/profiler/console-profile.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 235068 [details]
Patch

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

New failing tests:
inspector-protocol/profiler/console-profileEnd-parameterless.html
inspector-protocol/profiler/console-profile.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 235068 [details]
Patch

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

New failing tests:
inspector-protocol/profiler/console-profileEnd-parameterless.html
media/W3C/video/networkState/networkState_during_loadstart.html

[Build Bot]

Created attachment 235080 [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

[Joseph Pecoraro]

Comment on attachment 235068 [details]
Patch

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

r=me

> Source/WebCore/inspector/InspectorController.cpp:386
>  void InspectorController::setProfilerEnabled(bool enable)

This is now only for layout tests. Maybe we should rename it, or at least include a comment so it is less ambiguous.

Was this called by any other ports, or just mac?

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:161
> +    // the tile of an already recording profile.

Typo: "tile" => "title"

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:165
> +            record.data->getString("title", &recordTitle);

Nit: ASCIILiteral("title")

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:183
> +    for (ptrdiff_t i = m_pendingConsoleProfileRecords.size() - 1; i >= 0; --i) {

Nit: ptrdiff_t? Sounds like you want just "int", or am I missing something?

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:187
> +        record.data->getString("title", &recordTitle);

Nit: ASCIILiteral("title")

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:645
> +    entry.record->setObject("data", entry.data);
> +    entry.record->setArray("children", entry.children);
> +    entry.record->setNumber("endTime", timestamp());

Nit: ASCIILiteral

> Source/WebCore/inspector/InspectorTimelineAgent.h:235
> +        TimelineRecordEntry() { }

Nit: Initialize type to something, so it isn't random.

> Source/WebCore/inspector/InspectorTimelineAgent.h:286
> +    unsigned m_enabledFromConsole;

Nit: Unused? Seems like this can be removed.

[Timothy Hatcher]

Created attachment 235087 [details]
Patch

[Timothy Hatcher]

Created attachment 235089 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 235089 [details]
Patch

Clearing flags on attachment: 235089

Committed r171195: <http://trac.webkit.org/changeset/171195>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

This is causing fast/profiler/profile-with-no-title to crash in Debug builds, the JSC LegacyProfiler is asserting !title.isNull().
<http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r171195%20(6522)/fast/profiler/profile-with-no-title-crash-log.txt>

It looks like the Legacy Profiler allows null titles in stop profiling, and the tests pass in release, so it seems fine to remove the assert. But Tim, you may want to look deeper into this.

[Joseph Pecoraro]

Landed <http://trac.webkit.org/changeset/171204>

[Timothy Hatcher]

Thanks for the fix. It is correct that empty / null should work. In the past ConsoleAgent would substitute empty / null with a generated string. We don't do that now and just handle empty strings in the UI.

[Darin Adler]

Comment on attachment 235089 [details]
Patch

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

> Source/WebCore/bindings/js/ScriptState.cpp:62
> +    ScriptExecutionContext* context = scriptExecutionContextFromExecState(scriptState);
> +    Document* document = context && context->isDocument() ? toDocument(context) : nullptr;
> +    return document ? document->frame() : nullptr;

This includes an extra null check that isn’t helpful. I think it should just be two lines:

    ScriptExecutionContext* context = scriptExecutionContextFromExecState(scriptState);
    return context && context->isDocument() ? toDocument(context)->frame() : nullptr;

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:184
> +    for (ptrdiff_t i = m_pendingConsoleProfileRecords.size() - 1; i >= 0; --i) {
> +        const TimelineRecordEntry& record = m_pendingConsoleProfileRecords[i];

We really need a better idiom for iterating in reverse order. This ptrdiff_t and -1 stuff is super ugly. I want this to be:

    for (auto& record : m_pendingConsoleProfileRecords.reversed())

Or something like that.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:334
> +        const TimelineRecordEntry& entry = m_recordStack.last();
>          entry.data->setNumber("endLine", endLine);

Why do we need a local at all? Putting this into a single line might be cleaner.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:627
> +        const TimelineRecordEntry& parent = m_recordStack.last();
>          parent.children->pushObject(record.release());

Why do we need a local at all? Putting this into a single line might be cleaner.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:653
>      if (!m_recordStack.isEmpty()) {

Should be early return.

> Source/WebCore/inspector/InspectorTimelineAgent.h:145
> +    virtual void start(ErrorString* = nullptr, const int* maxCallStackDepth = nullptr) override;
> +    virtual void stop(ErrorString* = nullptr) override;

Seems strange to needs these default values in an override. Are people actually calling these functions on this class rather than on the base class?

In a related question, can this class be marked final?

> Source/WebCore/inspector/InspectorTimelineAgent.h:236
> +        TimelineRecordEntry()
> +            : type(TimelineRecordType::EventDispatch) { }

I’d put the braces vertically to match the other constructor just below.

If the check-webkit-style script allowed the “all in one line” style, that would be OK with me too.

But to me this looks like a hybrid.

> Source/WebCore/inspector/InspectorTimelineAgent.h:253
> +    TimelineRecordEntry createRecordEntry(PassRefPtr<Inspector::InspectorObject> data, TimelineRecordType, bool captureCallStack, Frame*);

Not sure the argument name “data” here is helpful.

> Source/WebInspectorUI/UserInterface/Images/TimelineRecordConsoleProfile.svg:2
> +<!-- Copyright © 2013 Apple Inc. All rights reserved. -->

Copyright 2013?

> Source/WebKit/mac/WebInspector/WebInspector.mm:113
> +    // No longer supported.
> +    return NO;

Are we going to be able to delete these ever? If so, when?

> Source/WebKit2/WebProcess/WebPage/WebInspector.cpp:280
> +    // No longer supported.

Same question as above.

[Timothy Hatcher]

*** Bug 117571 has been marked as a duplicate of this bug. ***