Divide by 0 in ProgressTracker::incrementProgress

• Component: Page Loading
• Status: RESOLVED FIXED

[Brett Wilson (Google)]

int numPendingOrLoadingRequests =
    m_originatingProgressFrame->loader()->numPendingOrLoadingRequests(true);
estimatedBytesForPendingRequests = progressItemDefaultEstimatedLength *
    numPendingOrLoadingRequests;
remainingBytes = ((m_totalPageAndResourceBytesToLoad +
    estimatedBytesForPendingRequests) - m_totalBytesReceived);

percentOfRemainingBytes = (double)bytesReceived / (double)remainingBytes;

The problem here is that remainingBytes can be 0 depending on the (not very reliable) estimatedBytesForPendingRequests. This should be an easy fix.

[Brett Wilson (Google)]

Created attachment 16204 [details]
Patch

[Oliver Hunt]

Comment on attachment 16204 [details]
Patch

There should be an ASSERTION in the else block to check that remainingBytes is not <0

[Brett Wilson (Google)]

Why should there be an assertion? Won't that just look like a crash in release mode as well?

From what I understand, the whole point is that the "remaining bytes" is an estimate and not necessarily reality. As far as the math here goes, you might as well be using rand(). It doesn't make sense to assert that rand() is any particular value. Unless you are going to assert that the remaining bytes is guaranteed accurate, it doesn't make sense to add an assertion here.

[Maciej Stachowiak]

Comment on attachment 16204 [details]
Patch

The fix looks good to me (I can understand why the remainingBytes estimate could be wrong, since resources might not specify their length). But do you have a test case that can trigger this? I believe it could be done as an http test. r- to consider providing a test case, but if that is not practical just reflag for review and I'll approve it.

[Brett Wilson (Google)]

Comment on attachment 16204 [details]
Patch

I don't think it's possible to write a test very easily. I think this would depend on the reported length from the server being wrong (maybe possible to test) and also that the packets arrive and WebKit does processing such that the size of the amount received is exactly the same as the predicted amount, without being complete.

Feel free to suggest a way if you know. I don't know any PHP and I've never written an HTTP test, so there could be something I don't know.

[Darin Adler]

Comment on attachment 16204 [details]
Patch

We already have regression tests for things like this: The regression test engine runs a local copy of Apache, and we write server-side scripts to deliver data and delay as appropriate.

For example:

    LayoutTests/http/tests/incremental/slow-utf8-html.pl
    LayoutTests/http/tests/incremental/split-hex-entities.pl

Is this situation enough different from those that making a test is impractical?

[Darin Adler]

Comment on attachment 16204 [details]
Patch

OK, you wore me down. Usually we do require tests, but since this is obviously correct and you're not inspired to make a test, r=me

[Andrew Wellington]

Committed in r28593