Closed Bug 1159473 Opened 9 years ago Closed 9 years ago

Add Mac-specific debug logging code

Categories

(Core :: Widget: Cocoa, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: smichaud, Assigned: smichaud)

References

Details

Attachments

(1 file, 2 obsolete files)

For some time I've had an interpose library (at http://people.mozilla.org/~stmichaud/ReverseEngineering/InterposeLibraryTemplate/) that can do more than the standard things we normally have access to on the Mac.  For example it has a logging method that's better than NSLog(), and it can log stack traces.  Though both methods are Mac-specific, they can be called from anywhere in the tree (they're not limited to Cocoa/Objective-C code).

This patch adds some of those capabilities to the trunk.  There's no need to limit this functionality to interpose libraries.

The code isn't used at all in "standard" builds.  But it can be called to debug specific problems (as the occasion arises), in self builds or even (say) in mozilla-central nightlies.  Since my methods log both to stdout and to the system log, we can ask people (who see a particular bug) to try them for a while, then look in the system log when they see the bug (to find bug-specific diagnostic information that's been written there).

Having this capability may be required to fix bug 1151345, for example.
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Attachment #8598931 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8598931 [details] [diff] [review]
Patch

Review of attachment 8598931 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Just a few nits. Thanks!

::: widget/cocoa/nsCocoaDebugUtils.h
@@ +105,5 @@
> +  // the system log.
> +  static void PrintStackTrace();
> +
> +  // The values returned by GetOwnerName() and GetAddressString() must be
> +  // free()ed by the caller.

nit: I would prefer if we could add this comment to each of the methods below because we could miss it up here. I don't mind the comment for the private methods GetOwnerNameInt() and GetAddressStringInt() further down since they're closer together.

@@ +123,5 @@
> +  // The values returned by GetOwnerNameInt() and GetAddressStringInt() must
> +  // be free()ed by the caller.
> +  static char* GetOwnerNameInt(void* aAddress,
> +                               CSTypeRef aOwner = sInitializer);
> +  static char *GetAddressStringInt(void* aAddress,

nit: s/char *GetAddressStringInt/char* GetAddressStringInt/

::: widget/cocoa/nsCocoaDebugUtils.mm
@@ +17,5 @@
> +
> +static void MaybeGetPathAndID()
> +{
> +  if (!gProcPath[0]) {
> +    proc_pidpath(getpid(), gProcPath, sizeof(gProcPath) - 1);

nit: The '- 1' for the buffersize can be removed.

@@ +21,5 @@
> +    proc_pidpath(getpid(), gProcPath, sizeof(gProcPath) - 1);
> +  }
> +  if (!gBundleID[0]) {
> +    CFStringRef bundleID = CFBundleGetIdentifier(CFBundleGetMainBundle());
> +    CFStringGetCString(bundleID, gBundleID, sizeof(gBundleID) - 1,

nit: The '- 1' for the buffersize can be removed.

@@ +71,5 @@
> +  int msgLength =
> +    CFStringGetMaximumSizeForEncoding(CFStringGetLength(message),
> +                                      kCFStringEncodingUTF8);
> +  char* msgUTF8 = (char*) calloc(msgLength + 1, 1);
> +  CFStringGetCString(message, msgUTF8, msgLength, kCFStringEncodingUTF8);

nit: the proper buffer size to pass here would be |msgLength + 1|

@@ +82,5 @@
> +  ctime_r(&currentTime, timestamp);
> +  timestamp[strlen(timestamp) - 1] = 0;
> +  if (aDecorate) {
> +    char threadName[MAXPATHLEN] = {0};
> +    GetThreadName(threadName, sizeof(threadName) - 1);

nit: -1 can be removed

@@ +115,5 @@
> +
> +void
> +nsCocoaDebugUtils::PrintStackTrace()
> +{
> +  void** addresses = (void **) calloc(STACK_MAX, sizeof(void *));

nit: void** and void*

@@ +139,5 @@
> +void
> +nsCocoaDebugUtils::PrintAddress(void* aAddress)
> +{
> +  char *ownerName = "unknown";
> +  char *addressString = "unknown + 0";

nit: char* on previous two lines

@@ +174,5 @@
> +
> +char*
> +nsCocoaDebugUtils::GetOwnerNameInt(void* aAddress, CSTypeRef aOwner)
> +{
> +  char* retval = (char*) calloc(MAXPATHLEN + 1, 1);

nit: + 1 can be removed here. snprintf() will do the right thing with the size given.

@@ +176,5 @@
> +nsCocoaDebugUtils::GetOwnerNameInt(void* aAddress, CSTypeRef aOwner)
> +{
> +  char* retval = (char*) calloc(MAXPATHLEN + 1, 1);
> +
> +  const char *ownerName = "unknown";

nit: char*

@@ +207,5 @@
> +
> +char*
> +nsCocoaDebugUtils::GetAddressStringInt(void* aAddress, CSTypeRef aOwner)
> +{
> +  char* retval = (char*) calloc(MAXPATHLEN + 1, 1);

nit: + 1 can be removed here. snprintf() will do the right thing with the size given.

@@ +209,5 @@
> +nsCocoaDebugUtils::GetAddressStringInt(void* aAddress, CSTypeRef aOwner)
> +{
> +  char* retval = (char*) calloc(MAXPATHLEN + 1, 1);
> +
> +  const char *addressName = "unknown";

nit: char*
Attachment #8598931 - Flags: review?(spohl.mozilla.bugs) → review+
Attached patch What I will land (obsolete) — Splinter Review
I followed all your suggestions, Stephen.

I don't use Mozilla style in my own code.  I'd already made most of the necessary changes ... but you found a few I'd missed.

I've grown used to explicitly making room for the null that terminates ascii strings ... even though it's (probably) no longer ever necessary.  I also removed all of this.

And finally I added a check for the case when CFBundleGetIdentifier() returns null -- not very likely, but it can't hurt.  Apple's CFLog() also does this.
Attachment #8598931 - Attachment is obsolete: true
Attachment #8599484 - Flags: review+
And ... mozilla-inbound is closed.

I'll land my patch sometime this evening, or maybe tomorrow morning.
Thank you, Steven!

(In reply to Steven Michaud from comment #3)
> I've grown used to explicitly making room for the null that terminates ascii
> strings ... even though it's (probably) no longer ever necessary.  I also
> removed all of this.

Just wanted to clarify that I've verified that all the functions in this patch that use the buffers/buffersizes in question deal with the null terminator correctly, i.e. they always stop short of the end of the buffer when writing to it to leave room for the null terminator. I'm sure there are functions out there that don't do this, but we're not using any here.
Attached patch What I will landSplinter Review
Actually it's just as well that mozilla-inbound was closed.  My CFBundleGetIndentifier() check needed to be more robust.
Attachment #8599484 - Attachment is obsolete: true
Attachment #8599532 - Flags: review+
Comment on attachment 8599532 [details] [diff] [review]
What I will land

I tried to land this patch several times last night and then again this morning.  mozilla-inbound was closed every time :-(

So I've started a tryserver build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8871157c22d0

Once it gets far enough along, I'll add the checkin-needed keyword and hope someone else will be able to get it landed.
See tryserver run from comment #7.
Keywords: checkin-needed
And there was a mad rush to land on mozilla-inbound as soon as it reopened ... but this time I made it! :-)
https://hg.mozilla.org/mozilla-central/rev/76684a166597
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1167834
You need to log in before you can comment on or make changes to this bug.