Closed Bug 1019913 Opened 6 years ago Closed 6 years ago

InitTraceLog() fails to close gBloatLog when creating gBloatView fails

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: mccr8, Assigned: shashank, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [good first bug])

Attachments

(1 file, 1 obsolete file)

If the creation of gBloatView fails, gBloatLog is set to NULL, instead of getting |maybeUnregisterAndCloseFile()| called on it.  Seems minor.
New less alarming summary.
Summary: InitTraceLog() fails to close gBloatLog → InitTraceLog() fails to close gBloatLog when creating gBloatView fails
Whiteboard: [mentor=mccr8] [good first bug]
Mentor: continuation
Whiteboard: [mentor=mccr8] [good first bug] → [good first bug]
717 static void
718 InitTraceLog()
719 {
...
731     if (!gBloatView) {
732       NS_WARNING("out of memory");
- 733       gBloatLog = nullptr;
734       gLogLeaksOnly = false;
735     }
+ 735       maybeUnregisterAndCloseFile(gBloatLog);
736   }

Is this the intended change?
Yes, assuming the maybeUnregisterAndCloseFile() is inside the if (!gBloatView).  I'm not quite sure how to read the diff. :)
Comment on attachment 8450650 [details] [diff] [review]
BUG 1019913 - if gBloatView fails, Call |maybeUnregisterAndCloseFile()| on gBloatLog instead of setting to NULL

Review of attachment 8450650 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me.  I'll get Nathan to review it.
Attachment #8450650 - Flags: review?(nfroyd)
Attachment #8450650 - Flags: review?(continuation)
Attachment #8450650 - Flags: feedback+
Comment on attachment 8450650 [details] [diff] [review]
BUG 1019913 - if gBloatView fails, Call |maybeUnregisterAndCloseFile()| on gBloatLog instead of setting to NULL

Review of attachment 8450650 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Apologies for the delay, I was out on Friday.
Attachment #8450650 - Flags: review?(nfroyd) → review+
I'll do a try push for this eventually, though I think it is unlikely a try run even runs this code. :)
Hi, it looks like this doesn't build.  The new use of maybeUnregisterAndCloseFile happens before it is defined.  Please upload a new version and make sure it builds.  Let us know if you need help with the build process.
Flags: needinfo?(shashank)
Well, I guess I'll just fix it.  If you did build locally, maybe you weren't making a debug build?
Flags: needinfo?(shashank)
Comment on attachment 8452004 [details] [diff] [review]
If gBloatView fails, call |maybeUnregisterAndCloseFile()| on gBloatLog instead of setting it to NULL.

I just moved the definition up.  Feel free to not actually re-review it, Nathan.

new try run: https://tbpl.mozilla.org/?tree=Try&rev=a62593216ea0
Attachment #8452004 - Flags: review?(nfroyd)
Attachment #8452004 - Flags: review?(nfroyd) → review+
Assignee: nobody → shashank
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4da1c8306287
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.