Bug 26496 - iframe isn't loaded until xmlhttprequest in parent completes
Summary: iframe isn't loaded until xmlhttprequest in parent completes
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-06-17 15:37 PDT by pablo
Modified: 2010-06-15 14:08 PDT (History)
5 users (show)

See Also:


Attachments
php files for repro step (901 bytes, application/octet-stream)
2009-07-02 13:00 PDT, Albert Ma
no flags Details
Patch that fixes XHR's confusing the max-connections-per-host count. (7.71 KB, text/plain)
2009-07-10 14:58 PDT, Brady Eidson
beidson: review-
Details
Updated (removed overzealous decrement in internalAbort()) (8.17 KB, patch)
2009-07-10 15:51 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Now as a patch (8.17 KB, patch)
2009-07-10 15:54 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description pablo 2009-06-17 15:37:04 PDT
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).
Comment 1 Alexey Proskuryakov 2009-06-21 00:07:37 PDT
Would it be possible for you to make a reproducible test case?
Comment 2 pablo 2009-06-27 16:03:58 PDT
(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
Comment 3 Albert Ma 2009-07-02 10:54:14 PDT
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).
> 

Comment 4 Albert Ma 2009-07-02 13:00:57 PDT
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
Comment 5 Albert Ma 2009-07-02 13:05:08 PDT
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
Comment 6 Darin Adler 2009-07-09 17:39:11 PDT
<rdar://problem/7046520>
Comment 7 Brady Eidson 2009-07-10 14:58:56 PDT
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)
Comment 8 Brady Eidson 2009-07-10 15:19:36 PDT
Comment on attachment 32581 [details]
Patch that fixes XHR's confusing the max-connections-per-host count.

Sent out wrong version, fixing soon.
Comment 9 Brady Eidson 2009-07-10 15:51:12 PDT
Created attachment 32585 [details]
Updated (removed overzealous decrement in internalAbort())

Still fixing up a layout test but this is the patch to go with!
Comment 10 Brady Eidson 2009-07-10 15:54:23 PDT
Created attachment 32587 [details]
Now as a patch
Comment 11 Antti Koivisto 2009-07-10 15:59:47 PDT
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
Comment 12 Brady Eidson 2009-07-10 16:02:38 PDT
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%
Comment 13 Brady Eidson 2009-07-10 16:17:58 PDT
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.
Comment 14 Brady Eidson 2009-07-10 16:47:10 PDT
We'll close this - I filed https://bugs.webkit.org/show_bug.cgi?id=27165 as a followup.
Comment 15 Brady Eidson 2009-07-11 12:22:47 PDT
r45755 tweaked this a little to disable the workaround for XHRs on worker threads.
Comment 16 Albert Ma 2009-07-12 18:19:31 PDT
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.
Comment 17 Brady Eidson 2009-07-13 09:19:39 PDT
Please do that as soon as you can!
Comment 18 Vicki Murley 2009-07-13 17:14:10 PDT
Albert, please use WebKit nightly build r45834 or later for any test cases or general testing.
Comment 19 Albert Ma 2009-07-15 09:38:23 PDT
(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.
Comment 20 Albert Ma 2009-07-16 04:54:19 PDT
(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.
Comment 21 Brady Eidson 2009-07-16 09:26:07 PDT
Reopening based on the new test case.

Thanks Albert!  I'll look at this shortly.
Comment 22 Darin Adler 2009-07-16 10:35:05 PDT
The old Radar tracked the old reported problem.

The new one is now covered by <rdar://problem/7065391>.
Comment 23 Brady Eidson 2009-07-16 11:13:05 PDT
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
Comment 24 Brady Eidson 2009-07-16 11:36:18 PDT
Antti reviewed the one-liner on IRC.

Sending        WebCore/ChangeLog
Sending        WebCore/loader/loader.cpp
Transmitting file data ..
Committed revision 45979.
Comment 25 Albert Ma 2009-07-16 18:42:21 PDT
(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!
Comment 26 Brady Eidson 2009-07-17 10:50:14 PDT
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.
Comment 27 Brady Eidson 2009-07-17 10:54:15 PDT
Sending        WebCore/ChangeLog
Sending        WebCore/loader/loader.cpp
Transmitting file data ..
Committed revision 46039.
Comment 28 Albert Ma 2009-07-17 17:27:38 PDT
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.
Comment 29 Albert Ma 2009-07-20 03:45:28 PDT
(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!
Comment 30 Albert Ma 2009-07-23 08:20:24 PDT
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!
Comment 31 Brady Eidson 2009-07-23 11:18:36 PDT
Using the server Albert sent me, I can't reproduce the problem with any recent WebKit nightly or a ToT Debug build.
Comment 32 Brady Eidson 2009-07-23 11:20:33 PDT
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.
Comment 33 Brady Eidson 2009-07-23 11:23:26 PDT
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.
Comment 34 David Levin 2009-07-24 04:03:22 PDT
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).
Comment 35 David Levin 2009-07-24 04:10:59 PDT
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
Comment 36 Brady Eidson 2009-07-24 08:22:36 PDT
Except there were about 5 commit messages in here!  ;)
Comment 37 Brady Eidson 2009-07-24 08:23:26 PDT
(errrrr, commit messages combined with other revision numbers)
Comment 38 David Levin 2009-07-24 08:52:09 PDT
> 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.
Comment 39 Brady Eidson 2009-07-24 08:59:33 PDT
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!
Comment 40 Alexey Proskuryakov 2010-06-15 14:08:16 PDT
Is this still an issue with Safari 5? I'm not sure what exactly this bug is tracking now.