247715 - ###!!! ASSERTION: unexpected progress values: 'progress <= progressMax', file /home/chb/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp, line 3712

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

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

###!!! ASSERTION: unexpected progress values: 'progress <= progressMax', file
/home/chb/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp, line 3712
1083267968[8093548]: ###!!! Break: at file
/home/chb/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp, line 3712

nsInt64 correctly determines that 284 is > -1 :(

        nsInt64 progressMax(mResponseHead->ContentLength());
        nsInt64 progress = mLogicalOffset + nsInt64(count);
        NS_ASSERTION(progress <= progressMax, "unexpected progress values");

Maybe ContentLength should be inited with LL_MaxInt() not MaxUint... or the
comparison should be unsigned

Comment 1

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

Attachment: patch

uses nsUint64; so requires the patch in the bug on which this one depends on
now...

Comment 2

[R.K.Aa.]

*** Bug 247773 has been marked as a duplicate of this bug. ***

Comment 3

[Darin Fisher]

Comment on attachment 151254 [details] [diff] [review]
patch

Hmm... I was worried about signed/unsigned affects.  Good patch.  I'm glad to
see that we have nsUint64 now.

Comment 4

[tenthumbs]

Seems like overkill just to quash an assertion. At 1GB/sec transferring 2^63 
bytes (the smallest integer that doesn't fit in a int64) will take > 272 
years. That far exceeds the shelf life of mozilla. :-)

You also lose the ability to detect impossible values and you need keep track 
of such things as "not yet started" (i.e., progressMax = -1) separately.

Do you really want to do this?

Comment 5

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

(In reply to comment #4)
> Seems like overkill just to quash an assertion.

huh?

> At 1GB/sec transferring 2^63 
> bytes (the smallest integer that doesn't fit in a int64) will take > 272 
> years. That far exceeds the shelf life of mozilla. :-)

I'll trust you on that.

> You also lose the ability to detect impossible values and you need keep track 
> of such things as "not yet started" (i.e., progressMax = -1) separately.

progressMax == -1 does not mean "not yet started", it means "total size
unknown". as it is the maximum 64 bit number, this is very suitable as an upper
bound when the real size is not known. I'm using nsUint64 here to get an
_unsigned_ comparison, not to double the number space.

Comment 6

[tenthumbs]

> progressMax == -1 does not mean "not yet started", it means "total size
> unknown".

Ok, but the important point is that you're storing both normal data and
status information in the same variable. It's easy to distinguish the two
types of data with signed quantities. Switching to unsigned variables makes
it harder to separate the data. That's why I said you would need to store
the two types of data separately.

> I'm using nsUint64 here to get an _unsigned_ comparison ...

You're really asking two questions: 1) is the data valid and 2) is it within
some limit. Signed quantities make answering both questions simultaneously
relatively easy. Unsigned quantities make it harder. Wouldn't it be better
to rework the tests to decide which question you're asking? Perhaps in 
separate steps. 

Comment 7

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

(In reply to comment #6)
> Ok, but the important point is that you're storing both normal data and
> status information in the same variable. It's easy to distinguish the two
> types of data with signed quantities. Switching to unsigned variables makes
> it harder to separate the data. That's why I said you would need to store
> the two types of data separately.

(you _are_ aware that this was always using unsigned numbers and only recently
changed accidentally to a signed number, right? (bug 243974))

the length of a stream is an unsigned data point. it thus makes sense to store
it in an unsigned variable.

it makes perfect sense to treat "no length known" as the maximum possible
number, so that it is effectively not limited.

> You're really asking two questions: 1) is the data valid and 2) is it within
> some limit.

I am? where?
I only want to know "did the server send more data than it claimed it would send"

> Signed quantities make answering both questions simultaneously
> relatively easy. Unsigned quantities make it harder.

since I don't particularly want to know whether the server did send a length, I
don't need to answer a second question.

Comment 8

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

Attachment: patch v2

after the latest patch in the nsUint64 patch, some more casts are needed

Comment 9

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

Checking in base/src/nsInputStreamPump.cpp;
/cvsroot/mozilla/netwerk/base/src/nsInputStreamPump.cpp,v  <-- 
nsInputStreamPump.cpp
new revision: 1.14; previous revision: 1.13
done
Checking in base/src/nsInputStreamPump.h;
/cvsroot/mozilla/netwerk/base/src/nsInputStreamPump.h,v  <--  nsInputStreamPump.h
new revision: 1.5; previous revision: 1.4
done
Checking in protocol/http/src/nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <-- 
nsHttpChannel.cpp
new revision: 1.209; previous revision: 1.208
done
Checking in protocol/http/src/nsHttpChannel.h;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.h,v  <--  nsHttpChannel.h
new revision: 1.59; previous revision: 1.58
done

Comment 10

[benc]

V/lxr.