Bug 140673 - Add a WebKitMessageRecorder DTrace provider, exposing IPC details to DTrace
Summary: Add a WebKitMessageRecorder DTrace provider, exposing IPC details to DTrace
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on: 141027
Blocks:
  Show dependency treegraph
 
Reported: 2015-01-20 04:57 PST by Tim Horton
Modified: 2015-02-02 22:12 PST (History)
10 users (show)

See Also:


Attachments
preliminary (53.30 KB, patch)
2015-01-20 04:58 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (60.62 KB, patch)
2015-01-21 11:55 PST, Tim Horton
no flags Details | Formatted Diff | Diff
derived header (2.27 KB, application/octet-stream)
2015-01-27 14:49 PST, Tim Horton
no flags Details
Patch (60.27 KB, patch)
2015-01-28 03:04 PST, Tim Horton
no flags Details | Formatted Diff | Diff
fixing sam's comments (58.11 KB, patch)
2015-01-28 16:50 PST, Tim Horton
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2015-01-20 04:57:24 PST
Add a WebKitMessageRecorder DTrace provider, exposing IPC details to DTrace
Comment 1 Tim Horton 2015-01-20 04:58:58 PST
Created attachment 244984 [details]
preliminary
Comment 2 Tim Horton 2015-01-20 04:59:24 PST
Need to sort out the ProcessType layering violation before this can go up for review.
Comment 3 Tim Horton 2015-01-20 14:25:09 PST
Sam thinks that we should use bundleID instead of process type enums, and keep around the remote bundleID on the Connection when it's launched, and have a mapping in the script that processes the output.

