Bug 92649

The diff looks correct, sorry for not checking the other ports before landing. I'll fix it tonight when I'm back in front of a computer.

Why r123978 caused this duplicated console message?

Problem exist on Qt5, too. http/tests/security/contentSecurityPolicy/object-src-none-blocked.html test makes http/tests/security/xss-DENIED-xsl-document-securityOrigin.xml fail after the commit r123978. Before this commit it had no problem with "logifloaded". --- /ramdisk/qt-linux-release/build/layout-test-results/http/tests/security/xss-DENIED-xsl-document-securityOrigin-expected.txt +++ /ramdisk/qt-linux-release/build/layout-test-results/http/tests/security/xss-DENIED-xsl-document-securityOrigin-actual.txt @@ -1,3 +1,5 @@ +CONSOLE MESSAGE: Refused to load the object 'data:application/x-webkit-test-netscape,logifloaded' because it violates the following Content Security Policy directive: "object-src 'none'". + CONSOLE MESSAGE: Unsafe JavaScript attempt to access frame with URL http://localhost:8080/security/resources/innocent-victim.html from frame with URL about:blank. Domains, protocols and ports must match. -This test passes if it doesn't alert the contents of innocent-victim.html. +This test passes if it doesn't alert the contents of innocent-victim.html.

Test skipped on Qt in http://trac.webkit.org/changeset/124030. Please unskip it with the proper fix.

(In reply to comment #2) > Why r123978 caused this duplicated console message? I spoke too quickly; I didn't see that the console message was being duplicated. That diff makes no sense.

Great, skipping a test made another one fail, but only on WK2: --- /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/http/tests/security/contentSecurityPolicy/object-src-no-url-blocked-expected.txt +++ /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/http/tests/security/contentSecurityPolicy/object-src-no-url-blocked-actual.txt @@ -1,3 +1,5 @@ +CONSOLE MESSAGE: Refused to load the object '' because it violates the following Content Security Policy directive: "object-src 'none'". + CONSOLE MESSAGE: Refused to load the object '' because it violates the following Content Security Policy directive: "object-src 'none'". This test passes if there is a console message saying the plugin was blocked.

Merging 92963 into this bug. Adam suggested that async console logs from one test might be leaking into the next. This could certainly be the case, given that plugin loading is wonky. I'll post a patch in a moment that adds delays and see how it goes. If it fixes the issues, I'll unskip the tests on the various ports.

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

Created attachment 156356 [details] Patch

This is a bit of a mess. Console messages are leaking into the next test, partially because we're generating more messages than we should when blocking plugins via Content Security Policy. The attached patch adds a new PluginUnavailabilityReason, and marks the plugin element's renderer as unavailable when CSP blocks the plugin. This should prevent additional console messages from being generated, and works in my local testing (NRWT, running each test in the `security/contentSecurityPolicy` directory 20 times). WDYT, Jochen and Adam?

Comment on attachment 156356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156356&action=review > Source/WebCore/loader/SubframeLoader.cpp:-130 > - return false; I assume you removed this check because you don't have a renderer yet at this point? Are there code paths that go through requestPlugin but never loadPlugin?

Comment on attachment 156356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156356&action=review >> Source/WebCore/loader/SubframeLoader.cpp:-130 >> - return false; > > I assume you removed this check because you don't have a renderer yet at this point? > > Are there code paths that go through requestPlugin but never loadPlugin? Right. Since someone already called out the dependence on renderer in loadPlugin as an issue, I decided that introducing it in another method would be a bad idea. loadPlugin and requestPlugin are both private methods of SubframeLoader, and so far as I can tell, loadPlugin is only called inside SubframeLoader from requestPlugin. Removing this check here seems safe.

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

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

Comment on attachment 156356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156356&action=review >>> Source/WebCore/loader/SubframeLoader.cpp:-130 >>> - return false; >> >> I assume you removed this check because you don't have a renderer yet at this point? >> >> Are there code paths that go through requestPlugin but never loadPlugin? > > Right. Since someone already called out the dependence on renderer in loadPlugin as an issue, I decided that introducing it in another method would be a bad idea. > > loadPlugin and requestPlugin are both private methods of SubframeLoader, and so far as I can tell, loadPlugin is only called inside SubframeLoader from requestPlugin. Removing this check here seems safe. Is the isSandboxed(SandboxPlugins) needed? Should we add an ASSERT?

(In reply to comment #15) > (From update of attachment 156356 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156356&action=review > > >>> Source/WebCore/loader/SubframeLoader.cpp:-130 > >>> - return false; > >> > >> I assume you removed this check because you don't have a renderer yet at this point? > >> > >> Are there code paths that go through requestPlugin but never loadPlugin? > > > > Right. Since someone already called out the dependence on renderer in loadPlugin as an issue, I decided that introducing it in another method would be a bad idea. > > > > loadPlugin and requestPlugin are both private methods of SubframeLoader, and so far as I can tell, loadPlugin is only called inside SubframeLoader from requestPlugin. Removing this check here seems safe. > > Is the isSandboxed(SandboxPlugins) needed? Should we add an ASSERT? I don't see another check for SandboxPlugins in SubframeLoader or in HTML{Object,Embed}Element. We could move that check to loadPlugin if you'd like? Moving some of these check out into HTMLXXXElement might very well make sense. I'm happy to do that, but I'd suggest doing it in a separate patch, as it's cleanup that's somewhat tangential to this bug.

Comment on attachment 156356 [details] Patch Ok. In general, these two checks should be right next to each other since they're doing very similar things, but we can clean that up in a later patch.

Comment on attachment 156356 [details] Patch I've filed a bug to follow up on the cleanup. Would you mind CQing this in the meantime?

(In reply to comment #18) > (From update of attachment 156356 [details]) > I've filed a bug to follow up on the cleanup. Would you mind CQing this in the meantime? web.it/93138

boom

Comment on attachment 156356 [details] Patch Clearing flags on attachment: 156356 Committed r124636: <http://trac.webkit.org/changeset/124636>

All reviewed patches have been landed. Closing bug.

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

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