Bug 134643 - Web Inspector: Profiler disappeared
Summary: Web Inspector: Profiler disappeared
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
: 117571 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-07-04 23:06 PDT by diebuche
Modified: 2014-07-26 20:53 PDT (History)
8 users (show)

See Also:


Attachments
Patch (48.70 KB, patch)
2014-07-17 08:40 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (509.78 KB, application/zip)
2014-07-17 10:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (539.71 KB, application/zip)
2014-07-17 10:50 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (510.46 KB, application/zip)
2014-07-17 11:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (530.58 KB, application/zip)
2014-07-17 11:56 PDT, Build Bot
no flags Details
Patch (48.76 KB, patch)
2014-07-17 13:29 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (55.72 KB, patch)
2014-07-17 13:38 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description diebuche 2014-07-04 23:06:50 PDT
The profile which used to live in the timeline tab is now gone completely. Starting profiles programmatically is also impossible; console.profile() returns undefined
Comment 1 Radar WebKit Bug Importer 2014-07-04 23:07:01 PDT
<rdar://problem/17566028>
Comment 2 Timothy Hatcher 2014-07-14 10:32:05 PDT
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.
Comment 3 Timothy Hatcher 2014-07-17 08:40:36 PDT
Created attachment 235068 [details]
Patch
Comment 4 Build Bot 2014-07-17 10:16:23 PDT
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
Comment 5 Build Bot 2014-07-17 10:16:26 PDT
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
Comment 6 Build Bot 2014-07-17 10:50:19 PDT
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
Comment 7 Build Bot 2014-07-17 10:50:21 PDT
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
Comment 8 Build Bot 2014-07-17 11:18:38 PDT
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
Comment 9 Build Bot 2014-07-17 11:18:41 PDT
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
Comment 10 Build Bot 2014-07-17 11:56:21 PDT
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
Comment 11 Build Bot 2014-07-17 11:56:24 PDT
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
Comment 12 Joseph Pecoraro 2014-07-17 13:17:20 PDT
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.
Comment 13 Timothy Hatcher 2014-07-17 13:29:00 PDT
Created attachment 235087 [details]
Patch
Comment 14 Timothy Hatcher 2014-07-17 13:38:57 PDT
Created attachment 235089 [details]
Patch
Comment 15 WebKit Commit Bot 2014-07-17 14:17:06 PDT
Comment on attachment 235089 [details]
Patch

Clearing flags on attachment: 235089

Committed r171195: <http://trac.webkit.org/changeset/171195>
Comment 16 WebKit Commit Bot 2014-07-17 14:17:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Joseph Pecoraro 2014-07-17 16:22:51 PDT
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.
Comment 18 Joseph Pecoraro 2014-07-17 16:31:21 PDT
Landed <http://trac.webkit.org/changeset/171204>
Comment 19 Timothy Hatcher 2014-07-17 22:45:56 PDT
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.
Comment 20 Darin Adler 2014-07-20 08:55:34 PDT
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.
Comment 21 Timothy Hatcher 2014-07-26 20:53:15 PDT
*** Bug 117571 has been marked as a duplicate of this bug. ***