Bug 147646 - [Win] Layout Test http/tests/security/contentSecurityPolicy/source-list-parsing-10.html is failing
Summary: [Win] Layout Test http/tests/security/contentSecurityPolicy/source-list-parsi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-08-04 12:32 PDT by Brent Fulgham
Modified: 2016-07-05 22:56 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.02 KB, patch)
2016-07-05 00:04 PDT, Per Arne Vollan
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2015-08-04 12:32:54 PDT
The following layout test is failing on Windows:

http/tests/security/contentSecurityPolicy/source-list-parsing-10.html

Probable cause:

Possibly related to my CSP_NEXT changes. I'm reviewing now.
Comment 1 Brent Fulgham 2015-08-04 13:35:32 PDT
Skipped failing test in r187879 <http://trac.webkit.org/changeset/187879>.
Comment 2 Radar WebKit Bug Importer 2016-05-27 12:33:43 PDT
<rdar://problem/26522796>
Comment 3 Per Arne Vollan 2016-07-05 00:04:42 PDT
Created attachment 282751 [details]
Patch
Comment 4 Per Arne Vollan 2016-07-05 00:26:17 PDT
(In reply to comment #3)
> Created attachment 282751 [details]
> Patch

Another option is to add Windows specific test results, since Windows has enabled CSP_NEXT.
Comment 5 Brent Fulgham 2016-07-05 08:39:19 PDT
Comment on attachment 282751 [details]
Patch

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

Could you please generate a Windows result with CSP_NEXT active instead of turning off the feature?

> ChangeLog:8
> +        Disable CSP_NEXT. We can then use the common expected test results for this test. 

We should leave it activated, and provide a test result. We want CSP_NEXT available on Windows, just like it is on Mac/iOS.

> Source/cmake/OptionsWin.cmake:13
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_CSP_NEXT PUBLIC OFF)

I don't like this change. We should keep CSP_NEXT activated and just provide a test result so we can confirm it functions properly.
Comment 6 Per Arne Vollan 2016-07-05 08:52:32 PDT
(In reply to comment #5)
> Comment on attachment 282751 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282751&action=review
> 
> Could you please generate a Windows result with CSP_NEXT active instead of
> turning off the feature?
> 
> > ChangeLog:8
> > +        Disable CSP_NEXT. We can then use the common expected test results for this test. 
> 
> We should leave it activated, and provide a test result. We want CSP_NEXT
> available on Windows, just like it is on Mac/iOS.
> 
> > Source/cmake/OptionsWin.cmake:13
> > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_CSP_NEXT PUBLIC OFF)
> 
> I don't like this change. We should keep CSP_NEXT activated and just provide
> a test result so we can confirm it functions properly.

Thanks for reviewing!

I'll add Windows test results instead :)

I have probably got this all wrong, but it seems to me that CSP_NEXT is disabled on Mac? The Mac test expectations also seem to indicate this.
Comment 7 Brent Fulgham 2016-07-05 15:22:12 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 282751 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=282751&action=review
> > 
> > Could you please generate a Windows result with CSP_NEXT active instead of
> > turning off the feature?
> > 
> > > ChangeLog:8
> > > +        Disable CSP_NEXT. We can then use the common expected test results for this test. 
> > 
> > We should leave it activated, and provide a test result. We want CSP_NEXT
> > available on Windows, just like it is on Mac/iOS.
> > 
> > > Source/cmake/OptionsWin.cmake:13
> > > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_CSP_NEXT PUBLIC OFF)
> > 
> > I don't like this change. We should keep CSP_NEXT activated and just provide
> > a test result so we can confirm it functions properly.
> 
> Thanks for reviewing!
> 
> I'll add Windows test results instead :)
> 
> I have probably got this all wrong, but it seems to me that CSP_NEXT is
> disabled on Mac? The Mac test expectations also seem to indicate this.

No! I'm wrong. I just looked at the Mac build configuration and CSP_NEXT is definitely turned off.

I was confused because Dan has been moving things out of CSP_NEXT as we've been adding support for CSP 2.0, and I thought for some reason that CSP_NEXT was turned on as well.

Go ahead and deactivate CSP_NEXT on Windows.
Comment 8 Per Arne Vollan 2016-07-05 22:55:38 PDT
Thanks, Brent!
Comment 9 Per Arne Vollan 2016-07-05 22:56:39 PDT
Committed r202848: <https://trac.webkit.org/changeset/202848>