Closed
Bug 1159473
Opened 9 years ago
Closed 9 years ago
Add Mac-specific debug logging code
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: smichaud, Assigned: smichaud)
References
Details
Attachments
(1 file, 2 obsolete files)
14.09 KB,
patch
|
smichaud
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Attachment #8598931 -
Flags: review?(spohl.mozilla.bugs)
Comment 2•9 years ago
|
||
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(¤tTime, 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+
Assignee | ||
Comment 3•9 years ago
|
||
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+
Assignee | ||
Comment 4•9 years ago
|
||
And ... mozilla-inbound is closed. I'll land my patch sometime this evening, or maybe tomorrow morning.
Comment 5•9 years ago
|
||
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.
Assignee | ||
Comment 6•9 years ago
|
||
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+
Assignee | ||
Comment 7•9 years ago
|
||
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.
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8599532 [details] [diff] [review] What I will land Landed on mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/76684a166597
Assignee | ||
Comment 11•9 years ago
|
||
And there was a mad rush to land on mozilla-inbound as soon as it reopened ... but this time I made it! :-)
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/76684a166597
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•