691103 - nsBoxObject::SetPropertyAsSupports cannot recognize nsInterfaceHashtable.Init error

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Makoto Kato [:m_kato]]

Although nsInterfaceHashtable.Init returns bool, nsBoxObject::SetPropertyAsSupports uses NS_FAILED.  NS_FAILED(bool) is always true.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So this used to always test false, and now always tests true?

That's really bad...

Michael, could we write some sort of static analysis to catch other cases like this?

Comment 2

[Michael Wu [:mwu]]

(In reply to Boris Zbarsky (:bz) from comment #1)
> So this used to always test false, and now always tests true?
> 
As far as I can tell, this always tested false, and will continue to test false. The underlying implementation (nsTHashtable) returns true for success and false for failure, so the switch to bool cannot change anything as far as I can tell.

> That's really bad...
> 
> Michael, could we write some sort of static analysis to catch other cases
> like this?

MSVC actually tells us if we're trying to test bits in a bool which cannot be anything but a zero.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> As far as I can tell, this always tested false, and will continue to test false

Ok, then there's no real issue.  Comment 0 was implying that NS_FAILED(bool) would be true for some reason, so I assumed there was something about the C++ treatment of bool that I was missing....

Comment 4

[Boris Zbarsky [:bzbarsky]]

I did just verify that bool(true & 0x80000000) is false, and likewise for false.  Sorry for the false alarm.  :(

Comment 5

[Makoto Kato [:m_kato]]

Attachment: fix

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 592595 [details] [diff] [review]
fix

r=me

Comment 7

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6361b071a5c

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a6361b071a5c