Bug 31200 – Tests in http/tests/security/mixedContent start to fail when new tests are added

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Roland Steiner]

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]

[Roland Steiner]

Correction:

[Roland Steiner]

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

[Beth Dakin]

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.

[Adam Barth]

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

[Mark Rowe (bdash)]

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

[Beth Dakin]

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

[Eric Seidel (no email)]

*** Bug 31305 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

It's also blocking two patches in the commit queue, see bug 31305. :)

[Mark Rowe (bdash)]

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

[Mark Rowe (bdash)]

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.

[Mark Rowe (bdash)]

I have a fix for this.

[Mark Rowe (bdash)]

Fixed in r50783.

[Alexey Proskuryakov]

Created attachment 42997 [details]
follow-up fix for Tiger

One patch per bug!

[Darin Adler]

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.

[Mark Rowe (bdash)]

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.

[Alexey Proskuryakov]

> 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