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

RESOLVED FIXED in Firefox 41

Status

()

Core
DOM: Security
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(3 attachments, 17 obsolete attachments)

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
(Assignee)

Description

4 years ago
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)

Updated

4 years ago
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 1

4 years ago
Created attachment 8543799 [details] [diff] [review]
bug_1117650_part_1_csp.patch
Attachment #8543799 - Flags: review?(sstamm)
(Assignee)

Comment 2

4 years ago
Created attachment 8543800 [details] [diff] [review]
bug_1117650_part_2_mixedcontentblocker.patch
Attachment #8543800 - Flags: review?(tanvi)
(Assignee)

Comment 3

4 years ago
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)
(Assignee)

Comment 5

4 years ago
Created attachment 8544071 [details] [diff] [review]
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.
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?
(Assignee)

Comment 7

4 years ago
(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!
(Assignee)

Comment 8

4 years ago
(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)
(Assignee)

Comment 10

4 years ago
Created attachment 8544361 [details] [diff] [review]
bug_1117650_part_1_csp.patch

Sid, as discussed, I stripped the "_CSP_" from all the tests.
Attachment #8543799 - Attachment is obsolete: true
Attachment #8544361 - Flags: review?(sstamm)
(Assignee)

Comment 11

4 years ago
Created attachment 8544362 [details] [diff] [review]
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.
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.
(Assignee)

Comment 14

4 years ago
(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+
(Assignee)

Comment 16

4 years ago
(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.
(Assignee)

Comment 17

4 years ago
Created attachment 8549152 [details] [diff] [review]
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!
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
(Assignee)

Comment 23

4 years ago
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)
(Assignee)

Comment 24

3 years ago
Created attachment 8560682 [details] [diff] [review]
bug_1117650_part_1_csp.patch

Just unbitrot!
Attachment #8544071 - Attachment is obsolete: true
Attachment #8544362 - Attachment is obsolete: true
Attachment #8549152 - Attachment is obsolete: true
Attachment #8560682 - Flags: review+
(Assignee)

Comment 25

3 years ago
Created attachment 8560683 [details] [diff] [review]
bug_1117650_part_2_mixedcontentblocker.patch

Just unbitrot!
Attachment #8560683 - Flags: review+
(Assignee)

Comment 26

3 years ago
Created attachment 8560684 [details] [diff] [review]
bug_1117650_part_3_cors.patch

Just unbitrot and including the missing part that philor pushed as a follow up previously!
Attachment #8560684 - Flags: review+
(Assignee)

Comment 27

3 years ago
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
(Assignee)

Comment 30

3 years ago
Oh boy - Nils, any ideas?
Flags: needinfo?(drno)
(Assignee)

Comment 31

3 years ago
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)
(Assignee)

Comment 34

3 years ago
(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.
(Assignee)

Comment 35

3 years ago
Created attachment 8584787 [details] [diff] [review]
rebased_bug_1117650_part_1_csp.patch
Attachment #8560682 - Attachment is obsolete: true
Attachment #8560683 - Attachment is obsolete: true
Attachment #8560684 - Attachment is obsolete: true
Attachment #8584787 - Flags: review+
(Assignee)

Comment 36

3 years ago
Created attachment 8584788 [details] [diff] [review]
rebased_bug_1117650_part_2_mixedcontentblocker.patch
Attachment #8584788 - Flags: review+
(Assignee)

Comment 37

3 years ago
Created attachment 8584789 [details] [diff] [review]
rebased_bug_1117650_part_3_cors.patch
Attachment #8584789 - Flags: review+
(Assignee)

Comment 38

3 years ago
Rebased all three bugs - all yours Nils :-)
Flags: needinfo?(drno)
(Assignee)

Comment 39

3 years ago
Created attachment 8604935 [details] [diff] [review]
bug_1117650_part_1_csp.patch

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+
(Assignee)

Comment 40

3 years ago
Created attachment 8604936 [details] [diff] [review]
bug_1117650_part_2_mcb.patch
Attachment #8604936 - Flags: review+
(Assignee)

Comment 41

3 years ago
Created attachment 8604937 [details] [diff] [review]
bug_1117650_part_3_cors.patch
Attachment #8604937 - Flags: review+
(Assignee)

Comment 42

3 years ago
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)
(Assignee)

Comment 43

3 years ago
Created attachment 8604966 [details] [diff] [review]
bug_1117650_part_1_csp.patch

Oh boy - hg add dom/security/test/unit/xpcshell.ini :-)
Attachment #8604935 - Attachment is obsolete: true
Attachment #8604966 - Flags: review+
(Assignee)

Comment 45

3 years ago
Created attachment 8605336 [details] [diff] [review]
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?
Attachment #8604937 - Attachment is obsolete: true
Flags: needinfo?(drno)
Attachment #8605336 - Flags: review+
(Assignee)

Comment 46

3 years ago
Created attachment 8616979 [details] [diff] [review]
bug_1117650_part_1_cors.patch

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+
(Assignee)

Comment 47

3 years ago
Created attachment 8616980 [details] [diff] [review]
bug_1117650_part_2_mcb.patch
Attachment #8616980 - Flags: review+
(Assignee)

Comment 48

3 years ago
Created attachment 8616982 [details] [diff] [review]
bug_1117650_part_3_csp.patch
Attachment #8616982 - Flags: review+
(Assignee)

Updated

3 years ago
Flags: needinfo?(drno)
(Assignee)

Comment 49

3 years ago
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)
(Assignee)

Comment 50

3 years ago
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
https://hg.mozilla.org/mozilla-central/rev/bac223e58428
https://hg.mozilla.org/mozilla-central/rev/651643506459
https://hg.mozilla.org/mozilla-central/rev/4ff05eb7eb53
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: mozilla38 → mozilla41
You need to log in before you can comment on or make changes to this bug.