Bug 31200 - Tests in http/tests/security/mixedContent start to fail when new tests are added
Summary: Tests in http/tests/security/mixedContent start to fail when new tests are added
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Mark Rowe (bdash)
URL:
Keywords:
: 31305 (view as bug list)
Depends on:
Blocks: 29972 31301
  Show dependency treegraph
 
Reported: 2009-11-06 00:28 PST by Roland Steiner
Modified: 2009-11-11 14:09 PST (History)
7 users (show)

See Also:


Attachments
follow-up fix for Tiger (1.54 KB, patch)
2009-11-11 13:56 PST, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roland Steiner 2009-11-06 00:28:07 PST
Discovered in r50495: when adding layout tests to the test set, http/tests/security/mixedContent/about-blank-iframe-in-main-frame.html fails as it has additional frame load delegate callbacks in the output.

To reproduce (as of writing this bug report): add more than 12 layout tests to the test set that run before http/tests/security/mixedContent/about-blank-iframe-in-main-frame.html (i.e., are before it in lexical order). Apart from the tests in fast/ruby that initially caused this, it also reproduced when duplicating fast/lists, fast/clip or fast/block/basic.

Tested on 2 different Mac Pros (only seems to occur on Mac).

[CC'ing abarth, ap, brady, as discussed with bdash on IRC]
Comment 1 Roland Steiner 2009-11-06 00:43:22 PST
Correction:
Comment 2 Roland Steiner 2009-11-06 01:40:48 PST
[Sorry about the comment spam, didn't mean to commit the previous one yet]

Corrections and additions after even more testing: 

Actually different tests in http/tests/security/mixedContent start to fail, not just about-blank-iframe-in-main-frame.html.

However, it's only ever 1 test that fails, and which test fails seems to be consistent for a given set of added tests.

Furthermore, if you remove 1 test from the added test set, the next test in line in http/tests/security/mixedContent fails. It therefore seems this is caused strictly by the number of tests (rather than any content) that come before.

And to even further add to the fun: if you add a LOT of layout tests (e.g., duplicating fast/block and fast/clip), everything runs through fine again.

This leads me to believe that perhaps something in http/tests/security/mixedContent is sensitive to some sort of run-over of the layout count. I.e., a test in this directory fails if it happens to be exactly the, say, 1000th test.
Comment 3 Beth Dakin 2009-11-10 15:32:49 PST
The bot is officially red because of this issue now. I added new tests for a new feature, and this bug is officially exposed. We should try to find a quick resolution to avoid a red bot.
Comment 4 Adam Barth 2009-11-10 15:56:56 PST
(In reply to comment #3)
> The bot is officially red because of this issue now. I added new tests for a
> new feature, and this bug is officially exposed. We should try to find a quick
> resolution to avoid a red bot.

Huh?  Why is it ok for you to land a patch that turns the tree red?  It seems fixing this bug is a re-requisite for landing that patch.
Comment 5 Mark Rowe (bdash) 2009-11-10 15:58:18 PST
(In reply to comment #4)
> (In reply to comment #3)
> > The bot is officially red because of this issue now. I added new tests for a
> > new feature, and this bug is officially exposed. We should try to find a quick
> > resolution to avoid a red bot.
> 
> Huh?  Why is it ok for you to land a patch that turns the tree red?  It seems
> fixing this bug is a re-requisite for landing that patch.

Presumably because she didn’t realize that would happen.
Comment 6 Beth Dakin 2009-11-10 16:02:04 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > The bot is officially red because of this issue now. I added new tests for a
> > > new feature, and this bug is officially exposed. We should try to find a quick
> > > resolution to avoid a red bot.
> > 
> > Huh?  Why is it ok for you to land a patch that turns the tree red?  It seems
> > fixing this bug is a re-requisite for landing that patch.
> 
> Presumably because she didn’t realize that would happen.

Indeed I did not. Also, we can not have a bug that blocks adding new tests. That's insane.
Comment 7 Eric Seidel (no email) 2009-11-10 16:02:21 PST
*** Bug 31305 has been marked as a duplicate of this bug. ***
Comment 8 Eric Seidel (no email) 2009-11-10 16:04:08 PST
It's also blocking two patches in the commit queue, see bug 31305. :)
Comment 9 Mark Rowe (bdash) 2009-11-10 17:00:22 PST
Printing out the error reveals:

main frame - didFailProvisionalLoadWithError Error Domain=NSURLErrorDomain Code=-1202 UserInfo=0x10269f380 "The certificate for this server is invalid. You might be connecting to a server that is pretending to be “127.0.0.1” which could put your confidential information at risk." Underlying Error=(Error Domain=kCFErrorDomainCFNetwork Code=-1202 UserInfo=0x1026d3400 "The certificate for this server is invalid. You might be connecting to a server that is pretending to be “127.0.0.1” which could put your confidential information at risk.")
Comment 10 Mark Rowe (bdash) 2009-11-10 17:04:57 PST
Ok, this is trivial to reproduce:

run-webkit-tests --nthly=1 http/tests/security/mixedContent

Every test fails in this manner.  If you instead did:

run-webkit-tests --nthly=2 http/tests/security/mixedContent

you'd see an interesting pattern where every second test fails in the same way.

This makes the problem relatively obvious: the first test in a given instance of DRT to use HTTPS gets extra frame load callbacks as a result of the self-signed SSL certificate.  It tells the network layer to ignore the certificate errors and then retry.  The new tests had the effect of shifting the point at which DRT was restarted, exposing this issue.
Comment 11 Mark Rowe (bdash) 2009-11-10 17:15:10 PST
I have a fix for this.
Comment 12 Mark Rowe (bdash) 2009-11-10 17:29:50 PST
Fixed in r50783.
Comment 13 Alexey Proskuryakov 2009-11-11 13:56:53 PST
Created attachment 42997 [details]
follow-up fix for Tiger

One patch per bug!
Comment 14 Darin Adler 2009-11-11 13:58:42 PST
Comment on attachment 42997 [details]
follow-up fix for Tiger

> +#if BUILDING_ON_TIGER
> +	// Initialize internal NSuRLRequest data for setAllowsAnyHTTPSCertificate:forHost: to work properly.
> +	[[[[NSURLRequest alloc] init] autorelease] HTTPMethod];
> +#endif

Bad spelling of NSURLRequest. Comment should use the phrase "work around for Tiger bug", I think.
Comment 15 Mark Rowe (bdash) 2009-11-11 13:59:13 PST
Comment on attachment 42997 [details]
follow-up fix for Tiger

> Index: WebKitTools/DumpRenderTree/mac/DumpRenderTree.mm
> ===================================================================
> --- WebKitTools/DumpRenderTree/mac/DumpRenderTree.mm	(revision 50817)
> +++ WebKitTools/DumpRenderTree/mac/DumpRenderTree.mm	(working copy)
> @@ -604,6 +604,10 @@ void dumpRenderTree(int argc, const char
>  
>      // <http://webkit.org/b/31200> In order to prevent extra frame load delegate logging being generated if the first test to use SSL
>      // is set to log frame load delegate calls we ignore SSL certificate errors on localhost and 127.0.0.1.
> +#if BUILDING_ON_TIGER
> +	// Initialize internal NSuRLRequest data for setAllowsAnyHTTPSCertificate:forHost: to work properly.
> +	[[[[NSURLRequest alloc] init] autorelease] HTTPMethod];
> +#endif

You appear to have inserted some tabs here!  And you’re missing an uppercase U in NSURLRequest.
Comment 16 Alexey Proskuryakov 2009-11-11 14:09:33 PST
> Comment should use the phrase "work around for
> Tiger bug", I think.

It's not an API, so while I'm not sure that its behavior on Tiger was formally a bug. But OK.

Committed r50842