Bug 86191

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).

(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.

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

Created attachment 141497 [details] Adds the feature

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'}

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'}

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.

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.

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.

> 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.

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

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];

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.

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

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?

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.

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