Closed
Bug 1176824
Opened 9 years ago
Closed 8 years ago
Intermittent browser_test_web_manifest.js | CSP manifest-src 'self' blocks cross-origin fetch. - Got This test timed out., expected csp-on-violate-policy
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: cbook, Assigned: marcosc)
References
()
Details
(Keywords: intermittent-failure)
Attachments
(1 file, 1 obsolete file)
7.28 KB,
patch
|
Details | Diff | Splinter Review |
Linux opt Mochitest e10s Mochitest e10s Browser Chrome M-e10s(bc3) https://treeherder.mozilla.org/logviewer.html#?job_id=3540155&repo=fx-team 18:01:50 INFO - 809 INFO TEST-UNEXPECTED-FAIL | dom/security/test/csp/browser_test_web_manifest.js | CSP manifest-src 'self' blocks cross-origin fetch. - Got This test timed out., expected csp-on-violate-policy
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 39•8 years ago
|
||
Marcos, the CSP directive web-manifest is a rather small feature but the tests consume quite so many resources everytime we push to try/inbound. Any chance I could convince you to slim the test down to the minimum?
Flags: needinfo?(mcaceres)
Assignee | ||
Comment 40•8 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #39) > Marcos, the CSP directive web-manifest is a rather small feature but the > tests consume quite so many resources everytime we push to try/inbound. Any > chance I could convince you to slim the test down to the minimum? Having a look.
Flags: needinfo?(mcaceres)
Assignee | ||
Comment 41•8 years ago
|
||
Going to try two things: 1. add "skipAnimation: true" to opening the tabs, which should shave a few milliseconds. 2. open all the tabs at the same time, instead of one at a time. That shouldn't cause an issues and hopefully will speed things up a bit. I can't really reduce tests themselves further, as they already test the absolutely minimum as far as CSP is concerned; but hopefully the above should be enough to stop the intermittent failures. I'll check the code though, and see if I can squeeze it a little more.
Assignee | ||
Comment 42•8 years ago
|
||
Going to try a couple of things: 1. add "skipAnimation: true" to opening the tabs, which should shave a few milliseconds. 2. open all the tabs at the same time, instead of one at a time, and let all tests run in parallel. 3. make sure timeouts are cleared when each test pass. That shouldn't cause an issues and hopefully will speed things up a bit (hopefully ~1 sec). I can't really reduce tests themselves further, as they already test the absolutely minimum as far as CSP is concerned (and noticed that a few things that should be tested are not ;(); but hopefully the above should be enough to stop the intermittent failures. I'll check the code though, and see if I can squeeze it a little more.
Assignee | ||
Comment 43•8 years ago
|
||
Tried to clean up code a bit... and fixed a couple of issues in the process and just on my machine, I saw a 1 second increase. I found a couple of busted tests, which I filed as bug 1250048. I'll send a patch with just those tests. If you have other suggestions, let me know! :) Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b793118032b7
Attachment #8721885 -
Flags: review?(mozilla)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mcaceres
Comment 44•8 years ago
|
||
Marcos, thanks for looking into optimizing those tests. The patch looks great, but I still think we don't need that many tests. I think it's enough to have one negative (blocking) test and one where CSP allows loading the web manifest. Right now we have 8 tests which is too much. Probably we should just comment some of the tests out but leave them in the file so we can reactive them? What do you think?
Flags: needinfo?(mcaceres)
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 47•8 years ago
|
||
Comment on attachment 8721885 [details] [diff] [review] Speed up attempt Review of attachment 8721885 [details] [diff] [review]: ----------------------------------------------------------------- Marcos, I am clearing my review request for now till you have answered my ni? or are you still missing some information from me? Hopefully we where not waiting on each other and created a deadlock situation here :-)
Attachment #8721885 -
Flags: review?(mozilla)
Comment 48•8 years ago
|
||
Marcos, can we eliminate some more of those tests? (See comment 44).
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 50•8 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #44) > Marcos, thanks for looking into optimizing those tests. The patch looks > great, but I still think we don't need that many tests. I think it's enough > to have one negative (blocking) test and one where CSP allows loading the > web manifest. Right now we have 8 tests which is too much. Probably we > should just comment some of the tests out but leave them in the file so we > can reactive them? What do you think? I don't feel very comfortable with disabling the tests: The tests are exercising different aspects of CSP and in combination with manifest-src. If we disable the tests, it's going to be more difficult to catch regressions. I would like us to go with the updated tests. If they still time out, then we should look at the what's actually causing them to time out.
Flags: needinfo?(mcaceres)
Comment 51•8 years ago
|
||
Comment on attachment 8721885 [details] [diff] [review] Speed up attempt Review of attachment 8721885 [details] [diff] [review]: ----------------------------------------------------------------- Ok. Then let's do it that way. Let's push them to inbound - if they still time out or cause any problems, we can re-evaluate. In general I am with you anyway. Why remove test coverage? Thanks for updating the tests.
Attachment #8721885 -
Flags: review+
Assignee | ||
Comment 52•8 years ago
|
||
Updated commit message.
Attachment #8721885 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 53•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9eaff1e23b0f
Keywords: checkin-needed
Reporter | ||
Comment 54•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9eaff1e23b0f
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 55•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/07ffad91cb33
status-firefox47:
--- → fixed
Comment 56•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/226d540bf92d
status-firefox46:
--- → fixed
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 58•8 years ago
|
||
(In reply to OrangeFactor Robot from comment #57) > 36 automation job failures were associated with this bug in the last 7 days. > > Repository breakdown: > * mozilla-inbound: 20 > * fx-team: 6 > * mozilla-beta: 4 > * try: 3 > * mozilla-aurora: 2 > * mozilla-central: 1 > > Platform breakdown: > * linux64: 24 > * linux32: 12 > > For more details, see: > https://brasstacks.mozilla.com/orangefactor/ > ?display=Bug&bugid=1176824&startday=2016-03-14&endday=2016-03-20&tree=all Noting that there are 0 issues for the 19th, which is when the patch landed... so, need to check again in a day or two to see if this is occurring again. I guess the bot will let us know.
Comment hidden (Intermittent Failures Robot) |
You need to log in
before you can comment on or make changes to this bug.
Description
•