Now that netwerk is ESLintable, I have a tool which lets us see if there's issues with use of unknown properties. I just discovered this one:

https://searchfox.org/mozilla-central/rev/227f5329f75bd8b16c6b146a7414598a420260cb/netwerk/test/unit/test_redirect-caching_failure.js#34

Bug 761932 changed that line from

chan.loadFlags |= Ci.nsIRequest.LOAD_FROM_CACHE;

to

chan.loadFlags |= Ci.nsIRequest.LOAD_ONLY_FROM_CACHE;

https://hg.mozilla.org/mozilla-central/rev/9db979e60a9e

However, LOAD_ONLY_FROM_CACHE isn't on nsIRequest, it is on nsICachingChannel

If I change the test to use Ci.nsICachingChannel.LOAD_ONLY_FROM_CACHE or Ci.nsIRequest.LOAD_FROM_CACHE | Ci.nsICachingChannel.LOAD_ONLY_FROM_CACHE then the test fails.

Reverting the test to Ci.nsIRequest.LOAD_FROM_CACHE, then it passes.

So it looks like there was either something wrong with the original test change, or there is something broken that the test missed.

Dragana, could you take a look please, as I don't think Jason/Patrick are around now.

Honza, can you take a look please?

How is it even possible we don't fail the test at runtime on some kind of "unknown property" when this is actually an xpcom interface we dereference here?

I'll look at this.

