Last Comment Bug 589320 - Audit printf()s in dom/ipc
: Audit printf()s in dom/ipc
Status: RESOLVED FIXED
[good first bug]
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla36
Assigned To: Lisa Hewus Fresh [:lfresh]
:
: Bill McCloskey (:billm)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-20 15:29 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2014-10-13 18:55 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Audit of printf() in dom/ipc (372 bytes, patch)
2014-10-10 13:50 PDT, Lisa Hewus Fresh [:lfresh]
bugs: review+
Details | Diff | Splinter Review

Description User image Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-08-20 15:29:25 PDT
We have some hanging around still.  They're quickly becoming not-useful and need to be nuked.
Comment 1 User image Gaurav Saxena 2014-01-23 13:32:43 PST
Can I work on this?
Comment 2 User image Mike Hoye [:mhoye] 2014-04-09 06:56:08 PDT
Gaurav - Are you still interested in working on this bug?
Comment 3 User image Gaurav Saxena 2014-05-06 23:03:15 PDT
(In reply to Mike Hoye [:mhoye] from comment #2)
> Gaurav - Are you still interested in working on this bug?

Yes I am.
Comment 4 User image Mike Hoye [:mhoye] 2014-06-06 12:27:17 PDT
Please go ahead - as soon as you've got a patch ready to go, we can give you ownership of the bug.
Comment 5 User image Lisa Hewus Fresh [:lfresh] 2014-09-28 17:11:39 PDT
I have looked over the code in dom/ipc and do not see an printf()s that seemed inappropriate. In ProcessPriorityManager.cpp it will use printf for logging if ENABLE_LOGGING is defined and that seems as it should be. In ContentProcess.h there is an _MOZ_LOG macro that does not appear to be used anywhere and can probably be removed.

diff -r 6a63bcb6e0d3 dom/ipc/ContentProcess.h
--- a/dom/ipc/ContentProcess.h	Sat Sep 27 03:16:58 2014 -0700
+++ b/dom/ipc/ContentProcess.h	Sun Sep 28 17:09:33 2014 -0700
@@ -11,9 +11,6 @@
 #include "mozilla/ipc/ScopedXREEmbed.h"
 #include "ContentChild.h"

-#undef _MOZ_LOG
-#define _MOZ_LOG(s)  printf("[ContentProcess] %s", s)
-
 namespace mozilla {
 namespace dom {
Comment 6 User image Vinay G Shetty 2014-09-30 09:03:50 PDT
Can I work  on this bug?
Comment 7 User image Mike Hoye [:mhoye] 2014-09-30 09:13:54 PDT
Hi, Vinay - 

It looks like Lisa has already got this one in-hand. Can you take a look at 

http://www.joshmatthews.net/bugsahoy/?

and see if there's one there that's relevant to your interests?

Thanks.
Comment 8 User image Lisa Hewus Fresh [:lfresh] 2014-10-10 11:19:55 PDT
Not sure who, if anyone can review comment 5. I'm happy to submit a patch but it was such a small change so I didn't.
Comment 9 User image Mike Hoye [:mhoye] 2014-10-10 13:17:17 PDT
Ok, then throw it up for review. Every little bit helps. Cjones, what do you think?
Comment 10 User image Andrew McCreight [:mccr8] 2014-10-10 13:38:55 PDT
Olli is probably a better person to ask about dom/ipc stuff at this point.

Lisa, would you mind attaching a regular patch to the bug?  Having a patch around makes the bugzilla process of getting a review etc. easier (so there's something to put an r+ on) etc., even though this patch is very small.
Comment 11 User image Olli Pettay [:smaug] 2014-10-10 13:45:41 PDT
There doesn't seem to be much to do here, as comment 5 hints. There used to be plenty of
printfs in dom/ipc, but apparently they have been removed.
Comment 12 User image Lisa Hewus Fresh [:lfresh] 2014-10-10 13:50:12 PDT
Created attachment 8503410 [details] [diff] [review]
Audit of printf() in dom/ipc

Audit found unused printf
Comment 13 User image Olli Pettay [:smaug] 2014-10-10 14:11:06 PDT
Comment on attachment 8503410 [details] [diff] [review]
Audit of printf() in dom/ipc

Ah yes, that could probably go.
Comment 14 User image Lisa Hewus Fresh [:lfresh] 2014-10-10 14:21:43 PDT
Great! Glad to hear it.
Comment 15 User image Andrew McCreight [:mccr8] 2014-10-10 14:34:41 PDT
Thanks for looking over things and writing the patch, even though it didn't turn up much.

Just for future reference, here's some information about how to make a properly formatted patch:
  https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

(If you use git, let me know and I can find some directions for it.)

For this bug, I'll just fix the patch up, assuming you want the same name and email as you use on bugzilla.  I'll do some simple local testing and land it.
Comment 16 User image Lisa Hewus Fresh [:lfresh] 2014-10-10 14:41:11 PDT
Oh thanks Andrew! I'm reading this over now so I can format them correctly in the future. I would like to use Mercurial for patches on a Mercurial repo but I do use Git as well.

Yes my bugzilla name and email are correct. Thank you.
Comment 17 User image Andrew McCreight [:mccr8] 2014-10-13 11:08:08 PDT
I landed the patch on mozilla-inbound.  It will automatically be merged into the central repository mozilla-central in the next day or so, and then the bug will be set to FIXED.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3cf4699f538
Comment 18 User image Wes Kocher (:KWierso) 2014-10-13 18:55:31 PDT
https://hg.mozilla.org/mozilla-central/rev/b3cf4699f538

Note You need to log in before you can comment on or make changes to this bug.