Bug 15055 - Divide by 0 in ProgressTracker::incrementProgress
Summary: Divide by 0 in ProgressTracker::incrementProgress
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Normal
Assignee: Brett Wilson (Google)
Depends on:
Reported: 2007-08-22 17:08 PDT by Brett Wilson (Google)
Modified: 2007-12-10 15:54 PST (History)
0 users

See Also:

Patch (1.55 KB, patch)
2007-09-05 09:01 PDT, Brett Wilson (Google)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brett Wilson (Google) 2007-08-22 17:08:40 PDT
int numPendingOrLoadingRequests =
estimatedBytesForPendingRequests = progressItemDefaultEstimatedLength *
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.
Comment 1 Brett Wilson (Google) 2007-09-05 09:01:29 PDT
Created attachment 16204 [details]
Comment 2 Oliver Hunt 2007-09-18 13:55:43 PDT
Comment on attachment 16204 [details]

There should be an ASSERTION in the else block to check that remainingBytes is not <0
Comment 3 Brett Wilson (Google) 2007-09-25 10:09:26 PDT
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.
Comment 4 Maciej Stachowiak 2007-09-29 20:22:07 PDT
Comment on attachment 16204 [details]

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.
Comment 5 Brett Wilson (Google) 2007-12-06 14:52:45 PST
Comment on attachment 16204 [details]

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.
Comment 6 Darin Adler 2007-12-06 14:55:45 PST
Comment on attachment 16204 [details]

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:


Is this situation enough different from those that making a test is impractical?
Comment 7 Darin Adler 2007-12-07 01:16:20 PST
Comment on attachment 16204 [details]

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
Comment 8 Andrew Wellington 2007-12-10 15:54:58 PST
Committed in r28593