Bug 48002 – network-simulator.php makes for very slow layout tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Michael Nordman]

The script simulates network errors with by forever redirecting back to requested url. Some layout tests make repeated use of this script in a single test run. On an unloaded system, each invocation can take many round trips and multiple seconds to trigger the desired error (depending on the underlying http stack). On a system trying to run tests in parallel, this slowness is more pronounced and timeouts result.

A few appcache layout tests use this script and at least one xhr test. In many (maybe all?) of the layout tests, an HTTP error would work as well as a network error for testing the logic, and would run a lot faster.

[Alexey Proskuryakov]

This script simulates a disconnected network, as well as we can. Sending an error response would test something different.

[Michael Nordman]

(In reply to comment #1)
> This script simulates a disconnected network, as well as we can. Sending an error response would test something different.

Yes, I see what it's doing and why. The fact remains that we have tests that timeout due to this.

I question whether we need to simulate a network error in all the places where this script is used. It looks like a http error would effectively test the logic that wants to be tested. I'm looking thru the appcache tests and noticing that simulated network errors are used thruout, but haven't really noticed (m)any tests that simulate a server error.

Also, I question whether this is as good of a simulation of a network error as can be constructed. I wonder if we can close the underlying connection from the server-side?

[Alexey Proskuryakov]

> Yes, I see what it's doing and why. The fact remains that we have tests that timeout due to this.

Can that be fixed in chromium networking code? I don't think CFNetwork has any performance issues with redirects.

> I'm looking thru the appcache tests and noticing that simulated network errors are used thruout, but haven't really noticed (m)any tests that simulate a server error.

Yeah, we need more of the latter kind.

> Also, I question whether this is as good of a simulation of a network error as can be constructed. I wonder if we can close the underlying connection from the server-side?

I couldn't find a better way when making those tests. If there is a way to force Apache to rudely drop a connection, that would be a much better approximation indeed.

[Michael Nordman]

(In reply to comment #3)
> > Yes, I see what it's doing and why. The fact remains that we have tests that timeout due to this.
> 
> Can that be fixed in chromium networking code? I don't think CFNetwork has any performance issues with redirects.

It functions fine, its just slow on this non-sensical content. I really don't want to modify the networking code for this.

> 
> > I'm looking thru the appcache tests and noticing that simulated network errors are used thruout, but haven't really noticed (m)any tests that simulate a server error.
> 
> Yeah, we need more of the latter kind.

I'd like to switch some of the former to be some of the latter, and retain some 'network error' cases in there too, but lean on them less.

> > Also, I question whether this is as good of a simulation of a network error as can be constructed. I wonder if we can close the underlying connection from the server-side?
> 
> I couldn't find a better way when making those tests. If there is a way to force Apache to rudely drop a connection, that would be a much better approximation indeed.

I don't know php very well, I didn't see an obvious way to close the connection? If I could get a reference to the underlying 'socket' there is a call to close the socket. 

Another approach could be to simulate a 'network error' could be with a malformed http message, maybe an excessively large header? I'll see if php lets me form a malformed header? It'd be nice if php let me produce the output w/o doing anything 'for me' in this case.

[Michael Nordman]

> Another approach could be to simulate a 'network error' could be with a malformed http message, maybe an excessively large header? I'll see if php lets me form a malformed header? It'd be nice if php let me produce the output w/o doing anything 'for me' in this case.

Ah ha... at least the instance of php i'm running here let's me produce arbitrarily large headers!

        # Simulate a network error with an excessively large response header.
        $largeString = "";
        for ($x = 0; $x < 10000; $x++)
            $largeString .= "abcdefghijklmonpqrstuvwxyz0123456789";
        header('HTTP/1.1 200 OK');
        header('X-WayTooBig: ' . $largeString);

[Michael Nordman]

Created attachment 71353 [details]
faster network simulator

as mentioned in the previous comments

[Alexey Proskuryakov]

Which network back-ends did you test with? What happened with performance on non-chromium ones?

[Alexey Proskuryakov]

Also, why do you think that changing an existing test in this way is a good idea? You can add a new one if you want to test this code path.

[Michael Nordman]

(In reply to comment #7)
> Which network back-ends did you test with? What happened with performance on non-chromium ones?

I haven't gotten that far yet, only run with chrome's test_shell thusfar (that's why no cq setting). I'm working on patching this in on my mac and am watching what the try bots are doing with the uploaded patch.

(In reply to comment #8)
> Also, why do you think that changing an existing test in this way is a good idea? You can add a new one if you want to test this code path.

As mentioned in the previous comments, because there's little (or no) coverage for http errors triggering fallbacks in the tests. I believe there still are other tests where 'network errors' trigger fallbacks.

[Michael Nordman]

Bah! Excessively large headers don't induce an error in safari(win), looks like the value is truncated (at least as displayed by the inspector).

Curious tidbits...

* chrome and firefox will follow 21 self-redirects before giving up
* safari(win) follows 17
* ie8, followed 800+ redirects before i gave up on it

... so i definitely can't change this self-redirect following behavior in the network stack

[Michael Nordman]

A related chrome issue with some relevant history.
http://code.google.com/p/chromium/issues/detail?id=54717

[Michael Nordman]

Created attachment 72099 [details]
faster network simulator

A smaller change to the script that does the trick.

        # Simulate a network error by replying with a non-sense response.
        header('HTTP/1.1 307 Temporary Redirect');
        header('Location: ' . $_SERVER['REQUEST_URI']); # redirect to self
        header('Content-Length: 1');
        header('Content-Length: 5', false); # multiple content-length headers
        echo "Intentionally incorrect response.";

[Alexey Proskuryakov]

Comment on attachment 72099 [details]
faster network simulator

View in context: https://bugs.webkit.org/attachment.cgi?id=72099&action=review

Nice, but please don't change existing tests.

> LayoutTests/http/tests/resources/network-simulator.php:5
> +// offline, it simulates a network error with a non-sense response.

What's the difference between non-sense and nonsense? Most dictionaries don't seem to include this spelling.

> LayoutTests/http/tests/resources/network-simulator.php:66
> +        header('Content-Length: 1');
> +        header('Content-Length: 5', false); # multiple content-length headers

A good comment would explain why we have these lines (like, it helps some network back-ends detect the error faster).

[Michael Nordman]

(In reply to comment #13)
> (From update of attachment 72099 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72099&action=review
> 
> Nice, but please don't change existing tests.

Do you mean in this patch or at all?

[Alexey Proskuryakov]

Well, I certainly agree it's a good idea to land a test using the new capability in the same patch.

It seems best and safest to add a new one.

[Michael Nordman]

(In reply to comment #15)
> Well, I certainly agree it's a good idea to land a test using the new capability in the same patch.

There is no new capability?

Some details of how the error is simulated is changed is all. The layout test changes in this patch are strictly minor improvements (imho). In fallback.html, don't send two requests when one will do. In offline-access.htlm, delete some stale code.

[Alexey Proskuryakov]

I was distracted, and somehow thought of your earlier patch, sorry.

[Alexey Proskuryakov]

There was some reason why I made a separate canLoad() check, I'll try to remember it. Maybe it wasn't a good one.

[Michael Nordman]

Created attachment 72250 [details]
faster network simulator

spelling and wordsmithing

[Michael Nordman]

ping

[Michael Nordman]

ping

[Alexey Proskuryakov]

I asked, and you said there was no urgency. Why do you ping each working day? Is this super urgent now?

[Michael Nordman]

(In reply to comment #22)
> I asked, and you said there was no urgency. Why do you ping each working day? Is this super urgent now?

It isn't super urgent, but the timeout problem does obscure actual test results, so I'd like to get it in so I can start seeing the actual results more clearly. It's a small change so I thought it would be easy to review.

Maybe I should have somebody else take a look?

[Alexey Proskuryakov]

Comment on attachment 72250 [details]
faster network simulator

View in context: https://bugs.webkit.org/attachment.cgi?id=72250&action=review

OK. Please address the comments, and verify how much of a performance win the test change is.

> LayoutTests/ChangeLog:9
> +        multiple content-length headers. Chrome's network stacks consider that an

Typo: considers.

> LayoutTests/ChangeLog:11
> +        This avoids time outs due to using PHP via slow CGI on windows.

Typo: timeouts.

> LayoutTests/ChangeLog:21
> +        * http/tests/resources/offline-access.html:

This change is in http/tests/appcache/resources/offline-access-frame.html, not offline-access.html.

> LayoutTests/http/tests/appcache/fallback.html:57
> +        log("FAIL:Cannot load a URL from fallback namespace when network is enabled, ex = " + ex);

Missing space.

> LayoutTests/http/tests/appcache/fallback.html:59
>          hadError = true;
>      }

I remembered what the benefit from canLoad() was - it helped write more concise and readable code. It's much harder to read the new version of the test.

If you get measurable speedup from these changes, let's land them - but if it can't be measured, let's keep the test as is.

> LayoutTests/http/tests/appcache/fallback.html:149
> -    if (!canLoad(testURL)) {
> -        log("FAIL: Cannot load an URL from fallback namespace when network is disabled");
> -        hadError = true;
> -    } else if (load(testURL) != load("resources/simple.txt")) {
> -        log("FAIL: Loading an URL from fallback namespace when network is disabled returned unexpected response");
> +    try {
> +      if (load(testURL) != load("resources/simple.txt")) {

There is a slight difference here - an exception from loading resources/simple.txt would be attributed to loading testURL. Not a big deal.

[Michael Nordman]

When using php via cgi it is measurable, 2 of the 3 urls given to load() are satisfied by a php script. Under normal cirucumstances each php request adds about 100ms.

I can eliminate the multiple try/catch blocks if you think that would help.

function load(url)
{
    try {
        var req = new XMLHttpRequest();
        req.open("GET", url, false);
        req.send("");
        return req.responseText;
    catch (ex) {
        log("FAIL: Cannot load " + url + ", ex = " + ex);
        hadError = true;
        return "";  // This value should not be expected as the responseText for a url presented to this function.
    }
}


    if (load(redirectURL) != "Hello, World!") {
        log("FAIL: Loaded the wrong content for a redirect to another origin when network is enabled");
        hadError = true;
    }

[Alexey Proskuryakov]

That looks better to me, thanks!

[Michael Nordman]

Created attachment 72763 [details]
faster network simulator

new patch with changes outlined above

[Alexey Proskuryakov]

Comment on attachment 72763 [details]
faster network simulator

View in context: https://bugs.webkit.org/attachment.cgi?id=72763&action=review

> LayoutTests/ChangeLog:15
> +        3) Removed some unneeded code from offline-access.html. Appcache events are guaranteed

offline-access-frame.html here, too.

[Michael Nordman]

Committed r71196: <http://trac.webkit.org/changeset/71196>