(In reply to Honza Bambas (:mayhemer) from comment #2)

How is it even possible we don't fail the test at runtime on some kind of "unknown property" when this is actually an xpcom interface we dereference here?

Because javascript doesn't care if you access a property and it doesn't exist, it just returns undefined (which will get coerced to 0 when then passed as an int to xpcom APIs, or used with operators that take numbers). I'm not aware of a way to add runtime checks for this, though we could talk to people who know xpcom/xpidl better and they might have ideas.

(In reply to :Gijs (he/him) from comment #3)

(In reply to Honza Bambas (:mayhemer) from comment #2)

How is it even possible we don't fail the test at runtime on some kind of "unknown property" when this is actually an xpcom interface we dereference here?

Because javascript doesn't care if you access a property and it doesn't exist, it just returns undefined (which will get coerced to 0 when then passed as an int to xpcom APIs, or used with operators that take numbers).

Yep, that is known to me. I only somehow thought that we would not do that with xpcom interfaces.

I'm not aware of a way to add runtime checks for this, though we could talk to people who know xpcom/xpidl better and they might have ideas.

Probably not needed when we have the linter now! Note that "use strict;" would not work for this either.

Thanks.

Backed out changeset a4768bd82d57 (Bug 1559088) for test_redirect-caching_failure_wrap.js failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=a4768bd82d57d0560d9f55750d0dc61822d4138d&tochange=93ca7904bb6869892abf2bab9922e4507892972e&selectedJob=264097330

Backout link: Backed out changeset a4768bd82d57 (Bug 1559088) for test_redirect-caching_failure_wrap.js failures CLOSED TREE

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=264097330&repo=autoland&lineNumber=2465

[task 2019-08-29T17:51:20.693Z] 17:51:20 INFO - TEST-PASS | services/sync/tests/unit/test_clients_engine.js | took 16111ms
[task 2019-08-29T17:51:20.695Z] 17:51:20 INFO - Retrying tests that failed when run in parallel.
[task 2019-08-29T17:51:20.703Z] 17:51:20 INFO - TEST-START | netwerk/test/unit_ipc/test_redirect-caching_failure_wrap.js
[task 2019-08-29T17:51:21.346Z] 17:51:21 WARNING - TEST-UNEXPECTED-FAIL | netwerk/test/unit_ipc/test_redirect-caching_failure_wrap.js | xpcshell return code: 0
[task 2019-08-29T17:51:21.346Z] 17:51:21 INFO - TEST-INFO took 644ms

Backed out changeset 9057d052acee (bug 1559088) for causing xpcshell failures on test_redirect-caching_failure.js CLOSED TREE

Backout revision https://hg.mozilla.org/integration/autoland/rev/2a6a80d2ad09e4cfaeeddbad7c974a6f22917e2f

Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=264296173&resultStatus=testfailed%2Cbusted%2Cexception&revision=9057d052aceef4771b6717a4631f30cd887e3159&searchStr=tv

Failure log
https://treeherder.mozilla.org/logviewer.html#?job_id=264296173&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=264295186&repo=autoland

Honza can you please take a look?

What the...

I have no idea what's happening here.

(In reply to Honza Bambas (:mayhemer) from comment #11)

I have no idea what's happening here.

This is only on android, by the looks of it. Potentially helpful bits of log:

[task 2019-08-30T18:58:50.441Z] 18:58:50     INFO -  netwerk/test/unit/test_redirect-caching_failure.js | [4831, Main Thread] WARNING: No Android crash handler set: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 1549
[task 2019-08-30T18:58:50.442Z] 18:58:50     INFO -  netwerk/test/unit/test_redirect-caching_failure.js | [4831, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2632
[task 2019-08-30T18:58:50.442Z] 18:58:50     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2019-08-30T18:58:50.442Z] 18:58:50     INFO -  netwerk/test/unit/test_redirect-caching_failure.js | [4831, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, kKnownEsrVersion) failed with result 0x80004002: file /builds/worker/workspace/build/src/toolkit/components/resistfingerprinting/nsRFPService.cpp, line 662
[task 2019-08-30T18:58:50.442Z] 18:58:50     INFO -  netwerk/test/unit/test_redirect-caching_failure.js | [4831, Main Thread] WARNING: NSS will be initialized without a profile directory. Some things may not work as expected.: file /builds/worker/workspace/build/src/security/manager/ssl/nsNSSComponent.cpp, line 1336
[task 2019-08-30T18:58:50.443Z] 18:58:50     INFO -  (xpcshell/head.js) | test pending (2)
[task 2019-08-30T18:58:50.443Z] 18:58:50     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2019-08-30T18:58:50.443Z] 18:58:50     INFO -  running event loop
[task 2019-08-30T18:58:50.443Z] 18:58:50     INFO -  netwerk/test/unit/test_redirect-caching_failure.js | [4831, Unnamed thread 7597d3f22230] WARNING: Forcing memory-only entry since CacheFileIOManager doesn't have mCacheDirectory.: file /builds/worker/workspace/build/src/netwerk/cache2/CacheFile.cpp, line 517
[task 2019-08-30T18:58:50.443Z] 18:58:50     INFO -  netwerk/test/unit/test_redirect-caching_failure.js | [4831, Unnamed thread 7597d3f22230] WARNING: Forcing memory-only entry since CacheFileIOManager doesn't have mCacheDirectory.: file /builds/worker/workspace/build/src/netwerk/cache2/CacheFile.cpp, line 517
[task 2019-08-30T18:58:50.443Z] 18:58:50     INFO -  TEST-PASS | netwerk/test/unit/test_redirect-caching_failure.js | firstTimeThrough - [firstTimeThrough : 45] 2152398878 == 2152398878
[task 2019-08-30T18:58:50.443Z] 18:58:50     INFO -  TEST-PASS | netwerk/test/unit/test_redirect-caching_failure.js | firstTimeThrough - [firstTimeThrough : 46] 1 == 1
[task 2019-08-30T18:58:50.443Z] 18:58:50     INFO -  netwerk/test/unit/test_redirect-caching_failure.js | [4831, Main Thread] WARNING: port blocked: file /builds/worker/workspace/build/src/netwerk/base/nsNetUtil.cpp, line 1041
[task 2019-08-30T18:58:50.443Z] 18:58:50     INFO -  netwerk/test/unit/test_redirect-caching_failure.js | [4831, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B0013: file /builds/worker/workspace/build/src/netwerk/protocol/http/nsHttpChannel.cpp, line 5995

Does the cache work differently on android? Perhaps we can land this and disable the test on android and file a follow-up to get android sorted?

Thanks, but this all seems normal. The test can easily work with just a memory cache, as it does on other platforms. Note that originally the test was caching as well and worked well. The port blocking thing is on purpose (I needed some way to control an early load failure to happen)

Disabling on android can be the way here, yes. The test on its own is actually important to keep running. The async redirect logic is sensitive.

The weird thing is that according [1] the test has finished. Looks like some shutdown logic (promise?) is not fired or handled correctly.

[1] https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=264303650&repo=autoland&lineNumber=2172

And note that the first version of the patch [1] was causing the same failure on android-debug. it's nearly the same as before the modification, except the Services.prefs.setCharPref("network.security.ports.banned", "65400"); bit. I'll try to push with a different port number...

[1] https://hg.mozilla.org/integration/autoland/rev/a4768bd82d57d0560d9f55750d0dc61822d4138d

It's TEST-UNEXPECTED-PASS !!! The test was disabled because of bug 675039, but slightly changing it makes it work again...

(In reply to Honza Bambas (:mayhemer) from comment #15)

It's TEST-UNEXPECTED-PASS !!! The test was disabled because of bug 675039, but slightly changing it makes it work again...

D'oh! Glad it works... :-)

https://treeherder.mozilla.org/#/jobs?repo=try&collapsedPushes=511147%2C509985%2C511151&revision=16fce9a7ec368ca01c82c1b7a45d49b2d4a6ec1e

https://hg.mozilla.org/mozilla-central/rev/cd2c36839bf4

I finally made it :D

https://hg.mozilla.org/releases/mozilla-beta/rev/9399706f30aa