Closed Bug 529877 Opened 11 years ago Closed 7 years ago
Audit stray printfs in e10s code
Bug 525090 points at a more general problem of us [f]printf()'ing willy-nilly. Should be easy enough to fix up with one patch, but this bug can be split into more specific sub-bugs if desireed.
11 years ago
Depends on: 531859
11 years ago
And all these years later, they're still here.
Whiteboard: [good first bug]
(In reply to Chris Jones [:cjones] mostly inactive; ni?/f?/r? if you need me from comment #1) > And all these years later, they're still here. I'm interested in working on this bug, but have a few questions :) - Is there any practical way to find just the e10s code within the Mozilla code base? Or do I need to manually go through the results of http://dxr.mozilla.org/mozilla-central/search?q=regexp%3A(%3Fi)%5Cbs%3Fprintf+ext%3Acpp&case=false - Should this be blocking Bug 879538 as well? - Should the found printfs just be reported or removed/logged somewhere else? Thanks!
(In reply to Alessio Placitelli from comment #2) > (In reply to Chris Jones [:cjones] mostly inactive; ni?/f?/r? if you need me > from comment #1) > > And all these years later, they're still here. > > I'm interested in working on this bug, but have a few questions :) > > - Is there any practical way to find just the e10s code within the Mozilla > code base? Or do I need to manually go through the results of > This bug only covers the stray printf's in dom/ipc, so you can restrict your search to there. However, there's no reason for any gecko code to directly printf(), so if you want to audit more code than just dom/ipc, I'm sure that would be appreciated :). > - Should this be blocking Bug 879538 as well? Sure, that looks like a useful way to track this > - Should the found printfs just be reported or removed/logged somewhere else? I would recommend just removing them for now.
Thank you for your clarifications! Just to be 100% sure, this is one of the stray printfs I need to get rid of: http://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#541 Right? Thanks! :D
Yes, that's one of the ones. They should be replaced with logging macros. Unfortunately, I haven't worked on gecko for a long time, so I'm not sure which macros developers use for logging nowadays.
All in all, I couldn't find many printfs! Does this patch make sense to you? I have some doubts regarding the following: 1) http://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#479 - Couldn't the whole block be replaced by NS_WARNING (which AFAIK should break when debugging)? 2) Should the printfs in ContentParent::JoinAllSubprocesses() be removed (see http://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#546 )? As far as I understand they are not directly related to a fault so there should be no point in replacing them with NS_WARNING (which would break in debugging builds) That was a great opportunity to dive into Mozilla's IPC code :)
Attachment #8401835 - Flags: review?(cjones.bugs)
Comment on attachment 8401835 [details] [diff] [review] bug529877.patch Looks OK. Let's use NS_ERROR() instead of NS_WARNING() for these messages. r=me with that change.
Attachment #8401835 - Flags: review?(cjones.bugs) → review+
Here we go!
7 years ago
Attachment #8405472 - Flags: review?(cjones.bugs) → review+
Whoops, sorry, I was missing an include file. The working patch is attached. That taught me a lesson: always push to try-serv, even for the printfs :D Try-serv: https://tbpl.mozilla.org/?tree=Try&rev=bec7d45b4df7
Attachment #8405472 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.