Closed Bug 1559088 Opened 5 years ago Closed 5 years ago

Use of unknown property Ci.nsIRequest.LOAD_ONLY_FROM_CACHE in test_redirect-caching_failure.js

Categories

(Core :: Networking, defect, P3)

Product:
Core
Component:
Networking
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- fixed
firefox71 --- fixed

People

(Reporter: standard8, Assigned: mayhemer)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [necko-triaged])

Attachments

(1 file)

Bug 1559088 - Fix test_redirect-caching_failure.js to again do what it has to, r=kershaw
47 bytes, text/x-phabricator-request
Details | Review

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.

Flags: needinfo?(dd.mozilla)

Honza, can you take a look please?

Flags: needinfo?(dd.mozilla) → needinfo?(honzab.moz)
See Also: → 1559087

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.

Assignee: nobody → honzab.moz
Flags: needinfo?(honzab.moz)
Priority: -- → P3
Whiteboard: [necko-triaged]

(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.

Keywords: regression
Pushed by honzab.moz@firemni.cz:
https://hg.mozilla.org/integration/autoland/rev/a4768bd82d57
Fix test_redirect-caching_failure.js to again do what it has to, r=kershaw

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

Flags: needinfo?(honzab.moz)
Pushed by honzab.moz@firemni.cz:
https://hg.mozilla.org/integration/autoland/rev/9057d052acee
Fix test_redirect-caching_failure.js to again do what it has to, r=kershaw

What the...

I have no idea what's happening here.

Flags: needinfo?(honzab.moz)

(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?

Flags: needinfo?(honzab.moz)

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

Flags: needinfo?(honzab.moz)

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... :-)

Pushed by honzab.moz@firemni.cz:
https://hg.mozilla.org/integration/autoland/rev/cd2c36839bf4
Fix test_redirect-caching_failure.js to again do what it has to, r=kershaw

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

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

I finally made it :D

Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: