Bug 86191 – Mac DRT should be able to load external URLs for replay performance tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Ryosuke Niwa]

For https://bugs.webkit.org/show_bug.cgi?id=84008, we want to be able to load URLs like http://www.google.com/ on DRT, and transparently forwarded to localhost by proxy setting.

So we need to let DRT load external resources when the main frame's URL itself is hosted on non-localhost http or https.

[Alexey Proskuryakov]

Please don't just drop the block unconditionally, it's these for a reason (it used to be not uncommon that tests with remote links were checked in).

[Ryosuke Niwa]

(In reply to comment #1)
> Please don't just drop the block unconditionally, it's these for a reason (it used to be not uncommon that tests with remote links were checked in).

My plan is to check the URL of the main resource, and allow external resource loading only if that's also an external URL. e.g. DumpRenderTree http://www.google.com/ can load external resources whereas DumpRenderTree test.html or DumpRenderTree http://localhost:8000/ can't.

[Eric Seidel (no email)]

It may still be common. :)  We used to have flaky tests which would flake based on your network connectivity.

[Ryosuke Niwa]

Created attachment 141497 [details]
Adds the feature

[Ryosuke Niwa]

e.g. I've also converted Parser/tiny-HTML.html and obtain the following results. 
It indicates that runs/s version no only several times faster to run, but it also produces more statistically sound result. Observe that the standard deviation between mean of runs is smaller than the standard deviation of individual runs for run/s, but it's bigger for ms.

RESULT Parser: tiny-innerHTML= 5.13164237919 runs/s
median= 5.14138817481 runs/s, stdev= 0.0394345880796 runs/s, min= 5.03778337531 runs/s, max= 5.19480519481 runs/s

RESULT Parser: tiny-innerHTML= 5.1284352539 runs/s
median= 5.13478818999 runs/s, stdev= 0.0342742796963 runs/s, min= 5.05689001264 runs/s, max= 5.17464424321 runs/s

RESULT Parser: tiny-innerHTML= 5.1199600961 runs/s
median= 5.12163892446 runs/s, stdev= 0.0314570907516 runs/s, min= 5.03778337531 runs/s, max= 5.16795865633 runs/s

RESULT Parser: tiny-innerHTML= 5.11276918215 runs/s
median= 5.12163892446 runs/s, stdev= 0.029477288025 runs/s, min= 5.05050505051 runs/s, max= 5.15463917526 runs/s

RESULT Parser: tiny-innerHTML= 5.12375369947 runs/s
median= 5.12163892446 runs/s, stdev= 0.0351098248412 runs/s, min= 5.05689001264 runs/s, max= 5.18806744488 runs/s

stat([5.13164237919, 5.1284352539, 5.1199600961, 5.11276918215, 5.12375369947])
{'min': 5.113, 'max': 5.132, 'median': 5.120, 'stdev': 0.014772, 'avg': 5.1233, 'unit': 'ms'}


RESULT Parser: tiny-innerHTML= 1940.45 ms
median= 1942.0 ms, stdev= 8.56431550096 ms, min= 1922.0 ms, max= 1955.0 ms

RESULT Parser: tiny-innerHTML= 1946.1 ms
median= 1943.0 ms, stdev= 11.5147731198 ms, min= 1929.0 ms, max= 1977.0 ms

RESULT Parser: tiny-innerHTML= 1955.6 ms
median= 1953.5 ms, stdev= 9.60937042683 ms, min= 1940.0 ms, max= 1975.0 ms

RESULT Parser: tiny-innerHTML= 1945.2 ms
median= 1945.0 ms, stdev= 9.61041102139 ms, min= 1928.0 ms, max= 1970.0 ms

RESULT Parser: tiny-innerHTML= 1948.5 ms
median= 1948.0 ms, stdev= 8.86284378741 ms, min= 1936.0 ms, max= 1976.0 ms

stat([1940.45, 1946.1, 1955.6, 1945.2, 1948.5])
{'min': 1940.45, 'max': 1955.60, 'median': 1955.60, 'stdev': 11.091, 'avg': 1947.17, 'unit': 'ms'}

[Ryosuke Niwa]

Ugh... stat function used in the previous post had a slight bug. They should have been:

stat([5.13164237919, 5.1284352539, 5.1199600961, 5.11276918215, 5.12375369947])
{'min': 5.11277, 'max': 5.13164, 'median': 5.12375, 'stdev': 0.0147724, 'avg': 5.12331, 'unit': 'ms'}

stat([1940.45, 1946.1, 1955.6, 1945.2, 1948.5])
{'min': 1940.45, 'max': 1955.60, 'median': 1946.10, 'stdev': 11.09, 'avg': 1947.17, 'unit': 'ms'}

[Alexey Proskuryakov]

Comment on attachment 141497 [details]
Adds the feature

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

The idea is perfectly good. I think that the patch would benefit from another iteration.

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:132
> +    return NSOrderedSame == [host compare:@"127.0.0.1"] || NSOrderedSame == [host caseInsensitiveCompare:@"localhost"];

We block IPv6 today, but it seems somewhat undesirable to rely on that here.

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:135
> +BOOL hostIsUsedBySomeTestToGetError(const NSString* host)

I'd say "tests", not "test", and also probably "simulate failure" or "generate error".

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:164
> +        NSURL *testUrl = done ? 0 : [[NSURL alloc] initWithString: [NSString stringWithUTF8String: gLayoutTestController->testPathOrURL().c_str()]];
> +        NSString *testUrlHost = testUrl ? [testUrl host] : 0;

s/Url/URL/

The function you use is named "testPathOrURL". Why is it OK to just create a URL from its result? This looks suspicious.

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:166
> +        if (!testUrlHost || (isLocalhost(testUrlHost) && !isLocalhost(host)) || hostIsUsedBySomeTestToGetError(host)) {
> +            printf("Blocked access to external URL %s\n", [[url absoluteString] cStringUsingEncoding:NSUTF8StringEncoding]);

This message is untrue when testUrlHost is nil.

Also, it looks like the message will be printed if hostIsUsedBySomeTestToGetError returns true, which is a change from original behavior.

[Ryosuke Niwa]

Comment on attachment 141497 [details]
Adds the feature

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

>> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:132
>> +    return NSOrderedSame == [host compare:@"127.0.0.1"] || NSOrderedSame == [host caseInsensitiveCompare:@"localhost"];
> 
> We block IPv6 today, but it seems somewhat undesirable to rely on that here.

Will add a FIXME.

>> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:135
>> +BOOL hostIsUsedBySomeTestToGetError(const NSString* host)
> 
> I'd say "tests", not "test", and also probably "simulate failure" or "generate error".

Will fix.

>> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:164
>> +        NSString *testUrlHost = testUrl ? [testUrl host] : 0;
> 
> s/Url/URL/
> 
> The function you use is named "testPathOrURL". Why is it OK to just create a URL from its result? This looks suspicious.

Will fix.

>> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:166
>> +            printf("Blocked access to external URL %s\n", [[url absoluteString] cStringUsingEncoding:NSUTF8StringEncoding]);
> 
> This message is untrue when testUrlHost is nil.
> 
> Also, it looks like the message will be printed if hostIsUsedBySomeTestToGetError returns true, which is a change from original behavior.

Nice catch! Will fix.

[Ryosuke Niwa]

Comment on attachment 141497 [details]
Adds the feature

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

>>> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:164
>>> +        NSString *testUrlHost = testUrl ? [testUrl host] : 0;
>> 
>> s/Url/URL/
>> 
>> The function you use is named "testPathOrURL". Why is it OK to just create a URL from its result? This looks suspicious.
> 
> Will fix.

Actually the current code is correct because NSURL initWithString returns nil if the URL was malformed (e.g. it was a file path).
Detecting whether it was a URL or not would require case-insensitive prefix comparison of strings, which neither WTFString nor NSString seems to support.

[Alexey Proskuryakov]

> Actually the current code is correct because NSURL initWithString returns nil if the URL was malformed (e.g. it was a file path).

Hmm. I'm not sure if this is correct (despite documentation). See e.g. bug 86265, where we've been getting an invalid URL, and I suspect that it was actually created with -initWithString.

[Ryosuke Niwa]

Created attachment 143164 [details]
Fixed per Alexey's comment

[Alexey Proskuryakov]

Comment on attachment 143164 [details]
Fixed per Alexey's comment

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

Looks better. I think that I should look at the final variant once more, so r-.

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:130
> +BOOL isLocalhost(const NSString *host)

There should be no "const" on NSString - this class is immutable.

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:136
> +BOOL hostIsUsedBySomeTestsToToGenerateError(const NSString *host)

Ditto.

ToTo?

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:164
> +        NSString *testURLString = [NSString stringWithUTF8String: gLayoutTestController->testPathOrURL().c_str()];

In WebKit code, there is usually no space after colon.

Maybe the variable should be called "testPathOrURLString" to match function name?

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:165
> +        NSString *lowercaseTestURL = [testURLString lowercaseString];

I would just combine these into a chain call: 

NSString *lowercaseTestPathOrURL = [[NSString stringWithUTF8String: gLayoutTestController->testPathOrURL().c_str()] lowercaseString];

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:167
> +        if ([lowercaseTestURL hasPrefix: [NSString stringWithCString: "http:"]] || [lowercaseTestURL hasPrefix: [NSString stringWithCString: "https:"]]) {

Instead of stringWithCString, please use literals: @"http:".

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:169
> +            NSURL *testURL = [[NSURL alloc] initWithString: testURLString];
> +            testHost = testURL ? [testURL host] : 0;

Alloc/init create an object that needs to be released. Also, it's usually fine to call methods on nil objects (with rare exceptions that depend on return type). A better way to write this is:

testHost = [[NSURL URLWithString:testURLString] host];

[Ryosuke Niwa]

Thanks for the review again. Will fix all of them except:

(In reply to comment #12)
> > Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:165
> > +        NSString *lowercaseTestURL = [testURLString lowercaseString];
> 
> I would just combine these into a chain call: 
> 
> NSString *lowercaseTestPathOrURL = [[NSString stringWithUTF8String: gLayoutTestController->testPathOrURL().c_str()] lowercaseString];

We would need non-lowercased version for instantiating URL.

[Ryosuke Niwa]

Created attachment 143603 [details]
More fixes per Alexey's comment

[Alexey Proskuryakov]

Comment on attachment 143603 [details]
More fixes per Alexey's comment

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

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:132
> +    // FIXME: Support IPv6 loopbacks

Nit: period at the end of  sentence.

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:165
> +        NSString *lowercaseTestURL = [testPathOrURL lowercaseString];

Should this be lowercaseTestPathOrURL, too?

[Ryosuke Niwa]

Comment on attachment 143603 [details]
More fixes per Alexey's comment

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

>> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:165
>> +        NSString *lowercaseTestURL = [testPathOrURL lowercaseString];
> 
> Should this be lowercaseTestPathOrURL, too?

Yes! Fixed.

[Ryosuke Niwa]

Committed r118231: <http://trac.webkit.org/changeset/118231>