Closed Bug 1117650 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

Attachments

(3 files, 17 obsolete files)

19.18 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
29.17 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
153.24 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attached patch bug_1117650_part_1_csp.patch (obsolete) — Splinter Review
Attachment #8543799 - Flags: review?(sstamm)
Attachment #8543800 - Flags: review?(tanvi)
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.
Flags: needinfo?(jonas)
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
Flags: needinfo?(jonas)
Attached patch bug_1117650_part_3_cors.patch (obsolete) — Splinter Review
(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.
Attachment #8544071 - Flags: review?(jonas)
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.
Attachment #8543799 - Flags: review?(sstamm)
Attached patch bug_1117650_part_1_csp.patch (obsolete) — Splinter Review
Sid, as discussed, I stripped the "_CSP_" from all the tests.
Attachment #8543799 - Attachment is obsolete: true
Attachment #8544361 - Flags: review?(sstamm)
Tanvi, I stripped the "mixed_content_" from all the files since they are all bundled together within test/mixedcontentblocker/ anyway.
Attachment #8543800 - Attachment is obsolete: true
Attachment #8543800 - Flags: review?(tanvi)
Attachment #8544362 - Flags: review?(tanvi)
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
Attachment #8544361 - Flags: review?(sstamm)
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.
Attachment #8544362 - Flags: review?(tanvi) → review+
(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.
Attached patch bug_1117650_part_1_csp.patch (obsolete) — Splinter Review
(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!
Attachment #8544361 - Attachment is obsolete: true
Attachment #8549152 - Flags: review?(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.
Attachment #8549152 - Flags: review?(sstamm) → review+
"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
Depends on: 1091322
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 :)
Flags: needinfo?(mozilla)
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
Depends on: 1122722
No longer depends on: 1091322
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.
Flags: needinfo?(mozilla)
Attached patch bug_1117650_part_1_csp.patch (obsolete) — Splinter Review
Just unbitrot!
Attachment #8544071 - Attachment is obsolete: true
Attachment #8544362 - Attachment is obsolete: true
Attachment #8549152 - Attachment is obsolete: true
Attachment #8560682 - Flags: review+
Just unbitrot!
Attachment #8560683 - Flags: review+
Attached patch bug_1117650_part_3_cors.patch (obsolete) — Splinter Review
Just unbitrot and including the missing part that philor pushed as a follow up previously!
Attachment #8560684 - Flags: review+
Passes locally, but to make sure I pushed to try as well:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ce60fb80a75
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?
Flags: needinfo?(drno)
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?
Flags: needinfo?(drno) → needinfo?(rjesup)
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
Flags: needinfo?(rjesup)
(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.
Attachment #8560682 - Attachment is obsolete: true
Attachment #8560683 - Attachment is obsolete: true
Attachment #8560684 - Attachment is obsolete: true
Attachment #8584787 - Flags: review+
Rebased all three bugs - all yours Nils :-)
Flags: needinfo?(drno)
Attached patch bug_1117650_part_1_csp.patch (obsolete) — Splinter Review
It has been a while. Rebased all of those patches again.
Attachment #8584787 - Attachment is obsolete: true
Attachment #8584788 - Attachment is obsolete: true
Attachment #8584789 - Attachment is obsolete: true
Attachment #8604935 - Flags: review+
Attached patch bug_1117650_part_2_mcb.patch (obsolete) — Splinter Review
Attachment #8604936 - Flags: review+
Attached patch bug_1117650_part_3_cors.patch (obsolete) — Splinter Review
Attachment #8604937 - Flags: review+
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
Flags: needinfo?(drno)
Attached patch bug_1117650_part_1_csp.patch (obsolete) — Splinter Review
Oh boy - hg add dom/security/test/unit/xpcshell.ini :-)
Attachment #8604935 - Attachment is obsolete: true
Attachment #8604966 - Flags: review+
Attached patch bug_1117650_part_3_cors.patch (obsolete) — Splinter Review
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?
Attachment #8604937 - Attachment is obsolete: true
Flags: needinfo?(drno)
Attachment #8605336 - Flags: review+
Rebased all of those patches, carrying over r+
Attachment #8604936 - Attachment is obsolete: true
Attachment #8604966 - Attachment is obsolete: true
Attachment #8605336 - Attachment is obsolete: true
Attachment #8616979 - Flags: review+
Flags: needinfo?(drno)
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!
Flags: needinfo?(rshenthar)
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.
Flags: needinfo?(rshenthar)
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.