1117650 - Consider moving all dom-security related tests into dom/security/test

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1117650_part_1_csp.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1117650_part_2_mixedcontentblocker.patch

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1117650_part_3_cors.patch

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

[Sid Stamm [:geekboy or :sstamm]]

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?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

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

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1117650_part_1_csp.patch

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

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1117650_part_2_mixedcontentblocker.patch

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

Comment 12

[Sid Stamm [:geekboy or :sstamm]]

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 13

[Tanvi Vyas[:tanvi]]

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.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

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

[Tanvi Vyas[:tanvi]]

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.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1117650_part_1_csp.patch

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

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8549152 [details] [diff] [review]
bug_1117650_part_1_csp.patch

Review of attachment 8549152 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 20

[Phil Ringnalda (:philor)]

"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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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 :)

Comment 22

[Nils Ohlmeier [:drno]]

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

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1117650_part_1_csp.patch

Just unbitrot!

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1117650_part_2_mixedcontentblocker.patch

Just unbitrot!

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1117650_part_3_cors.patch

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

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 29

[Phil Ringnalda (:philor)]

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

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Oh boy - Nils, any ideas?

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

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!

Comment 32

[Nils Ohlmeier [:drno]]

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?

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

Attachment: rebased_bug_1117650_part_1_csp.patch

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Attachment: rebased_bug_1117650_part_2_mixedcontentblocker.patch

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

Attachment: rebased_bug_1117650_part_3_cors.patch

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

Rebased all three bugs - all yours Nils :-)

Comment 39

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1117650_part_1_csp.patch

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

Comment 40

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1117650_part_2_mcb.patch

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1117650_part_3_cors.patch

Comment 42

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 43

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1117650_part_1_csp.patch

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

Comment 44

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 45

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1117650_part_3_cors.patch

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?

Comment 46

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1117650_part_1_cors.patch

Rebased all of those patches, carrying over r+

Comment 47

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1117650_part_2_mcb.patch

Comment 48

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1117650_part_3_csp.patch

Comment 49

[Christoph Kerschbaumer [:ckerschb]]

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!

Comment 50

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 51

[Pulsebot]

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

Comment 52

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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