Last Comment Bug 691103 - nsBoxObject::SetPropertyAsSupports cannot recognize nsInterfaceHashtable.Init error
: nsBoxObject::SetPropertyAsSupports cannot recognize nsInterfaceHashtable.Init...
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Makoto Kato [:m_kato]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-01 16:08 PDT by Makoto Kato [:m_kato]
Modified: 2012-02-03 11:51 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
fix (861 bytes, patch)
2012-01-29 19:27 PST, Makoto Kato [:m_kato]
bzbarsky: review+
Details | Diff | Splinter Review

Description Makoto Kato [:m_kato] 2011-10-01 16:08:00 PDT
Although nsInterfaceHashtable.Init returns bool, nsBoxObject::SetPropertyAsSupports uses NS_FAILED.  NS_FAILED(bool) is always true.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-10-01 18:14:46 PDT
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] 2011-10-01 20:27:39 PDT
(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 [:bz] (still a bit busy) 2011-10-01 20:31:47 PDT
> 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 [:bz] (still a bit busy) 2011-10-01 20:33:37 PDT
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] 2012-01-29 19:27:39 PST
Created attachment 592595 [details] [diff] [review]
fix
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-01-31 21:05:03 PST
Comment on attachment 592595 [details] [diff] [review]
fix

r=me
Comment 8 Ed Morley [:emorley] 2012-02-03 11:51:24 PST
https://hg.mozilla.org/mozilla-central/rev/a6361b071a5c

Note You need to log in before you can comment on or make changes to this bug.