Closed Bug 1296845 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Security, defect, P3)

Product:
Core
Component:
DOM: Security
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: aryx, Assigned: gbrown)

References

Details

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

Attachments

(2 files)

increase interval to 10 seconds on android only
997 bytes, patch
francois
: review+
Details | Diff | Splinter Review
skip on Android Debug only
1.39 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
Priority: -- → P3
Assignee: nobody → gbrown
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+
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
https://hg.mozilla.org/mozilla-central/rev/762cd8291634
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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.
(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)
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+
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3647cd5e8459
Skip test_beaconRedirect.html on Android Debug only; r=ckerschb
https://hg.mozilla.org/mozilla-central/rev/3647cd5e8459
Status: REOPENED → RESOLVED
Closed: 8 years ago → 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: