Bug 26496 – iframe isn't loaded until xmlhttprequest in parent completes

• Component: Frames
• Status: REOPENED

[pablo]

I have a parent frame that uses long-polling(comet/BOSH) requests and an iframe.
Both the parent frame and the iframe are from the same domain.
The iframe displays webpages and users can navigate in it.

If a user tries to navigate the iframe when a long-polling request is running in the parent, the iframe won't load until the request completes. It happens when using xmlhttprequest or dynamic script tags (JSONP).

[Alexey Proskuryakov]

Would it be possible for you to make a reproducible test case?

[pablo]

(In reply to comment #1)
> Would it be possible for you to make a reproducible test case?
> 

I'm working on a test case.
I think this is related:
http://groups.google.com/group/prototype-scriptaculous/browse_thread/thread/919bf83a1a9a88da/8166655d6643611d?lnk=raot

[Albert Ma]

I also met this bug on Safari 4 (build 530). 
The XHR stream connection not only blocks iframe loading, but also possibly blocks all kinds of resources downloading. 

The cause seems to be:  
While a resource loader/worker is working on XHR stream connection, the other resource might be enqueue into this loader/worker. But because XHR stream connection is  persistent, so the resource request queued won't get downloaded.

The worst cases are that the blocked resource is iframe or Javascript file, when iframe is blocked, the content won't get shown. when Javascript is blocked, no content after the JS will be rendered.

I will provide repro step for it soon.



(In reply to comment #0)
> I have a parent frame that uses long-polling(comet/BOSH) requests and an
> iframe.
> Both the parent frame and the iframe are from the same domain.
> The iframe displays webpages and users can navigate in it.
> 
> If a user tries to navigate the iframe when a long-polling request is running
> in the parent, the iframe won't load until the request completes. It happens
> when using xmlhttprequest or dynamic script tags (JSONP).
> 

[Albert Ma]

Created attachment 32192 [details]
php files for repro step

1 extract the tar ball to a PHP enabled web server
2. visit the index.php
3. click the button to see the result

[Albert Ma]

simple repro steps: 
1. visit http://www.ieasy.org/webkit26496/
2. click the first button to launch XHR stream request (which lasts for 40s)
3. click the second button to load iframe (which contains 200 JS files to load)
4. see the content of the iframe

expected result:
the content of the iframe should be rendered gradually as "0~199"

actual result:
in Safari 4, the rendering stops at "2" or "3", after 40 seconds, it resumes to complete to 199.

Only repro on Safari 4.

Safari 3, IE, Firefox has no problem

[Darin Adler]

<rdar://problem/7046520>

[Brady Eidson]

Created attachment 32581 [details]
Patch that fixes XHR's confusing the max-connections-per-host count.

Patch that fixes XHR's confusing the max-connections-per-host count.

I think we should land this very localized fix while we hash out a reworking of the Loader::Host mechanism that tracks connections-per-host at the ResourceHandle level.

(a test case is coming)

[Brady Eidson]

Comment on attachment 32581 [details]
Patch that fixes XHR's confusing the max-connections-per-host count.

Sent out wrong version, fixing soon.

[Brady Eidson]

Created attachment 32585 [details]
Updated (removed overzealous decrement in internalAbort())

Still fixing up a layout test but this is the patch to go with!

[Brady Eidson]

Created attachment 32587 [details]
Now as a patch

[Antti Koivisto]

Comment on attachment 32587 [details]
Now as a patch

> +    if (m_didTellLoaderAboutRequest) {
> +            cache()->loader()->nonCacheRequestComplete(m_url);
> +            m_didTellLoaderAboutRequest = false;
> +    }

Double indentation here.

r=me

[Brady Eidson]

Sending        WebCore/ChangeLog
Sending        WebCore/loader/loader.cpp
Sending        WebCore/loader/loader.h
Sending        WebCore/xml/XMLHttpRequest.cpp
Sending        WebCore/xml/XMLHttpRequest.h
Transmitting file data .....
Committed revision 45732.

Leaving the bug open until we can rework the Loader::Host and ResourceHandle machinery to work 100%

[Brady Eidson]

Layout test is difficult because reproducing the actual bug is a long running thing.

Adding methods to track and dump the actual number of live resource handles to a host seem to be a requirement to make an efficient and dependable test.

[Brady Eidson]

We'll close this - I filed https://bugs.webkit.org/show_bug.cgi?id=27165 as a followup.

[Brady Eidson]

r45755 tweaked this a little to disable the workaround for XHRs on worker threads.

[Albert Ma]

Just verified on webkit build 45763. 

Issue on the simple repro step for bug 26496 (http://www.ieasy.org/webkit26496/) seems fixed.

But Exchange Outlook Web Access 2010 still has the similar issue. 

It seems the fix(workaround) for webkit bug 26496 is incomplete. 

I will write another test page (repro steps) for webkit.

[Brady Eidson]

Please do that as soon as you can!

[Vicki Murley]

Albert, please use WebKit nightly build r45834 or later for any test cases or general testing.

[Albert Ma]

(In reply to comment #18)
> Albert, please use WebKit nightly build r45834 or later for any test cases or
> general testing.

Sorry, I was on flight whole day of yesterday. 

I have tried r45834, Outlook Web Access 2010 still have problem on it. 
I am working on the simpler repro (that is not specific to OWA), but I haven't got it yet. I will continue working on it. Thanks.

[Albert Ma]

(In reply to comment #17)
> Please do that as soon as you can!

I have found the repro steps for webkit.  The main point is about "Etag" or "Last-Modified" Header, which triggers Safari bug.

Repro steps for webkit:

(I updated php files in www.ieasy.org for this repro steps, js.php adds "Last-Modified:" header:
=== start of js.php ===
<?
header("Content-Type: text/javascript");
header("Last-Modified: Fri, 12 Jun 2009 08:56:45 GMT");
printf("document.write('%s<br>');\n", $_GET['id']);
?>
== end of js.php ==
)

0. Empty Safari Cache
1. visit http://www.ieasy.org/webkit26496/
2. click the first button to launch XHR stream request (which lasts for 40s)
3. click the second button to load iframe (which contains 200 JS files to load)
4. when 0~199 are printed, refresh current page
5. click the first button to launch XHR stream request
6. click the second button to load iframe

expected result:
the content of the iframe should be rendered gradually as "0~199"

actual result:
in Safari 4, the rendering stops at "2" or "3", after 40 seconds, it resumes to
complete to 199.

[Brady Eidson]

Reopening based on the new test case.

Thanks Albert!  I'll look at this shortly.

[Darin Adler]

The old Radar tracked the old reported problem.

The new one is now covered by <rdar://problem/7065391>.

[Brady Eidson]

We have a new fix for this.

The key here is on a largely uncached web page where cache revalidation still gets a free pass around the connection limit, we're still running in to the same issue we had before r45732

[Brady Eidson]

Antti reviewed the one-liner on IRC.

Sending        WebCore/ChangeLog
Sending        WebCore/loader/loader.cpp
Transmitting file data ..
Committed revision 45979.

[Albert Ma]

(In reply to comment #23)
> We have a new fix for this.
> 
> The key here is on a largely uncached web page where cache revalidation still
> gets a free pass around the connection limit, we're still running in to the
> same issue we had before r45732

I just verified on build r45980. The problem with "Etag" and "Last-Modified" headers has been fixed. While, there is other bug in webkit that block Outlook Web Access. 

The changes fixed the problem with Javascript resources, but Image resources still get blocked with the XHR request.

I have updated my repro steps on http://www.ieasy.org/webkit26496/, I create a new page for images.

Repro steps:
1. visit http://www.ieasy.org/webkit26496/
2. click the first button to launch XHR stream request (which lasts for 40s)
3. click the third button to load images  (which contains 8 images to load)
4. see the content of the iframe

expected result:
totally there are 8 images displayed

actual result:
5~6 images are displayed

As I saw the webkit code, the bug  happens when parsedAndStylesheetsKnown==true @ loader.cpp: 293


Thanks!

[Brady Eidson]

As the comment in the code indicates, the original intention of the prioritization was to have as lite a touch as possible and only enforce the connection-per-host limit when prioritizing things was necessary, and otherwise let the networking layer do whatever it likes.

We're not convinced this is necessary, and Darin has reviewed a simple patch that changes this behavior so that WebCore always manages this limit itself.

[Brady Eidson]

Sending        WebCore/ChangeLog
Sending        WebCore/loader/loader.cpp
Transmitting file data ..
Committed revision 46039.

[Albert Ma]

Just did a quick test on Outlook Web Access. The problem seems fixed. Thank you very much for your great effort! 
We will do a complete test of OWA on the new build later, and I will add comment here when the result is ready.

[Albert Ma]

(In reply to comment #28)
> Just did a quick test on Outlook Web Access. The problem seems fixed. Thank you
> very much for your great effort! 
> We will do a complete test of OWA on the new build later, and I will add
> comment here when the result is ready.

OWA looks good on latest nightly build. 
Thanks!

[Albert Ma]

A bad news: from r46126 till now (r46237), Exchange 2010 Outlook Web Access has the problem again. The resource downloading is blocked by XHR stream connection.

This time I cannot find a simple test page to repro the problem of webkit.  I will deploy Exchange 2010 beta server for reproducing problem. The server information and repro steps will be sent to Brady Eidson by a separate email. Hope it will help finding the root cause of the problem. Thanks!

[Brady Eidson]

Using the server Albert sent me, I can't reproduce the problem with any recent WebKit nightly or a ToT Debug build.

[Brady Eidson]

Albert, you explicitly mention that the r46126 nightly and later reproduces this problem.  Can you please confirm that the most recent nightly before that - r46105 - does *not* reproduce the problem?

Between r46105 and r46126 there were no code changes that seem like they could affect this.

[Brady Eidson]

Oh, I was able to reproduce after about 6 tries, with /themes/base/clear1x1.gif

I still haven't been able to reproduce with a script, which would obviously block a frame load, but it's clear there is still a bug now.

[David Levin]

Comment on attachment 32587 [details]
Now as a patch


> Index: xml/XMLHttpRequest.cpp
> +#include "Cache.h"

Rather than flagging each instance below, I just do it once here:
Don't use cache() in XMLHttpRequest because XMLHttpRequest may be used on a thread (in a worker) and cache() isn't threadsafe.

From workers and in Documents, XHR eventually gets to DocumentThreadableLoader (on the main thread), so maybe you can do what you want in there.  (Note that DocumentThreadableLoader may be used by things other than XHR like importScripts in workers).

[David Levin]

Comment on attachment 32587 [details]
Now as a patch

Sorry about that. Clearing my r-. Since I didn't see any commit messages in here, I thought this had not been landed (when I saw it in the commit queue) but it has been.

Committed as http://trac.webkit.org/changeset/45732

Then there was a follow up to address workers here: http://trac.webkit.org/changeset/45755

[Brady Eidson]

Except there were about 5 commit messages in here!  ;)

[Brady Eidson]

(errrrr, commit messages combined with other revision numbers)

[David Levin]

> Except there were about 5 commit messages in here!  ;)
In short: Yeah my bad :( -- sorry.

My only lame defense is that is was in the commit queue when it shouldn't be  -- due to the r+ and being open -- and I was dumb to be working at 4 am.

[Brady Eidson]

Oh interesting.  Yah... an r+, then closing the bug, then reopening...  that shouldn't be in the commit queue and is seemingly a bug in the tool.  Quite understandable!

[Alexey Proskuryakov]

Is this still an issue with Safari 5? I'm not sure what exactly this bug is tracking now.