Since we have CSP, MixedContentBlocker, CORS, and soon also SRI in dom/security, we should also host all related tests to those features in dom/security/test.

In Bug 1116624 we are also moving CORS into dom/security - Jonas, I am not sure, do we have any cors specific tests? If so, we should also move those into dom/security/test so we have all dom related security tests within dom/security. I am pretty sure, once we start moving security checks into AsyncOpen() in our revamp-project - we are going to benefit from having all those tests within one location.

I don't really care either way. Not sure that moving tests around is worth it. But if you want to, then this is the closest thing to CORS specific tests that we have: http://mxr.mozilla.org/mozilla-central/find?string=crosssitexhr

(In reply to Jonas Sicking (:sicking) from comment #4) > I don't really care either way. Not sure that moving tests around is worth > it. But if you want to, then this is the closest thing to CORS specific > tests that we have: > > http://mxr.mozilla.org/mozilla-central/find?string=crosssitexhr Thanks Jonas - I think it makes sense to have tests bundled together. Especially for people who haven't worked with our codebase and otherwise have to grep,search and find the appropriate tests. I think those people would benefit the most when code and tests are contained by the same sub-directory.

Comment on attachment 8543799 [details] [diff] [review] bug_1117650_part_1_csp.patch Review of attachment 8543799 [details] [diff] [review]: ----------------------------------------------------------------- Would it be too much of a pain to remove "CSP" from all the CSP test files' names?

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #6) > Comment on attachment 8543799 [details] [diff] [review] > bug_1117650_part_1_csp.patch > > Review of attachment 8543799 [details] [diff] [review]: > ----------------------------------------------------------------- > > Would it be too much of a pain to remove "CSP" from all the CSP test files' > names? I should have done that right away actually - since we are cleaning that stuff up, we should do it right. I am going to remove the "CSP" later today and re-flag you. Thanks!

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7) > I should have done that right away actually - since we are cleaning that > stuff up, we should do it right. I am going to remove the "CSP" later today > and re-flag you. Thanks! In fact we can eliminate the "mixed_content" as well, since all the tests are in subfolders anyway.

Comment on attachment 8543799 [details] [diff] [review] bug_1117650_part_1_csp.patch Review of attachment 8543799 [details] [diff] [review]: ----------------------------------------------------------------- Looking forward to the re-flag. Thanks for doing this, Chris.

Sid, as discussed, I stripped the "_CSP_" from all the tests.

Tanvi, I stripped the "mixed_content_" from all the files since they are all bundled together within test/mixedcontentblocker/ anyway.

Comment on attachment 8544361 [details] [diff] [review] bug_1117650_part_1_csp.patch Review of attachment 8544361 [details] [diff] [review]: ----------------------------------------------------------------- What about xpcshell tests? dom/base/test/unit/test_cspreports.js You'll have to update the reference to a CSP file here too: dom/base/test/test_warning_for_blocked_cross_site_request.html:16

Comment on attachment 8544362 [details] [diff] [review] bug_1117650_part_2_mixedcontentblocker.patch Wasn't there a separate bug for moving all mixed content tests into the same directory? I can't find it at the moment, but it lists a number of tests not covered here.

