Closed Bug 1258005 Opened 10 years ago Closed 10 years ago

Intermittent browser_test_web_manifest.js | default-src 'none' blocks mochi.test:8888 - Got This test timed out., expected csp-on-violate-policy

Categories

(Core :: DOM: Security, defect)

Product:
Core
Component:
DOM: Security
Version:
47 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox47 --- wontfix
firefox48 --- fixed

People

(Reporter: KWierso, Assigned: marcosc)

References

Details

(Keywords: intermittent-failure, Whiteboard: [domsecurity-intermittent])

Attachments

(1 file, 2 obsolete files)

Adds r=bkelly
2.82 KB, patch
Details | Diff | Splinter Review
Marcos, is that still an issue?
Flags: needinfo?(mcaceres)
Whiteboard: [domsecurity-intermittent]
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2) > Marcos, is that still an issue? Yeah :_( Looking into it now.
Flags: needinfo?(mcaceres)
Assignee: nobody → mcaceres
Depends on: 1250048
The code in bug 1250048 changes the tests a bit, so maybe... anyway, it's more important to land that one first.
Marcos, it seems landing bug 1250048 has increased the frequency of this failure: https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1258005&startday=2016-04-01&endday=2016-04-08&tree=all
Flags: needinfo?(mcaceres)
(In reply to Ben Kelly [:bkelly] from comment #13) > Marcos, it seems landing bug 1250048 has increased the frequency of this > failure: > > https://brasstacks.mozilla.com/orangefactor/ > ?display=Bug&bugid=1258005&startday=2016-04-01&endday=2016-04-08&tree=all Sadness. I'll try upping the timeout value to 10 seconds: https://github.com/mozilla/gecko-dev/blob/master/dom/security/test/csp/browser_test_web_manifest.js#L222
Flags: needinfo?(mcaceres)
Is there a less-flaky way this test could work? Using setTimeout is pretty much asking for orange.
Also, given the high frequency of this failure, it needs help or disabling sooner rather than later.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16) > Is there a less-flaky way this test could work? Using setTimeout is pretty > much asking for orange. You are right. I was using setTimeout during development should have removed it. If the observer that this code relies on doesn't fire, then we have bigger problems. As such, I will remove the timeout. (In reply to Ryan VanderMeulen [:RyanVM] from comment #18) > Also, given the high frequency of this failure, it needs help or disabling > sooner rather than later. Sorry, I thought bug 1250048 would fix this (landed friday). But it didn't:( I'm on it.
Attached patch Remove setTiemout (obsolete) — — Splinter Review
Removes setTimeout(). Just waits for promise to settle instead. In theory, the following is an invariant that should always fire: SpecialPowers.addObserver(observer, "csp-on-violate-policy", false); Otherwise, it's an actual regression or we have bigger problems in the underlying CSP machinery.
Attachment #8739816 - Flags: review?(ryanvm)
Comment on attachment 8739816 [details] [diff] [review] Remove setTiemout Wish I could help you, but I'm not a peer of this code. That said, this doesn't look like the right patch? :)
Attachment #8739816 - Flags: review?(ryanvm)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #22) > Comment on attachment 8739816 [details] [diff] [review] > Remove setTiemout > > Wish I could help you, but I'm not a peer of this code. That said, this > doesn't look like the right patch? :) Woops! Too many patches starting 000*. Will ask bkelly for review.
Attachment #8739816 - Attachment is obsolete: true
Attachment #8739817 - Flags: review?(bkelly)
Comment on attachment 8739817 [details] [diff] [review] 0001-Bug-1258005-Remove-setTimeout-to-avoid-intermittent-.patch Review of attachment 8739817 [details] [diff] [review]: ----------------------------------------------------------------- Looks reasonable, but I didn't reverse engineer the entire test. It seems we don't need the timeout because the try/catch checks that the block occurs. Is that correct?
Attachment #8739817 - Flags: review?(bkelly) → review+
> Looks reasonable, but I didn't reverse engineer the entire test. It seems we don't need the timeout because the try/catch checks that the block occurs. Is that correct? Yep, that is correct.
Attached patch Adds r=bkelly — — Splinter Review
Attachment #8739817 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a956dc9f9398
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
I can't tell if the Robot is still reporting failures from after the bug was fixed. I guess I'll wait another day to see if the OrangeFactor bot comes back.
Looks like this is indeed fixed on trunk. Any chance we can get a rebased patch for Aurora47 as well? :)
status-firefox47: --- → affected
Flags: needinfo?(mcaceres)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #35) > Looks like this is indeed fixed on trunk. Any chance we can get a rebased > patch for Aurora47 as well? :) Sure, but when I `git checkout remote/aurora`, it seems that the patch is already applied? Or is that different from 47? If so, how do I checkout version 47 to make a patch for it?
Flags: needinfo?(mcaceres) → needinfo?(ryanvm)
The trees moved today. 47 is now on beta.
Flags: needinfo?(ryanvm)
Woot!
Umm, 46 is still on Beta for another 6 weeks :)
Flags: needinfo?(mcaceres)
Gah, s/46/47
I tried cherry-picking the patch onto `remote/mozilla/beta`, but it seems beta is missing other things so the tests fail, unfortunately. Tomorrow, I can try just removing the setTimeout and seeing how we go... might save me having to back-port other stuff.
Flags: needinfo?(mcaceres)
Not worth it at this point.
status-firefox47: affected → wontfix
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: