If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Audit stray printfs in e10s code

RESOLVED FIXED in mozilla31

Status

()

Core
IPC
RESOLVED FIXED
8 years ago
4 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.
Depends on: 531859
Blocks: 516518
And all these years later, they're still here.
Whiteboard: [good first bug]
(Assignee)

Updated

4 years ago
Assignee: nobody → alessio.placitelli
(Assignee)

Comment 2

4 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!
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)
(Assignee)

Comment 4

4 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
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)
(Assignee)

Comment 6

4 years ago
Created attachment 8401835 [details] [diff] [review]
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 :)
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+
(Assignee)

Comment 8

4 years ago
Created attachment 8405472 [details] [diff] [review]
bug529877.patch

Here we go!
Attachment #8401835 - Attachment is obsolete: true
Attachment #8405472 - Flags: review?(cjones.bugs)
tracking-e10s: --- → +
Attachment #8405472 - Flags: review?(cjones.bugs) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec23d10b47eb
Flags: in-testsuite-
Keywords: checkin-needed
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
(Assignee)

Comment 11

4 years ago
Created attachment 8405844 [details] [diff] [review]
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
Attachment #8405472 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a2232514c15
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4a2232514c15
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.