344641 - In <connection-xpcom.js>, "Warning: function CBSConnection does not always return a value"

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1b1) Gecko/20060713 SeaMonkey/1.1a] (nightly) (W98SE)

When starting ChatZilla for the first time:
[
Warning: function CBSConnection does not always return a value
Source File: chrome://chatzilla/content/lib/js/connection-xpcom.js
Line: 164
Source Code:
}
]

Comment 1

[James Ross]

I'm going to suggest WONTFIX; this is actually deliberate code. The function is only ever called as a constructor, and needs to |return null| in certain cases. Could make it |return this| at the end, but that seems needlessly stupid.

Comment 2

[Serge Gautherie (:sgautherie)]

I'd prefer the warning to be silenced, even if there is no underlying issue:
keep the Error Console clean, don't waste time reporting/searching bugs, ....

Comment 3

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1b1) Gecko/20060801 SeaMonkey/1.1a] (nightly) (W98SE) [CZ-0.9.75]

(Bug still there, of course.)

Comment 4

[James Ross]

Attachment: Return |this| to quiet warning

Comment 5

[[e-mail]]

Comment on attachment 231577 [details] [diff] [review]
Return |this| to quiet warning

The original code is bogus. There should be a strict warning for trying to uselessly return a value inside a constructor, as the value gets ignored.

Comment 6

[[e-mail]]

(In reply to comment #5)
>(From update of attachment 231577 [details] [diff] [review] [edit])
>The original code is bogus. There should be a strict warning for trying to
>uselessly return a value inside a constructor, as the value gets ignored.
Well, "return null;" gets ignored; you have to return an object of some sort.

Comment 7

[James Ross]

Horray for broken specs!

Comment 8

[James Ross]

Also horray for warning about the bit that doesn't matter and not the bit that 'doesn't work'!

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

I would guess the warning gets shown at something like parse time, when it's unknown whether this will be used as a normal function or as a constructor

Comment 10

[James Ross]

Checked in --> FIXED.

Comment 11

[Serge Gautherie (:sgautherie)]

Neil, what kind of fix would you suggest / accept for 1.8.1 branch ?

NB: SM v1.1 will get the current patch, whenever the branch upgrades to CZ v0.9.76 :-|

Comment 12

[[e-mail]]

(In reply to comment #11)
>Neil, what kind of fix would you suggest / accept for 1.8.1 branch ?
I do not intend to learn the entire ChatZilla codebase to give an accurate answer. However, based on a cursory inspection of that code fragment, I would suggest that it needs to throw an exception. Failing that, the returns should be removed, leaving the assert as the last statement in the constuctor.

>NB: SM v1.1 will get the current patch, whenever the branch upgrades to CZ
>v0.9.76 :-|
Fair enough.

Comment 13

[[e-mail]]

Given the latest patch for bug 329440 we should back this out again.

Comment 14

[James Ross]

Reverse fixed.