Intermittent dom/tests/mochitest/beacon/test_beaconRedirect.html | SendBeacon should follow cross origin redirects! - got "reset", expected "green"

RESOLVED FIXED in Firefox 51

Status

()

P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: aryx, Assigned: gbrown)

Tracking

({intermittent-failure})

unspecified
mozilla53
intermittent-failure
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox51 fixed, firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: [domsecurity-intermittent])

Attachments

(2 attachments)

Priority: -- → P3
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Assignee: nobody → gbrown
Created attachment 8814277 [details] [diff] [review]
increase interval to 10 seconds on android only

ckerschb previously tried increasing the interval to handle this type of failure, in bug 1290065. It looks like all the remaining failures are on Android Debug, which can be remarkably slow. Since increases in interval increase the test run time, I am reluctant to increase the interval except as needed -- so I am increasing it to 10 seconds, only on Android.

This change appears to be effective: https://treeherder.mozilla.org/#/jobs?repo=try&revision=324e13a1e66e1a17a7afd9e122b372694d825baa
Attachment #8814277 - Flags: review?(wisniewskit)
Comment on attachment 8814277 [details] [diff] [review]
increase interval to 10 seconds on android only

The idea seems reasonable to me, but I'm actually not a dom/security peer, so my review won't count for much. I'll switch over to :francois.
Attachment #8814277 - Flags: review?(wisniewskit) → review?(francois)
Attachment #8814277 - Flags: review?(francois) → review+

Comment 15

2 years ago
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/762cd8291634
Increase wait interval in test_beaconRedirect, on Android only; r=francois

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/762cd8291634
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment hidden (Intermittent Failures Robot)
status-firefox51: --- → affected
status-firefox52: --- → affected

Comment 18

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/cf6ea65ba55b
status-firefox52: affected → fixed
Flags: in-testsuite+

Comment 19

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/79b38ed02056
status-firefox51: affected → fixed
10000's not enough, maybe 100000?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'm not sure there's been any change in failure rate. Maybe I was just lucky on that try push? Or did I introduce an error with the Android condition? Will have a look...
I verified that the test is actually waiting for 10 seconds, on Android.

https://queue.taskcluster.net/v1/task/T-93Ns0GQAW0Udsj3BJiSA/runs/0/artifacts/public/test_info//logcat-emulator-5554.log

11-30 14:37:36.791   762   781 I GeckoDump: ⰲ겿{"action":"test_start","time":1480545456787,"thread":null,"pid":null,"source":"mochitest","test":"/tests/dom/tests/mochitest/beacon/test_beaconRedirect.html","js_source":"TestRunner.js"}ⰲ겿
...
11-30 14:37:52.611   762   781 E GeckoConsole: Location: http://mochi.test:8888/tests/dom/tests/mochitest/beacon/beacon-redirect-handler.sjs?verifyRedirectDidSucceed
11-30 14:37:52.611   762   781 E GeckoConsole: Line Number 1, Column 1:" {file: "http://mochi.test:8888/tests/dom/tests/mochitest/beacon/beacon-redirect-handler.sjs?verifyRedirectDidSucceed" line: 1 column: 1 source: "reset"}]
11-30 14:37:52.681   762   781 I GeckoDump: ⰲ겿{"action":"test_status","time":1480545472677,"thread":null,"pid":null,"source":"mochitest","test":"/tests/dom/tests/mochitest/beacon/test_beaconRedirect.html","subtest":"SendBeacon should follow cross origin redirects!","status":"FAIL","expected":"PASS","message":"got \"reset\", expected \"green\"","stack":"    SimpleTest.is@SimpleTest/SimpleTest.js:271:5\n    queryIfRedirectSucceeded/xhr.onload@dom/tests/mochitest/beacon/test_beaconRedirect.html:35:5\n","js_source":"TestRunner.js"}ⰲ겿
Contrasting:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64e9aa499b74b46b5ea08f78dda61d20eb64b6dd
(no additional change - 10 second interval)

with:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9896604953da7f9b72761101f5d0e6cadfbb97bf
(30 second interval)

it again appears that increasing the interval to 30 seconds might be effective on Android.

...but I am reluctant to increase the interval that much.
:ckerschb - What are your thoughts on what we should do here? I am tempted to skip-if the test on Android Debug only. I think increasing the interval significantly is an option, but not a good one. Otherwise, rewrite the test to somehow avoid the need for the setInterval()?
Flags: needinfo?(ckerschb)
Another thought: Why setInterval() rather than setTimeout()? With clearInterval() called in queryIfRedirectSucceeded(), it looks to me like queryIfRedirectSucceeded() will only ever be called once. I just ask because setInterval() suggests periodic retries to me, but I don't think that ever happens...want to make sure that was intentional.
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
(In reply to Geoff Brown [:gbrown] from comment #24)
> :ckerschb - What are your thoughts on what we should do here? I am tempted
> to skip-if the test on Android Debug only. I think increasing the interval
> significantly is an option, but not a good one. Otherwise, rewrite the test
> to somehow avoid the need for the setInterval()?

Sorry for the lag of response here. In general, the problem is that sendBeacon fires off to Nirvana and we can not test whether it succeeded or not, hence we have to use those flaky test setups, where we wait a certain amount of time and then make sure the redirect succeeded or not. I think it shouldn't make a difference whether to use setTimeOut or setInterval. If there is a better way to test beacon redirects I would really be interested to know that and I would be happy to rewrite the test.

I think increasing the timeout of the test to 30 seconds is probably too long and will just result in long waits ultimately. As things stand right now I am fine with disabling the test on android debug only (which means the test is still executed on a android release, right?)
Flags: needinfo?(ckerschb)
Created attachment 8819937 [details] [diff] [review]
skip on Android Debug only

Thanks :ckerschb - let's disable on Android Debug then. The test will continue to run on Android Release and all other platforms. I've backed out my previous change here too since it was ineffective and was intended for Android Debug.
Attachment #8819937 - Flags: review?(ckerschb)
Comment on attachment 8819937 [details] [diff] [review]
skip on Android Debug only

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

(In reply to Geoff Brown [:gbrown] from comment #30)

> The test will continue to run on Android Release and all other platforms. I've backed out
> my previous change here too since it was ineffective and was intended for
> Android Debug.

Thanks a lot Geoff!
Attachment #8819937 - Flags: review?(ckerschb) → review+

Comment 32

2 years ago
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3647cd5e8459
Skip test_beaconRedirect.html on Android Debug only; r=ckerschb

Comment 33

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3647cd5e8459
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.