Bug 41592 – ProgressTracker looks not smart

• Status: UNCONFIRMED
• Component: Page Loading

[Ryuan Choi]

I checked ProgressTracker because I can't get fully loaded contents in several web page.
I realized that those site had bad resource which takes very long times.

but in this case,
ProgressTracker also wasn't updated well.

attachment is my test log using GtkLauncher.
as you can see, series of finishedCallback were called without got-chunk callback sometimes.
so, incrementProgress wasn't called

After simply adding incrementProgress in ResourceLoadNotifier::dispatchDidFinishLoading,
I could get more accurate progress.

but, I don't know whether it's right approach or not.

[Ryuan Choi]

Created attachment 60501 [details]
my test log

[Ryuan Choi]

Created attachment 60705 [details]
Patch

[Adam Barth]

Comment on attachment 60705 [details]
Patch

Thanks for the patch, but a number of things are unclear to me:

WebCore/loader/ProgressTracker.cpp:184
 +      if (frame->loader()->client())
Do we need to null-check the client here?  Is the client disappearing somehow?

WebCore/loader/ProgressTracker.cpp: 
 +      // FIXME: Can this ever happen?
Did we resolve this FIXME?  It might be worth explaining when item might be NULL.

In general, we would like to see a test with each patch.  Is there a way to test this code change?  I'm not 100% sure this is the right direction.  It's difficult to tell from this bug report and from the ChangeLog exactly what problem we're solving here.  It's also unclear to me how this change effects all the different clients of resource load notifier.

[Ryuan Choi]

Thank you for your interesting

(In reply to comment #3)
> (From update of attachment 60705 [details])
> Thanks for the patch, but a number of things are unclear to me:
> 
> WebCore/loader/ProgressTracker.cpp:184
>  +      if (frame->loader()->client())
> Do we need to null-check the client here?  Is the client disappearing somehow?
> 
It's my mistake. I'll remove it.

> WebCore/loader/ProgressTracker.cpp: 
>  +      // FIXME: Can this ever happen?
> Did we resolve this FIXME?  It might be worth explaining when item might be NULL.
> 
this patch make a case which item was null.
so I moved this check logic like below
-    item->bytesReceived += bytesReceived;
-    if (item->bytesReceived > item->estimatedLength) {
-        m_totalPageAndResourceBytesToLoad += ((item->bytesReceived * 2) - item->estimatedLength);
-        item->estimatedLength = item->bytesReceived * 2;
+
+    if (item) {
+        item->bytesReceived += bytesReceived;
+        if (item->bytesReceived > item->estimatedLength) {
+            m_totalPageAndResourceBytesToLoad += ((item->bytesReceived * 2) - item->estimatedLength);
+            item->estimatedLength = item->bytesReceived * 2;
+        }
     }

> In general, we would like to see a test with each patch.  Is there a way to test this code change?  I'm not 100% sure this is the right direction.  It's difficult to tell from this bug report and from the ChangeLog exactly what problem we're solving here.  It's also unclear to me how this change effects all the different clients of resource load notifier.

you are right, but I don't know how to make proper test case.
In almost other network situation, this wasn't happened.
especially I am testing wifi through slow proxy server.

anyway I'll prepare some video file and more log for you.
If I am possible, I can show you this issues using android browser (based on proyo) in my test environment.