Bug 140673 – Add a WebKitMessageRecorder DTrace provider, exposing IPC details to DTrace

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Add a WebKitMessageRecorder DTrace provider, exposing IPC details to DTrace

[Tim Horton]

Created attachment 244984 [details]
preliminary

[Tim Horton]

Need to sort out the ProcessType layering violation before this can go up for review.

[Tim Horton]

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.

[Tim Horton]

(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::.

[Tim Horton]

Created attachment 245074 [details]
Patch

[Sam Weinig]

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*.

[Tim Horton]

Created attachment 245474 [details]
derived header

[Tim Horton]

Created attachment 245528 [details]
Patch

[Sam Weinig]

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& ?

[Tim Horton]

Created attachment 245583 [details]
fixing sam's comments

[Tim Horton]

http://trac.webkit.org/changeset/179326

[Darin Adler]

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.

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

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.

[Darin Adler]

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.

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

[Csaba Osztrogonác]

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

[Carlos Alberto Lopez Perez]

(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

[Carlos Garcia Campos]

(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

[Carlos Alberto Lopez Perez]

(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.

[Carlos Garcia Campos]

(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.

[Tim Horton]

(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 :|

[Ting-Wei Lan]

(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 :|