975944 - Log.jsm's appender objects have wrong ._name

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: Don't overwrite this._name

Every appender in log.jsm sets this._name to something correct, then calls Appender() - which resets this._name to "Appender".  As a result every appender has a name of "Appender".

This patch just changes Appender() to only set this._name if it is not already defined.  Another alternative would be to change every appender to this._name is set only after Appender has been called.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8380454 [details] [diff] [review]
Don't overwrite this._name

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

I'd rather have the other approach of calling the super constructor first (generally more common anyway) - as that also solves it for cases such as BoundedFileAppender. Otherwise you'd need to add that check to every constructor, just to be safe :\

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0008-Bug-975944-fix-log-appender-s-._name-property.-r-unf.patch

(In reply to Blair McBride [:Unfocused] from comment #1)
> I'd rather have the other approach of calling the super constructor first
> (generally more common anyway)

Fairy nuff - how's this?

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8380566 [details] [diff] [review]
0008-Bug-975944-fix-log-appender-s-._name-property.-r-unf.patch

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

r+ with a fixup

::: toolkit/modules/Log.jsm
@@ +588,5 @@
>  
>    // This is a promise exposed for testing/debugging the logger itself.
>    this._lastWritePromise = null;
>    Appender.call(this, formatter);
> +  this._name = "FileAppender";

Should move the Appender.call() line to the top, rather than the this._name line to the bottom. Ditto in BoundedFileAppender.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Thanks!  Pushed with fixup

https://hg.mozilla.org/integration/fx-team/rev/3e84bd445574

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3e84bd445574