1019913 - InitTraceLog() fails to close gBloatLog when creating gBloatView fails

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

If the creation of gBloatView fails, gBloatLog is set to NULL, instead of getting |maybeUnregisterAndCloseFile()| called on it.  Seems minor.

Comment 1

[Andrew McCreight [:mccr8]]

New less alarming summary.

Comment 2

[Shashank VRSN Sabniveesu]

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?

Comment 3

[Andrew McCreight [:mccr8]]

Yes, assuming the maybeUnregisterAndCloseFile() is inside the if (!gBloatView).  I'm not quite sure how to read the diff. :)

Comment 4

[Shashank VRSN Sabniveesu]

Attachment: BUG 1019913 - if gBloatView fails, Call |maybeUnregisterAndCloseFile()| on gBloatLog instead of setting to NULL

Comment 5

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Nathan Froyd [:froydnj]]

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.

Comment 7

[Andrew McCreight [:mccr8]]

I'll do a try push for this eventually, though I think it is unlikely a try run even runs this code. :)

Comment 8

[Andrew McCreight [:mccr8]]

try run: https://tbpl.mozilla.org/?tree=Try&rev=8d9fbc9e7340

Comment 9

[Andrew McCreight [:mccr8]]

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.

Comment 10

[Andrew McCreight [:mccr8]]

Well, I guess I'll just fix it.  If you did build locally, maybe you weren't making a debug build?

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: If gBloatView fails, call |maybeUnregisterAndCloseFile()| on gBloatLog instead of setting it to NULL.

Comment 12

[Andrew McCreight [:mccr8]]

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

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[Carsten Book [:Tomcat]]

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