561848 - nsNavHistoryContainerResultNode::GetChildIndex is assigning a signed int to an unsigned one

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

*_retval = FindChild(static_cast<nsNavHistoryResultNode*>(aNode));
  if (*_retval == -1)

FindChild returns a signed int that can be -1, but *_retval is unsigned, this could explain some of the random failures i've seen.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 2

[Dietrich Ayala (:dietrich)]

is there a way for us to test a failure scenario here?

Comment 3

[Marco Bonardo [:mak]]

oh, it should be possible to invoke the method, pass it a not existing node and get back an index instead, so yeah i think i can make a test (actually i wanted to do it yesterday but i was surprised by the thunderstorm, and just attached the patch)

Comment 4

[Marco Bonardo [:mak]]

pushed the fix:
http://hg.mozilla.org/mozilla-central/rev/06b78c632960

i'm working on the test, will follow shortly.

Comment 5

[Marco Bonardo [:mak]]

Attachment: functionality test

So, unfortunatly i cannot test the wrong assignment because cpp does not have strict type safety, in this case it is clearly wrong, but on our tier platforms that is probably going to be unnoticed, Windows does not even warn about the signed assign to an unsigned int (Linux GCC does luckily). And actually the condition was still satisfied thanks to an implicit conversion. To test this i should add more than 2^32 nodes, that's not feasible.
Thus, this is just a functionality test for the API, but passes regardless status of this bug.

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 441803 [details] [diff] [review]
functionality test

code coverage ftw, i guess.

Comment 7

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/d36b48ffd03d