529877 - Audit stray printfs in e10s code

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

And all these years later, they're still here.

Comment 2

[Alessio Placitelli [:Dexter]]

(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!

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 4

[Alessio Placitelli [:Dexter]]

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

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: bug529877.patch

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 :)

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: bug529877.patch

Here we go!

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec23d10b47eb

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/936a3cbca03e

for failures like:

https://tbpl.mozilla.org/php/getParsedLog.php?id=37684356&tree=Mozilla-Inbound

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: bug529877.patch

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a2232514c15

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a2232514c15