(In reply to Tanvi Vyas [:tanvi] from comment #13) > Comment on attachment 8544362 [details] [diff] [review] > bug_1117650_part_2_mixedcontentblocker.patch > > Wasn't there a separate bug for moving all mixed content tests into the same > directory? I can't find it at the moment, but it lists a number of tests > not covered here. Yes - https://bugzilla.mozilla.org/show_bug.cgi?id=1086619#c6 I thought about moving them, but those are browser tests, they don't really fit in here. I think those should remain where they are, no?

Comment on attachment 8544362 [details] [diff] [review] bug_1117650_part_2_mixedcontentblocker.patch (In reply to Christoph Kerschbaumer [:ckerschb] from comment #14) > Yes - https://bugzilla.mozilla.org/show_bug.cgi?id=1086619#c6 > I thought about moving them, but those are browser tests, they don't really > fit in here. > I think those should remain where they are, no? Ah okay. Maybe we should move those to browser/base/content/test/mixedcontent/, but that can be handled separately. r+'ing this patch.

(In reply to Tanvi Vyas [:tanvi] from comment #15) > Comment on attachment 8544362 [details] [diff] [review] > bug_1117650_part_2_mixedcontentblocker.patch > > (In reply to Christoph Kerschbaumer [:ckerschb] from comment #14) > > Yes - https://bugzilla.mozilla.org/show_bug.cgi?id=1086619#c6 > > I thought about moving them, but those are browser tests, they don't really > > fit in here. > > I think those should remain where they are, no? > > Ah okay. Maybe we should move those to > browser/base/content/test/mixedcontent/, but that can be handled separately. > r+'ing this patch. I agree, we should bundle them together as well. At the moment I just want to have all mochis in dom/security/test, which will affect our workflow once we start moving security checks into asyncOpen.

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #12) > Comment on attachment 8544361 [details] [diff] [review] > bug_1117650_part_1_csp.patch > > Review of attachment 8544361 [details] [diff] [review]: > ----------------------------------------------------------------- > > What about xpcshell tests? dom/base/test/unit/test_cspreports.js Yeah, let's move those too. Please note, that I had to include * Cu.import("resource://testing-common/httpd.js"); otherwise we can not use the http-server needed for the xpcshell tests in test_cspreports.js > You'll have to update the reference to a CSP file here too: > dom/base/test/test_warning_for_blocked_cross_site_request.html:16 Good catch!

Comment on attachment 8549152 [details] [diff] [review] bug_1117650_part_1_csp.patch Review of attachment 8549152 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b25392be437 https://hg.mozilla.org/integration/mozilla-inbound/rev/eef01ed4d406 https://hg.mozilla.org/integration/mozilla-inbound/rev/aff44058c799

"Oh, I'll just move some tests around, what could possibly go wrong, la de dah la de de," he said. Pushed a followup, https://hg.mozilla.org/integration/mozilla-inbound/rev/9318cab3bd13, because a dom/fetch test thought it could use your CORS server without having it disappear out from under it (a bad decision on its part, but what can you do?). However, I don't have a followup for the other problem, that running WebRTC right before shutdown causes graphics to assert while shutting down. Because you moved your tests from alphabetically before media/ to after it, you moved the WebRTC tests from running early in linux debug mochitest-e10s chunk 3 to running last in chunk 2, which then makes bug 1091322 happen constantly. Have fun fighting it out in bug 1091322 to get one or the other of them to fix that problem so that you can move your tests :| Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/218c8f17c77f

There's been some progress made over in bug 1091322 via some deps. Any chance you could rebase these patches for a fresh Try run? Don't forget philor's follow-up fix for the fetch tests too :)

I combined the patches from this ticket (including :philor's patch) with the WIP code from bug 1122722 and the result looks very green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cabfcd658a3

Thanks for the update Ryan. Chatted with Nils in person. As soon as he landed bug 1122722, I am going to land this one again.

Just unbitrot!

Just unbitrot!

Just unbitrot and including the missing part that philor pushed as a follow up previously!

Passes locally, but to make sure I pushed to try as well: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ce60fb80a75

https://hg.mozilla.org/integration/mozilla-inbound/rev/eed379ade07e https://hg.mozilla.org/integration/mozilla-inbound/rev/646b7dddf7c5 https://hg.mozilla.org/integration/mozilla-inbound/rev/73d0f6b58639

Whatever magic you were thinking might cause that WebRTC ASan mochitest-2 leak that was permaorange on try to go away didn't happen. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/be57b67e1bc8

Oh boy - Nils, any ideas?

Additional note: In particular I am curious why everything turned green in your try run from comment 22: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cabfcd658a3 and in my try run from Comment 27: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ce60fb80a75 why get mostly green for Mochitest(2) but not always - still seems very intermittent to me. Let me know if I can be of any help Nils! Thanks Phil for backing out - sorry about that!

I believe these leaks in the video engines exist for quite some time already. Not sure why these are popping up now in your build. Maybe these leaks are actually on the ignore list already, but for some reason the white listing no longer works?! Randell do you what the status of these video engine leaks is?

I don't think these are on the ignore list... However, moving tests around can cause cleanup to happen "later" (since we seem to be on the tail end of the testrun now), and that may be causing a shutdown race leak

(In reply to Randell Jesup [:jesup] from comment #33) > I don't think these are on the ignore list... However, moving tests around > can cause cleanup to happen "later" (since we seem to be on the tail end of > the testrun now), and that may be causing a shutdown race leak Thanks Randell for the info - is there anything we can do about it? We are about to move security checks into AsyncOpen and it would be great if we have all dom/security related tests bundled together which would greatly improve the development process.

Rebased all three bugs - all yours Nils :-)

It has been a while. Rebased all of those patches again.

Nils, I rebased the patches and pushed to TRY again [1]. Maybe we get lucky and stuff changed enough that the WebRTC problems magically disappear :-) [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b2f45fa3098

Oh boy - hg add dom/security/test/unit/xpcshell.ini :-)

Here we go: https://treeherder.mozilla.org/#/jobs?repo=try&revision=763403eb8566

Hey Nils, our plan did not work out :-( I fixed the problem on TBPL for CORS, but the Asan problem caused by WebRTC in M(2) is still present. Any chance I could convince you to look at that again?

Rebased all of those patches, carrying over r+

Here is a new TRY run, lets see if everything still works: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=4379e3175ef6 Thanks for looking into that!

To any of the sheriffs, please have a look at the latest try run before you check in the code: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=4379e3175ef6 I assume, the bc1, bc2, and bc3 problems on OS X 10.6 opt are unrelated to my patches. If you feel the same way, please check in the code, otherwise let me know and I can take yet another look - thanks! Just stating the obvious, but the patches apply in the order of part_1, part_2 and part_3.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bac223e58428 https://hg.mozilla.org/integration/mozilla-inbound/rev/651643506459 https://hg.mozilla.org/integration/mozilla-inbound/rev/4ff05eb7eb53

https://hg.mozilla.org/mozilla-central/rev/bac223e58428 https://hg.mozilla.org/mozilla-central/rev/651643506459 https://hg.mozilla.org/mozilla-central/rev/4ff05eb7eb53