third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html - Issue 1849223002: Blocking synchronous and third party doc.written scripts

Side by Side Diff: third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html

Issue 1849223002: Blocking synchronous and third party doc.written scripts (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Added layout tests and review comments incorporation Created 4 years, 8 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« no previous file with comments | « no previous file | third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block-expected.txt » ('j') | third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block1.html » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <script src="../resources/testharness.js"></script>

	3 <script src="../resources/testharnessreport.js"></script>

	4 <script>

	5 var t = async_test('Makes sure synchronous third-party scripts in document.w rite are blocked');
	Bryan McQuade 2016/04/05 12:46:11 Since these scripts are loaded synchronously you m Since these scripts are loaded synchronously you may not need to use an async test either (though i dont know layout tests well enough to say for sure). Also, looks like this description does not match the test (same for some other tests). I'm not familiar with layout tests. is convention to use the same description for all related tests, or to make the description of each test match the thing it is testing? shivanisha 2016/04/05 20:37:14 Sounds good. changing to sync. Show quoted text On 2016/04/05 at 12:46:11, Bryan McQuade wrote: > Since these scripts are loaded synchronously you may not need to use an async test either (though i dont know layout tests well enough to say for sure). > > Also, looks like this description does not match the test (same for some other tests). I'm not familiar with layout tests. is convention to use the same description for all related tests, or to make the description of each test match the thing it is testing? Sounds good. changing to sync.
	6 var src = './resources/js-loaded.js';

	7 var jsLoaded = false;

	8 if (window.internals){

	9 internals.settings.setDisallowFetchForDocWrittenScriptsInMainFrame(true) ;

	10 internals.setNetworkStateNotifierTestOnly(true);

	11 internals.setNetworkConnectionInfo('cellular2g', 2.0);

	12 }

	13 document.write('<scr' + 'ipt src="' + src + '"></scr' + 'ipt>');
	Bryan McQuade 2016/04/05 12:46:11 though it makes the test a bit longer, i tend to l though it makes the test a bit longer, i tend to like asserting both the positive and negative case in the same test. that way, since both script loads are running in the same test environment, we are confirming that the difference in behavior is due to the origin difference, rather than due to e.g. an accidental test misconfiguration that happens to produce the intended result. so for example, we can load 2 scripts one after the other in this test, one same origin and one cross origin, and verify that the same origin one succeeds while the cross origin one fails. going a step further, we should also be able to verify that the cross origin script loads correctly before you configure the test environment, to make sure it's the enabling of the window.internals settings that causes the difference in behavior there (rather than e.g. the script not being served correctly when requested on port 8000 which would also result in a failed load): var jsLoaded = false; document.write(...crossOriginScript...); assert_true(jsLoaded, ...); if (window.internals){ ... configure test environment, enable cross origin script blocking ... } jsLoaded = false; document.write(...crossOriginScript...); assert_false(jsLoaded, ...); jsLoaded = false; document.write(...sameOriginScript...); assert_true(jsLoaded, ...); Testing these 3 conditions in the same test flow lets us be more confident that it's the enabling of internals settings that causes cross origin scripts to get blocked. I have encountered many tests that pass for the wrong reasons, so I think it's really valuable to validate both positive and negative cases in the same test flow. If layout tests tend to not like this approach, and want to test each case in isolation, we can go that route. See also the comment below about a common helper function to load the script and assert the expected results, which should be useful here. Note that if caching is enabled, then the second load of the cross origin script may succeed because it's in cache. i'm not sure if there's a way to disable caching or not. if there is a way to control caching, it'd be good to have a test that verifies that once a script is cached, it successfully loads even if it's cross origin. shivanisha 2016/04/05 20:37:14 sounds good. Added 4 tests in 1 html file: cross Show quoted text On 2016/04/05 at 12:46:11, Bryan McQuade wrote: > though it makes the test a bit longer, i tend to like asserting both the positive and negative case in the same test. that way, since both script loads are running in the same test environment, we are confirming that the difference in behavior is due to the origin difference, rather than due to e.g. an accidental test misconfiguration that happens to produce the intended result. > > so for example, we can load 2 scripts one after the other in this test, one same origin and one cross origin, and verify that the same origin one succeeds while the cross origin one fails. > > going a step further, we should also be able to verify that the cross origin script loads correctly before you configure the test environment, to make sure it's the enabling of the window.internals settings that causes the difference in behavior there (rather than e.g. the script not being served correctly when requested on port 8000 which would also result in a failed load): > > > var jsLoaded = false; > document.write(...crossOriginScript...); > assert_true(jsLoaded, ...); > > if (window.internals){ > ... configure test environment, enable cross origin script blocking ... > } > > jsLoaded = false; > document.write(...crossOriginScript...); > assert_false(jsLoaded, ...); > > jsLoaded = false; > document.write(...sameOriginScript...); > assert_true(jsLoaded, ...); > > Testing these 3 conditions in the same test flow lets us be more confident that it's the enabling of internals settings that causes cross origin scripts to get blocked. > > I have encountered many tests that pass for the wrong reasons, so I think it's really valuable to validate both positive and negative cases in the same test flow. If layout tests tend to not like this approach, and want to test each case in isolation, we can go that route. > > See also the comment below about a common helper function to load the script and assert the expected results, which should be useful here. > > Note that if caching is enabled, then the second load of the cross origin script may succeed because it's in cache. i'm not sure if there's a way to disable caching or not. if there is a way to control caching, it'd be good to have a test that verifies that once a script is cached, it successfully loads even if it's cross origin. sounds good. Added 4 tests in 1 html file: cross origin script gets loaded before feature is enabled feature enabled same domain script not blocked same cross origin script not blocked as it was cached different cross origin script blocked
	14 </script>

	15 <script>

	16 window.addEventListener("load", t.step_func(function() {

	17 assert_true(jsLoaded, "Same domain doc.written scripts are not blocked") ;

	18 t.done();

	19 }));

	20 window.addEventListener('beforeunload', function() {

	21 internals.setNetworkStateNotifierTestOnly(false);

	22 internals.settings.setDisallowFetchForDocWrittenScriptsInMainFrame(false );

	23 }, false);

	24 </script>

OLD	NEW