Audit stray printfs in e10s code

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: cjones, Assigned: Dexter)

Tracking

unspecified
mozilla31
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(e10s+)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

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.
And all these years later, they're still here.
Whiteboard: [good first bug]
Assignee: nobody → alessio.placitelli
(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!
Flags: needinfo?(cjones.bugs)
(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.
Flags: needinfo?(cjones.bugs)
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
Flags: needinfo?(cjones.bugs)
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.
Flags: needinfo?(cjones.bugs)
Posted patch bug529877.patch (obsolete) — Splinter Review
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+
Posted patch bug529877.patch (obsolete) — Splinter Review
Here we go!
Attachment #8401835 - Attachment is obsolete: true
Attachment #8405472 - Flags: review?(cjones.bugs)
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
https://hg.mozilla.org/mozilla-central/rev/4a2232514c15
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.