Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Description • 15 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 1 • 11 years ago

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

	Alessio Placitelli [:Dexter] Assignee
	Comment 2 • 10 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 3 • 10 years ago

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

	Alessio Placitelli [:Dexter] Assignee
	Comment 4 • 10 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 5 • 10 years ago

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.

	Alessio Placitelli [:Dexter] Assignee
	Comment 6 • 10 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 7 • 10 years ago

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.

	Alessio Placitelli [:Dexter] Assignee
	Comment 8 • 10 years ago

Here we go!

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 9 • 10 years ago

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

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 10 • 10 years ago

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

	Alessio Placitelli [:Dexter] Assignee
	Comment 11 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 13 • 10 years ago

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