Dan thinks that the DTrace script that traces the messages should be included in this patch.
Comment 4 Tim Horton 2015-01-20 18:29:51 PST
(In reply to comment #3)
> Sam thinks that we should use bundleID instead of process type enums, and
> keep around the remote bundleID on the Connection when it's launched, and
> have a mapping in the script that processes the output.

We're not going to do this, just move the enum to IPC::.
Comment 5 Tim Horton 2015-01-21 11:55:34 PST
Created attachment 245074 [details]
Patch
Comment 6 Sam Weinig 2015-01-23 11:41:56 PST
Comment on attachment 245074 [details]
Patch

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

I would like to see the generated output as well before finishing the review.

> Source/WebKit2/Platform/IPC/MessageRecorder.cpp:35
> +using namespace IPC;

You should instead use the normal namespace IPC { }.

> Source/WebKit2/Platform/IPC/MessageRecorder.cpp:49
> +    record.sourceProcessType = (uint64_t)connection->client()->localProcessType();
> +    record.destinationProcessType = (uint64_t)connection->client()->remoteProcessType();

static_cast.

> Source/WebKit2/Platform/IPC/MessageRecorder.cpp:77
> +    record.sourceProcessType = (uint64_t)connection->client()->remoteProcessType();
> +    record.destinationProcessType = (uint64_t)connection->client()->localProcessType();

static_cast.

> Source/WebKit2/Platform/IPC/MessageRecorder.h:45
> +    MallocPtr<char> messageReceiverName;
> +    MallocPtr<char> messageName;

Maybe this should use std::unique_ptr<char[]>, though I don't know if either have guarantees that that they have the same memory layout as a char*.
Comment 7 Tim Horton 2015-01-27 14:49:38 PST
Created attachment 245474 [details]
derived header
Comment 8 Tim Horton 2015-01-28 03:04:09 PST
Created attachment 245528 [details]
Patch
Comment 9 Sam Weinig 2015-01-28 11:51:47 PST
Comment on attachment 245528 [details]
Patch

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

> Source/WebKit2/Platform/IPC/MessageRecorder.cpp:42
> +std::unique_ptr<MessageRecorder::MessageProcessingToken> MessageRecorder::recordOutgoingMessage(Connection* connection, MessageEncoder* encoder)

Can this take a Connection& and a MessageDecoder& ?

> Source/WebKit2/Platform/IPC/MessageRecorder.cpp:70
> +void MessageRecorder::recordIncomingMessage(Connection* connection, MessageDecoder* decoder)

Can this take a Connection& and a MessageDecoder& ?
Comment 10 Tim Horton 2015-01-28 16:50:34 PST
Created attachment 245583 [details]
fixing sam's comments
Comment 11 Tim Horton 2015-01-28 18:32:50 PST
http://trac.webkit.org/changeset/179326
Comment 12 Darin Adler 2015-01-28 23:56:19 PST
Comment on attachment 245583 [details]
fixing sam's comments

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

> Source/WebKit2/Platform/IPC/ArgumentCoders.h:32
> +#include <uuid/uuid.h>

This header doesn’t seem to be present on EFL or GTL, so the code needs to be conditional to allow them to continue to build.
Comment 13 Csaba Osztrogonác 2015-01-29 00:00:04 PST
(In reply to comment #12)
> Comment on attachment 245583 [details]
> fixing sam's comments
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245583&action=review
> 
> > Source/WebKit2/Platform/IPC/ArgumentCoders.h:32
> > +#include <uuid/uuid.h>
> 
> This header doesn’t seem to be present on EFL or GTL, so the code needs to
> be conditional to allow them to continue to build.

Added uuid-dev as package dependency list:
http://trac.webkit.org/changeset/179337
Comment 14 Csaba Osztrogonác 2015-01-29 00:06:50 PST
Comment on attachment 245583 [details]
fixing sam's comments

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

> Source/WebKit2/Platform/IPC/MessageRecorder.cpp:32
> +#include "MessageRecorderProbes.h"

Where is it come from? I can't see this file in the tree checked 
in and can't find any generator responsible for generating it.
Comment 15 Darin Adler 2015-01-29 00:12:00 PST
Comment on attachment 245583 [details]
fixing sam's comments

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

>> Source/WebKit2/Platform/IPC/MessageRecorder.cpp:32
>> +#include "MessageRecorderProbes.h"
> 
> Where is it come from? I can't see this file in the tree checked 
> in and can't find any generator responsible for generating it.

I believe it is generated from the "MessageRecorder.d" file by Xcode.
Comment 16 mitz 2015-01-29 00:14:22 PST
You can see the dtrace invocation in the build log:
    /usr/sbin/dtrace -h -s Platform/IPC/MessageRecorderProbes.d -o ${DERIVED_SOURCES_DIR}/MessageRecorderProbes.h
Comment 17 Csaba Osztrogonác 2015-01-29 00:19:44 PST
It seems there is no dtrace on Linux, I'll try add ifdef guards to disable
this new feature on non Cocoa platforms, I filed a new bug report: bug141027
Comment 18 Carlos Alberto Lopez Perez 2015-01-29 02:58:37 PST
(In reply to comment #17)
> It seems there is no dtrace on Linux, I'll try add ifdef guards to disable
> this new feature on non Cocoa platforms, I filed a new bug report: bug141027

For the record:

To my knowledge, there are at least two or three ports of DTrace for Linux. One of them done by Oracle itself (the owners of the copyright for DTrace)

Is not shipped within the Linux kernel (and never will be) because of incompatibility between the GPL (Linux) and the CDDL (DTrace) Licenses.

However, is possible to ship it as a DKMS kernel module that is built on the user machine at install time. That avoids the legal issues.

Also it seems that Oracle ships it on their distribution.

Some links:

https://events.linuxfoundation.org/images/stories/pdf/lfcs2012_zannoni_hees.pdf
https://blogs.oracle.com/linux/entry/announcement_dtrace_for_oracle_linux
https://github.com/dtrace4linux/linux
http://www.crisp.demon.co.uk/tools.html
Comment 19 Carlos Garcia Campos 2015-01-29 03:14:03 PST
(In reply to comment #17)
> It seems there is no dtrace on Linux, I'll try add ifdef guards to disable
> this new feature on non Cocoa platforms, I filed a new bug report: bug141027

I would use ENABLE(DTRACE) or something like that, instead of PLATFORM(COCOA), since we might support DTrace eventually. In any case, I think we want to support it optionally, so we need the #ifdefs guards even if we support DTrace
Comment 20 Carlos Alberto Lopez Perez 2015-01-29 04:02:29 PST
(In reply to comment #19)
> (In reply to comment #17)
> > It seems there is no dtrace on Linux, I'll try add ifdef guards to disable
> > this new feature on non Cocoa platforms, I filed a new bug report: bug141027
> 
> I would use ENABLE(DTRACE) or something like that, instead of
> PLATFORM(COCOA), since we might support DTrace eventually. In any case, I
> think we want to support it optionally, so we need the #ifdefs guards even
> if we support DTrace

Also, other platforms where WebKitGTK+ runs, like FreeBSD, include support for DTrace out of the box... so I think that ENABLE(DTRACE) is ok.

And maybe default ENABLE(DTRACE) to true on the platforms that this support comes out of the box: To my knowledge -> MacOS, FreeBSD, NetBSD and Solaris.
Comment 21 Carlos Garcia Campos 2015-01-29 04:06:49 PST
(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #17)
> > > It seems there is no dtrace on Linux, I'll try add ifdef guards to disable
> > > this new feature on non Cocoa platforms, I filed a new bug report: bug141027
> > 
> > I would use ENABLE(DTRACE) or something like that, instead of
> > PLATFORM(COCOA), since we might support DTrace eventually. In any case, I
> > think we want to support it optionally, so we need the #ifdefs guards even
> > if we support DTrace
> 
> Also, other platforms where WebKitGTK+ runs, like FreeBSD, include support
> for DTrace out of the box... so I think that ENABLE(DTRACE) is ok.
> 
> And maybe default ENABLE(DTRACE) to true on the platforms that this support
> comes out of the box: To my knowledge -> MacOS, FreeBSD, NetBSD and Solaris.

I wonder if it would be desirable to disable the message recorder in production builds even for platforms supporting DTrace. ENABLE(DTRACE) would work for that case as well.
Comment 22 Tim Horton 2015-01-29 04:33:14 PST
(In reply to comment #21)
> (In reply to comment #20)
> 
> I wonder if it would be desirable to disable the message recorder in
> production builds even for platforms supporting DTrace. ENABLE(DTRACE) would
> work for that case as well.

The plan is that it should be sufficiently low overhead that you don't have to do this, so you can trace production builds.

But, sure, I think ENABLE(DTRACE) is probably a good plan all around (for https://bugs.webkit.org/show_bug.cgi?id=141027, not here).

> This header doesn’t seem to be present on EFL or GTL, so the code needs to be conditional to allow them to continue to build.

I checked to make sure that one was available for the other ports (though didn't know how to fix the build), but I totally forgot about DTrace :|
Comment 23 Ting-Wei Lan 2015-02-02 22:12:09 PST
(In reply to comment #22)
> (In reply to comment #21)
> The plan is that it should be sufficiently low overhead that you don't have
> to do this, so you can trace production builds.
> 
> But, sure, I think ENABLE(DTRACE) is probably a good plan all around (for
> https://bugs.webkit.org/show_bug.cgi?id=141027, not here).

I think ENABLE(DTRACE) is needed on FreeBSD because dtrace -h needs dtraceall.ko to be loaded to work. Users may want to disable dtrace when they don't have root privileges to load kernel modules.

> 
> > This header doesn’t seem to be present on EFL or GTL, so the code needs to be conditional to allow them to continue to build.
> 
> I checked to make sure that one was available for the other ports (though
> didn't know how to fix the build), but I totally forgot about DTrace :|