1373198 - Disable RCWN for tests which require deterministic cache behavior

Status: RESOLVED FIXED

(Core :: Networking: Cache, enhancement)

Description

[Michal Novotny [:michal]]

When RCWN is enabled the request might be served from the server even if we have valid entry in the cache. This will break some cache related tests. We need to disable RCWN for these tests.

Comment 1

[Michal Novotny [:michal]]

Junior, could you work on this bug? Have a look at failures in this push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d262ca4d1a4a78f7280faeb02531b53b09a8c65

Comment 2

[Junior [inactive]]

(In reply to Michal Novotny (:michal) from comment #1)
> Junior, could you work on this bug? Have a look at failures in this push:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3d262ca4d1a4a78f7280faeb02531b53b09a8c65

Yes, I can work on this.

As you mentioned in bug 1367742 comment 28,
the first step is to disable RCWN for those tests expecting cached content.

Comment 3

[Junior [inactive]]

(In reply to Junior[:junior] from comment #2)
> (In reply to Michal Novotny (:michal) from comment #1)
> > Junior, could you work on this bug? Have a look at failures in this push:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=3d262ca4d1a4a78f7280faeb02531b53b09a8c65
> 
> Yes, I can work on this.
> 
> As you mentioned in bug 1367742 comment 28,
> the first step is to disable RCWN for those tests expecting cached content.

Also mentioned in the title :p

Comment 4

[Michal Novotny [:michal]]

(In reply to Junior[:junior] from comment #2)
> Yes, I can work on this.
> 
> As you mentioned in bug 1367742 comment 28,
> the first step is to disable RCWN for those tests expecting cached content.

Thanks, just make sure that all those tests really need consistent results from the cache and that they are not badly written or failing for other reason.

Comment 5

[Junior [inactive]]

After examine all the cases in comment 1, we might have a test which removes CacheEnrtry::AsyncOpenURI when we rcwn.

Comment 6

[Junior [inactive]]

(In reply to Junior[:junior] from comment #5)
> After examine all the cases in comment 1, we might have a test which removes
> CacheEnrtry::AsyncOpenURI when we rcwn.

I found that we tend to use the network data in the test.
Therefore, it won't be much more failure for this try if I guess correctly.

Comment 7

[Junior [inactive]]

Attachment: Part1-wpt-test - v1

It's intuitive that these tests need deterministic cache behavior.

I can reproduce /cors/304.html, so let's keep watch in the treeherder.

Comment 8

[Junior [inactive]]

Attachment: Part2-xpcshell

This group has some annotation or cache counting code
image/test/unit/test_private_channel.js // count the net hits 
netwerk/test/unit/test_bug596443.js with annotation // Note param: we expect this to come from cache
netwerk/test/unit/test_bug650995.js with annotation // expect cached value
netwerk/test/unit/test_bug770243.js // CL_FROM_CACHE Response must be from the cache
netwerk/test/unit/test_cache_jar.js // check handlers_called, which expecting from the cache/net
netwerk/test/unit/test_bug1279246.js // check request.isFromCache


Same as cache concurrent tests, and I'm wondering we should disable 29a-c too.
netwerk/test/unit/test_cache2-29e-concurrent_read_half-non-206-response.js 
netwerk/test/unit/test_cache2-29d-concurrent_read_half-corrupted-206.js

Obviously we need to disable for this 
netwerk/test/unit/test_cache-control_request.js 

This is tricky, we use |TriggerNetwork| manually rather than let Necko race by itself. Hence we can not let Necko race again.
netwerk/test/unit/test_race_cache_with_network.js


I still have no idea about this
toolkit/mozapps/extensions/test/xpcshell/test_backgroundupdate.js

Comment 9

[Junior [inactive]]

Attachment: Part3-dom-mochitest-1, v1

Hello bz,
bug 1358038 comment 0 has a brief of RCWN.
Here we disable RCWN for those tests which rely on deterministic cache behavior.

Could you review this patch? Thanks.


I also see orange on the following tests, but I can not reproduce. Let's see.
dom/browser-element/mochitest/test_browserElement_oop_ExposableURI.html
dom/tests/mochitest/localstorage/test_localStorageQuotaSessionOnly2.html

Also another tricky one is
docshell/test/chrome/test_bug89419.xul

The steps are
1) we go to page A,
2) and go to about:blank,
3) and back to page A.
The result for 1) and 3) are not the same if we enable RCWN.

We race here, so obviously it doesn't set LOAD_ONLY_FROM_CACHE, 
I do think we should not race here.
I will keep investigate.

Comment 10

[Junior [inactive]]

FWIW, we still have these uncheck mochitests

netwerk/test/mochitests/test_partially_cached_content.html

browser/components/originattributes/test/browser/browser_imageCacheIsolation.js
image/test/mochitest/test_bug1217571.html
 
devtools/client/webconsole/test/browser_webconsole_bug_599725_response_headers.js 
devtools/client/framework/test/browser_toolbox_options_disable_cache-01.js 
devtools/client/framework/test/browser_toolbox_options_disable_cache-02.js
devtools/client/debugger/test/mochitest/browser_dbg_post-page.js
devtools/client/styleeditor/test/browser_styleeditor_fetch-from-cache.js 
devtools/client/netmonitor/test/browser_net_cached-status.js
devtools/client/debugger/test/mochitest/browser_dbg_post-page.js

toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html
toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.
toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html

Comment 11

[Junior [inactive]]

> Also another tricky one is
> docshell/test/chrome/test_bug89419.xul
> 
> The steps are
> 1) we go to page A,
> 2) and go to about:blank,
> 3) and back to page A.
> The result for 1) and 3) are not the same if we enable RCWN.
> 
> We race here, so obviously it doesn't set LOAD_ONLY_FROM_CACHE, 
> I do think we should not race here.
> I will keep investigate.

In my environment, the Loadflags(26d1000) is the same for enabling or disabling RCWN.
That is, no special load flag is set for the "back".

It's pretty weird since:
1) if we disable, the resource is from cache
2) if we enable, the resource is from net, but rcwn is not triggered.

Comment 12

[Junior [inactive]]

Attachment: Part4-necko-mochitest, v1

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8878437 [details] [diff] [review]
Part3-dom-mochitest-1, v1

>+++ b/docshell/test/test_bug669671.html

I don't understand the change in this file.  What will make the test actually run after that yield happens?  I see nothing that would.  Has this been tested on try?

>+++ b/dom/workers/test/browser_bug1047663.js
>+  Preferences.set("network.http.rcwn.enabled", false);

What restores the old state later?

Comment 14

[Junior [inactive]]

Attachment: Part5-devtools-mochitest - v1

Hello Honza,
I set you as the reviewer since you already knew RCWN.
Could you help to review this?
Thanks.

And there's an rcwn bug I found from browser_dbg_post-page.js :
We might accidentally changed the method from POST to GET in devtool form submit.

Comment 15

[Junior [inactive]]

Attachment: Part6-toolkit-mochitest, v1

Hello Shane,

bug 1358038 comment 0 has a brief of RCWN.
RCWN might remove If-Modified-Since/Match header, thus breaking some expectation from cache in this test.

Could you help to review?
Thanks.

Comment 16

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e7d87361bb85f62aea1bc0e01fb2137fe414874

Comment 17

[Junior [inactive]]

Attachment: Part3-dom-mochitest-1, v2

I also disable rcwn in browser_imageCacheIsolation.js since it counts gHits

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #13)
> Comment on attachment 8878437 [details] [diff] [review]
> Part3-dom-mochitest-1, v1
> 
> >+++ b/docshell/test/test_bug669671.html
> 
> I don't understand the change in this file.  What will make the test
> actually run after that yield happens?  I see nothing that would.  Has this
> been tested on try?

We see orange before this patch
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d262ca4d1a4a78f7280faeb02531b53b09a8c65&selectedJob=107051674

and it's green after (comment 16)

> 
> >+++ b/dom/workers/test/browser_bug1047663.js
> >+  Preferences.set("network.http.rcwn.enabled", false);
> 
> What restores the old state later?

Thanks for catching this.

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8878992 [details] [diff] [review]
Part5-devtools-mochitest - v1

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

Looks reasonable to me!

R+

Honza

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Junior[:junior] from comment #14)
> Created attachment 8878992 [details] [diff] [review]
> Part5-devtools-mochitest - v1
> 
> Hello Honza,
> I set you as the reviewer since you already knew RCWN.
> Could you help to review this?
> Thanks.
Done

> And there's an rcwn bug I found from browser_dbg_post-page.js :
> We might accidentally changed the method from POST to GET in devtool form
> submit.
Is it reported?

Honza

Comment 20

[Junior [inactive]]

> > And there's an rcwn bug I found from browser_dbg_post-page.js :
> > We might accidentally changed the method from POST to GET in devtool form
> > submit.
> Is it reported?
> 
> Honza

No, I don't understand enough about the setup in this test.
It looks like that we click something in the devtool and we should send a POST.

Comment 21

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Junior[:junior] from comment #20)
> > > And there's an rcwn bug I found from browser_dbg_post-page.js :
> > > We might accidentally changed the method from POST to GET in devtool form
> > > submit.
> > Is it reported?
> > 
> > Honza
> 
> No, I don't understand enough about the setup in this test.
> It looks like that we click something in the devtool and we should send a
> POST.

@Jason?

Honza

Comment 22

[Jason Laster [:jlast]]

It looks like the test simulates creating a new source via the form. Honestly, if the test is causing an issue it is safe to remove it as we are planning removing the old debugger UI sometime later this year.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8879023 [details] [diff] [review]
Part3-dom-mochitest-1, v2

OK, I sat down and sorted through the behavior of yield*.  It does absolutely nothing if there is nothing to yield, as in this case.  So these lines:

>+  function* disableRcwn() {
>+    SpecialPowers.pushPrefEnv({set: [["network.http.rcwn.enabled", false]]});
>+  }
>+  yield* disableRcwn();

behave exactly the same as this would:

   SpecialPowers.pushPrefEnv({set: [["network.http.rcwn.enabled", false]]});

which is why the test doesn't time out.  On the other hand, that means that this patch is racing the pref set against the rest of the test and hoping that the pref set completes before we get to the caching parts of the test.  This happens to work sometimes, sure.  There's no guarantee it will work.

Please actually wait for the pref set to finish before continuing.

Comment 24

[Junior [inactive]]

Attachment: Part3-dom-mochitest-1, v3

Let's try this old-fashioned way :)

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8879401 [details] [diff] [review]
Part3-dom-mochitest-1, v3

Yes, that's much more like it.  ;)

r=me

Comment 26

[Junior [inactive]]

Notes:
1. It seems a regression for 
dom/browser-element/mochitest/test_browserElement_oop_ExposableURI.html

It still exposed the user name.

FWIW, Bug 1377162 comment 1 elaborate some about the reproducing in local.

2. Still no idea about toolkit/mozapps/extensions/test/xpcshell/test_backgroundupdate.js

3. After offline discussion, docshell/test/chrome/test_bug89419.xul mentioned in comment 9 is not a regression.
   I'll disable the rcwn for this test.

4. For devtools/client/debugger/test/mochitest/browser_dbg_post-page.js
   Will disable by comment 22

5. will disable:
dom/base/test/test_bug482935.html // test cache behaviour
/cors/304.htm // test IMS header
netwerk/test/unit/test_immutable.js // test If-None-Match header

6. the name in patch 1 should s/defalt/default

7. comment 8 should have handled netwerk/test/unit/test_bug1279246.js (facepalm

Comment 27

[Junior [inactive]]

Attachment: pref-on

Hello :Mossop,

I don't have an idea to toolkit/mozapps/extensions/test/xpcshell/test_backgroundupdate.js

The test would fail if we turn on the RCWN.
onDownloadFailed FAIL [onDownloadFailed : 118] false == true

We'd like to see if it's a RCWN bug, or it's no bug there (if so, we just turn off the pref in this test)

STR:
1. apply the patch in this comment
2. run the xpcshell-test

bug 1358038 comment 0 has a brief of RCWN.
RCWN might:
 a) get some resource from net instead of cache
 b) remove If-Modified-Since/If-Match header

Log talks about the HTTP cache.
I'm wondering if it needs something which must be from cache.

What do you think?

Comment 28

[Dave Townsend [:mossop]]

I have no idea what RCWN is but I no longer own this code either.

Comment 29

[Robert Helmer [:rhelmer]]

(In reply to Junior[:junior][ooo 7/1-9] from comment #27)
> Created attachment 8882528 [details]
> pref-on
> 
> Hello :Mossop,
> 
> I don't have an idea to
> toolkit/mozapps/extensions/test/xpcshell/test_backgroundupdate.js
> 
> The test would fail if we turn on the RCWN.
> onDownloadFailed FAIL [onDownloadFailed : 118] false == true
> 
> We'd like to see if it's a RCWN bug, or it's no bug there (if so, we just
> turn off the pref in this test)
> 
> STR:
> 1. apply the patch in this comment
> 2. run the xpcshell-test
> 
> bug 1358038 comment 0 has a brief of RCWN.
> RCWN might:
>  a) get some resource from net instead of cache
>  b) remove If-Modified-Since/If-Match header
> 
> Log talks about the HTTP cache.
> I'm wondering if it needs something which must be from cache.
> 
> What do you think?

Hm. Well I can reproduce the issue, the root cause seems to be that the local test server (httpd.js) is returning 404 for a XPI being used as part of the test, so it's possible that the issue is in httpd.js

I don't see anything in the addons manager background update check code, or this test, that is explicitly depending on the HTTP cache.

Are the log messages you're referring to `Clearing cache because it is disabled`? If so, this is referring to the AddonRepository database in the profile so probably not a useful lead, unfortunately.

Is this the only test in mozilla-central that's failing? That's surprising to me if so... let me know if you are stuck on this and I can reduce to a minimal test case so we can figure it out.

Comment 30

[Junior [inactive]]

> Hm. Well I can reproduce the issue, the root cause seems to be that the
> local test server (httpd.js) is returning 404 for a XPI being used as part
> of the test, so it's possible that the issue is in httpd.js
> 
> I don't see anything in the addons manager background update check code, or
> this test, that is explicitly depending on the HTTP cache.

You are right. No HTTP cache is used in this test case.
The root cause is "addons-background-update-complete" is sent after
|OnStopRequest->OnDownloadFailed| when we enabled RCWN.

And I guess the dependency is important, isn't it?
> 
> Is this the only test in mozilla-central that's failing? That's surprising
> to me if so... let me know if you are stuck on this and I can reduce to a
> minimal test case so we can figure it out.

As you can see, we file this bug for handling this kind of failure.
Most of the cases are owing to HTTP cache/If-Modified-Since,If-Match checking.

This one is not the case.
Do you have any idea why the order of |addons-background-update-complete|
and |OnDownloadFailed| are reversed?

Comment 31

[Michal Novotny [:michal]]

Comment on attachment 8878434 [details] [diff] [review]
Part2-xpcshell

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

(In reply to Junior[:junior] from comment #8)
> Same as cache concurrent tests, and I'm wondering we should disable 29a-c
> too.

Yes.

> I still have no idea about this
> toolkit/mozapps/extensions/test/xpcshell/test_backgroundupdate.js

I don't see a reason, why this test would need racing disabled.

Comment 32

[Junior [inactive]]

Attachment: Part5-devtools-mochitest, v2

Update metadata, carry r+

Comment 33

[Junior [inactive]]

Attachment: Part6-toolkit-mochitest, v2

Update metadata, carry r+

Comment 34

[Junior [inactive]]

Attachment: Part7-wpt-test-2, v1

Comment 35

[Junior [inactive]]

Attachment: Part9-xpcshell-2, v1

Comment 36

[Junior [inactive]]

Attachment: Part10-devtools-mochitest-2

disable rcwn in this test by comment 22

Comment 37

[Junior [inactive]]

Attachment: Part8-dom-mochitest-2, v1

Hello smaug,
bug 1358038 comment 0 has a brief of RCWN.
Here we disable RCWN for those tests which rely on deterministic cache behavior.

The steps in test_bug89419.xul is
1) we go to page A,
2) and go to about:blank,
3) and back to page A.
The result for 1) and 3) are not the same, since 3) could be from network if we enable RCWN.

and test_bug482935.html tests some cache behavior.

Could you review this patch? Thanks.

Comment 38

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59280807f21a85234ce6f22e3ba684789e9b2cd1

Comment 39

[Michal Novotny [:michal]]

Comment on attachment 8885131 [details] [diff] [review]
Part9-xpcshell-2, v1

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

::: netwerk/test/unit/test_bug1279246.js
@@ +69,5 @@
>    prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
>    cePref = prefs.getCharPref("network.http.accept-encoding");
>    prefs.setCharPref("network.http.accept-encoding", "gzip, deflate, br");
>  
> +  // Diable rcwn to make cache behavior deterministic.

Disable

::: netwerk/test/unit/test_immutable.js
@@ +21,5 @@
>  
>    prefs.setBoolPref("network.http.spdy.enabled", true);
>    prefs.setBoolPref("network.http.spdy.enabled.http2", true);
>    prefs.setCharPref("network.dns.localDomains", "foo.example.com, bar.example.com");
> +  // Diable rcwn to make cache behavior deterministic.

Disable

Comment 40

[Junior [inactive]]

Attachment: Part3-dom-mochitest-1, v4

Fix nit, carry r+

Comment 41

[Junior [inactive]]

Attachment: Part5-devtools-mochitest, v3

Fix nit, carry r+

Comment 42

[Junior [inactive]]

Attachment: Part9-xpcshell-2, v2

Fix nit, carry r+

Comment 43

[Junior [inactive]]

Attachment: Part8-dom-mochitest-2, v2

Fix a nit, sorry for spam

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8885168 [details] [diff] [review]
Part8-dom-mochitest-2, v2

setBoolPref without ever clearing the value doesn't look right.
If possible, use pushPrefEnv also in bug89419_window.xul

Comment 45

[Junior [inactive]]

Attachment: Part9-xpcshell-2, v3

Attach the right file

Comment 46

[Junior [inactive]]

Attachment: Part8-dom-mochitest-2, v3

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8885216 [details] [diff] [review]
Part8-dom-mochitest-2, v3

># HG changeset patch
># User Junior Hsu <[e-mail]>
># Parent  78ae03a9015d917f359d2c2018dfc9ab40208499
>Bug 1373198 - Part 8: disable rcwn for dom mochitest tests which require deterministic cache behavior, r=smaug
>
>diff --git a/docshell/test/chrome/bug89419_window.xul b/docshell/test/chrome/bug89419_window.xul
>--- a/docshell/test/chrome/bug89419_window.xul
>+++ b/docshell/test/chrome/bug89419_window.xul
>@@ -14,16 +14,17 @@
>   <script type="text/javascript"
>           src="chrome://mochikit/content/tests/SimpleTest/SpecialPowersObserverAPI.js"/>
>   <script type="text/javascript"
>           src="chrome://mochikit/content/tests/SimpleTest/specialpowers.js"/>
>   <script type="application/javascript" src="docshell_helpers.js" />
>   <script type="text/javascript" src="chrome://mochikit/content/tests/SimpleTest/WindowSnapshot.js"></script>
> 
>   <script type="application/javascript"><![CDATA[
>+
unneeded change

Comment 48

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8885132 [details] [diff] [review]
Part10-devtools-mochitest-2

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

Thanks for the patch!

Please see my inline comment.

Honza

::: devtools/client/debugger/test/mochitest/browser_dbg_post-page.js
@@ +18,5 @@
> +  Preferences.set("network.http.rcwn.enabled", false);
> +  registerCleanupFunction(() => {
> +    Preferences.set("network.http.rcwn.enabled", rcwnEnabled);
> +  });
> +

Please use `Services.prefs.setBoolPref` and `Services.prefs.clearUserPref` so it's more consistent with the rest of the code base.


Services.prefs.setBoolPref("network.http.rcwn.enabled", false);
registerCleanupFunction(() => {
  Services.prefs.clearUserPref("network.http.rcwn.enabled");
});

Comment 49

[Junior [inactive]]

Attachment: Part8-dom-mochitest-2, v4

Fix nit, carry r+

Comment 50

[Junior [inactive]]

Attachment: Part10-devtools-mochitest-2, v2

Do you think we should also change Part5?

Comment 51

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8885552 [details] [diff] [review]
Part10-devtools-mochitest-2, v2

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

Thanks!

R+ assuming Try is green.

Honza

Comment 52

[Junior [inactive]]

Do you think we should also change the pref-setting style in Part5 (Comment 41)?

Comment 53

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Junior[:junior] from comment #52)
> Do you think we should also change the pref-setting style in Part5 (Comment
> 41)?
So, I asked around and there is actually yet another API for setting prefs:
`pushPref` (based on  SpecialPowers.pushPrefEnv)

http://searchfox.org/mozilla-central/rev/31311070d9860b24fe4a7a36976c14b328c16208/devtools/client/framework/test/shared-head.js#554

So, the best way is actually using this API. Here is an example:

add_task(function* () {
  yield pushPref("network.http.rcwn.enabled", false);

  // ...
});

The good news is that this API will make sure the pref is restored to its original value automatically.

It should be used even in the: Part10-devtools-mochitest-2, v2 patch

Sorry I didn't mentioned that before!

Thanks,
Honza

Comment 54

[Junior [inactive]]

Attachment: Part5-devtools-mochitest, v4

Comment 55

[Junior [inactive]]

Attachment: Part10-devtools-mochitest-2, v3

Comment 56

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6374b04d5daca3b254e3fdbc9ec4abd75d9e1e0f

try without the pref-on patch

Comment 57

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8885623 [details] [diff] [review]
Part5-devtools-mochitest, v4

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

Looks great now, thanks!

Honza

Comment 58

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8885624 [details] [diff] [review]
Part10-devtools-mochitest-2, v3

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

Thanks for the patch!

Honza

Comment 59

[Junior [inactive]]

Please check in from Part 1 to Part 10, thanks!
Note: Patch "pref-on" is a test patch, not for landing.

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55471601da44
Part 1: disable rcwn for wpt tests which require deterministic cache behavior, r=michal
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a52987915e7
Part2: disable rcwn for xpcshell tests which require deterministic cache behavior, r=michal
https://hg.mozilla.org/integration/mozilla-inbound/rev/3679429ca4d6
Part 3-1: disable rcwn for dom mochitest tests which require deterministic cache behavior, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/09b311b83042
Part 4: disable rcwn for necko mochitest tests which require deterministic cache behavior, r=michal
https://hg.mozilla.org/integration/mozilla-inbound/rev/50def55507e6
Part 5: disable rcwn for devtools mochitest tests which require deterministic cache behavior, r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/198cc96cc9b8
Part 6: disable rcwn for toolkit mochitest tests which require deterministic cache behavior, r=mixedpuppy
https://hg.mozilla.org/integration/mozilla-inbound/rev/212958380e64
Part 7: disable rcwn for wpt tests which require deterministic cache behavior, r=michal
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc83f0af29ff
Part 8: disable rcwn for dom mochitest tests which require deterministic cache behavior, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/933aeb87fcbb
Part 9: disable rcwn for xpcshell tests which require deterministic cache behavior, r=michal
https://hg.mozilla.org/integration/mozilla-inbound/rev/31973778f0ed
Part 10: disable rcwn for devtools mochitest tests which require deterministic cache behavior, r=Honza

Comment 61

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/55471601da44
https://hg.mozilla.org/mozilla-central/rev/1a52987915e7
https://hg.mozilla.org/mozilla-central/rev/3679429ca4d6
https://hg.mozilla.org/mozilla-central/rev/09b311b83042
https://hg.mozilla.org/mozilla-central/rev/50def55507e6
https://hg.mozilla.org/mozilla-central/rev/198cc96cc9b8
https://hg.mozilla.org/mozilla-central/rev/212958380e64
https://hg.mozilla.org/mozilla-central/rev/bc83f0af29ff
https://hg.mozilla.org/mozilla-central/rev/933aeb87fcbb
https://hg.mozilla.org/mozilla-central/rev/31973778f0ed

Comment 62

[Junior [inactive]]

Hello Henrik,
We found that some tier-2 tests could be broken when we enable rcwn.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59280807f21a85234ce6f22e3ba684789e9b2cd1&selectedJob=113270005

ni to ensure if you're still interested in these tests

Comment 63

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The tier-2 Firefox-UI tests are important and are only tier-2 because they have to access remote host. So if rcwn has to be disabled for them please do so. I'm happy to review. But please do not break them. Thanks.

Comment 64

[Junior [inactive]]

Attachment: Part11-firefox-ui-test, v1

I found that |mach firefox-ui-functional| leads a crash right after |Starting fixture servers| in my local macOS.
Did I miss any procedures?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c8ef65faf9cc28c51f65c8120e3874bb01456b0

Comment 65

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Yes, this is because of bug 1379906. You can use '--pref security.sandbox.content.level:2' to prevent it for now.

Comment 66

[Junior [inactive]]

(In reply to Henrik Skupin (:whimboo) [away 07/21 - 07/31] from comment #65)
> Yes, this is because of bug 1379906. You can use '--pref
> security.sandbox.content.level:2' to prevent it for now.

Thanks for this information.
For the patch, I'd like to revert the change of test_safe_browsing_initial_download
since Bug 1381745.

Comment 67

[Junior [inactive]]

Attachment: Part11-firefox-ui-test, v2

Comment 68

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8887877 [details] [diff] [review]
Part11-firefox-ui-test, v2

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

r=me with comments addressed. Thanks.

::: testing/firefox-ui/tests/functional/security/test_submit_unencrypted_info_warning.py
@@ +17,5 @@
>          self.url = 'https://ssl-dv.mozqa.com/data/firefox/security/unencryptedsearch.html'
>          self.test_string = 'mozilla'
>  
>          self.marionette.set_pref('security.warn_submit_insecure', True)
> +        # Disable rcwn to make cache behavior deterministic

Mind sorting the prefs alphabetically? Same for tearDown.

::: testing/firefox-ui/tests/functional/security/test_untrusted_connection_error_page.py
@@ +15,5 @@
>      def setUp(self):
>          super(TestUntrustedConnectionErrorPage, self).setUp()
>  
>          self.url = 'https://ssl-selfsigned.mozqa.com'
> +        # Disable rcwn to make cache behavior deterministic

nit: new line before please.

Comment 69

[Junior [inactive]]

Attachment: Part11-firefox-ui-test, v3

Thanks!

Comment 70

[Junior [inactive]]

pref-on: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fd9da4b3ef3d2c6fb5744aa3604c5cfe7ee0597

pref-off:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a181e3a887ea3e29b8bfc5eb609ed06e14328fed

Comment 71

[Junior [inactive]]

Please help to land Part11. Thanks!

Comment 72

[Junior [inactive]]

For the rest of oranges,
they are either not reproducible in local or unable to find out why the race matters.

However, disabling them would solve the possible intermittent in the future.
I'd like to give them a shot and file a follow-up.

Comment 73

[Junior [inactive]]

Attachment: rest - WIP v1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b704add600d58717233c243cbb97e8b7f7fd3d38

Reproducible in local:
toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html
browser/base/content/test/general/browser_save_video.js
dom/base/test/test_urgent_start.html
toolkit/mozapps/extensions/test/xpcshell/test_backgroundupdate.js

local crash:
wpt preload/link-header-preload-delay-onload.html 

Not reproducible:
toolkit/modules/tests/browser/browser_WebRequest_filtering.js
devtools/client/webconsole/net/test/mochitest/browser_net_headers.js
dom/presentation/tests/mochitest/test_presentation_reconnect.html

Comment 74

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66a54d08470e
Part 11: Disable rcwn for firefox-ui-test tests which require deterministic cache behavior. r=whimboo

Comment 75

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/66a54d08470e

Comment 76

[Junior [inactive]]

Attachment: Part12: rest - v2

Comment 77

[Junior [inactive]]

pref-on but not always race:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a1b812e35c410a411c8adfc6d2a719887c3cc82

Comment 78

[Junior [inactive]]

pref-on with Part 12, but not always race:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbab5aa15b087251ee97987fcbdd95b0d8ff1e10

Comment 79

[Michal Novotny [:michal]]

Comment on attachment 8889315 [details] [diff] [review]
Part12: rest - v2

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

There is no explanation why these tests need RCWN disabled.

Comment 80

[Junior [inactive]]

Attachment: Disable the rest of tests in doubt. Patch for reference -- PLEASE DO NOT LAND

still oranges appear and need some handles.
Upload WIP here. 

Note that Ref-test is bad in the tree, I think we can ignore them here.

Comment 81

[Junior [inactive]]

1. devtools/client/webconsole/net/test/mochitest/browser_net_headers.js
"This test generates XHR requests in the page, expands networks details
in the Console panel and checks that HTTP headers are there."

The comment indicates RCWN may broke the web console.

2. wpt preload also rely on cache behavior.
Would like to disable all the folder.

3. File bug 1384478, bug 1384493, bug 1384506, and bug 1384510 for non-trivial cases.

Comment 82

[Junior [inactive]]

Move the discussion 
toolkit/mozapps/extensions/test/xpcshell/test_backgroundupdate.js
to Bug 1384510

Comment 83

[Junior [inactive]]

pref-on with 0 delay: https://treeherder.mozilla.org/#/jobs?repo=try&revision=33b834a19c73dcc9d6eac7d77763c79be0eccffb

Comment 84

[Junior [inactive]]

Attachment: Part12-devtools-mochitest-3

Please see comment 81.

Comment 85

[Junior [inactive]]

Attachment: Part13-wpt-test-3

Disable wpt preload since it heavily relies on cache behavior.

Comment 86

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8890306 [details] [diff] [review]
Part12-devtools-mochitest-3

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

LGTM. thanks!

Comment 87

[Junior [inactive]]

When we enable rcwn, some tests [1] in treeherder hits an NOT_REACH [2]

How about SimpleTest.expectAssertions(0,n) and file a follow-up bug, Valentin?

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=116905039&repo=try&lineNumber=9593
[2] https://hg.mozilla.org/try/file/4a1b812e35c410a411c8adfc6d2a719887c3cc82/netwerk/protocol/http/nsHttpChannel.cpp#l7135

Comment 88

[Junior [inactive]]

Hello Shane,
Still have some failures of web request in treeherder when we enable rcwn.

For example:
toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a1b812e35c410a411c8adfc6d2a719887c3cc82&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=116905096

toolkit/modules/tests/browser/browser_WebRequest_filtering.js
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a1b812e35c410a411c8adfc6d2a719887c3cc82&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=116937168

RCWN might:
 a) get some resource from net instead of cache
 b) remove If-Modified-Since/If-Match header

And it seems that some status relies on the cache status.
I'm not sure if these status check matters a lot.
Could we just disable rcwn in these test?

Comment 89

[Junior [inactive]]

(In reply to Junior[:junior] from comment #88)
> Hello Shane,
> Still have some failures of web request in treeherder when we enable rcwn.
> 
> For example:
> toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4a1b812e35c410a411c8adfc6d2a719887c3cc82&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-
> classifiedState=unclassified&selectedJob=116905096
> 
> toolkit/modules/tests/browser/browser_WebRequest_filtering.js
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4a1b812e35c410a411c8adfc6d2a719887c3cc82&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-
> classifiedState=unclassified&selectedJob=116937168
> 
> RCWN might:
>  a) get some resource from net instead of cache
>  b) remove If-Modified-Since/If-Match header
> 
> And it seems that some status relies on the cache status.
> I'm not sure if these status check matters a lot.
> Could we just disable rcwn in these test?

Also here
https://treeherder.mozilla.org/logviewer.html#?job_id=116905030&repo=try&lineNumber=15824

Comment 90

[Junior [inactive]]

(In reply to Junior[:junior] from comment #87)
> When we enable rcwn, some tests [1] in treeherder hits an NOT_REACH [2]
> 
> How about SimpleTest.expectAssertions(0,n) and file a follow-up bug,
> Valentin?
> 
> [1]
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=116905039&repo=try&lineNumber=9593
> [2]
> https://hg.mozilla.org/try/file/4a1b812e35c410a411c8adfc6d2a719887c3cc82/
> netwerk/protocol/http/nsHttpChannel.cpp#l7135

Also here
toolkit/components/url-classifier/tests/mochitest/test_trackingprotection_bug1312515.html 
https://treeherder.mozilla.org/logviewer.html#?job_id=116905030&repo=try&lineNumber=15824

Comment 91

[Junior [inactive]]

(In reply to Junior[:junior] from comment #90)
> (In reply to Junior[:junior] from comment #87)
> > When we enable rcwn, some tests [1] in treeherder hits an NOT_REACH [2]
> > 
> > How about SimpleTest.expectAssertions(0,n) and file a follow-up bug,
> > Valentin?
> > 
> > [1]
> > https://treeherder.mozilla.org/logviewer.
> > html#?job_id=116905039&repo=try&lineNumber=9593
> > [2]
> > https://hg.mozilla.org/try/file/4a1b812e35c410a411c8adfc6d2a719887c3cc82/
> > netwerk/protocol/http/nsHttpChannel.cpp#l7135
> 
> Also here
> toolkit/components/url-classifier/tests/mochitest/
> test_trackingprotection_bug1312515.html 
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=116905030&repo=try&lineNumber=15824

and here
dom/xul/test/test_bug1290965.xul
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a1b812e35c410a411c8adfc6d2a719887c3cc82&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=116905092

Comment 92

[Junior [inactive]]

Please help to land Part12 and Part13. Thanks!

Comment 93

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79b945191dea
Part 12: Disable RCWN for the devtools mochitest. r=rickychien
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a388409e4a9
Part 13: Disable rcwn for wpt tests which require deterministic cache behavior. r=michal

Comment 94

[Shane Caraveo (:mixedpuppy)]

(In reply to Junior[:junior] from comment #88)
> Hello Shane,
> Still have some failures of web request in treeherder when we enable rcwn.

> And it seems that some status relies on the cache status.
> I'm not sure if these status check matters a lot.
> Could we just disable rcwn in these test?

I was about to say disable it, but then I see it's not just one, but many test failures.  Some seem due to prior failures, etc.  So I think that if it is only those two files, then lets disable, but if it turns out to be more tests, I want a better understanding why rcwn is causing failures in one of the core addon APIs.

Comment 95

[Junior [inactive]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #94)
> (In reply to Junior[:junior] from comment #88)
> > Hello Shane,
> > Still have some failures of web request in treeherder when we enable rcwn.
> 
> > And it seems that some status relies on the cache status.
> > I'm not sure if these status check matters a lot.
> > Could we just disable rcwn in these test?
> 
> I was about to say disable it, but then I see it's not just one, but many
> test failures.  Some seem due to prior failures, etc.  So I think that if it
> is only those two files, then lets disable, but if it turns out to be more
> tests, I want a better understanding why rcwn is causing failures in one of
> the core addon APIs.

After disable rcwn to the mentioned two files (and the patch in Comment 15),
treeherder shows it's green for web request per Comment 77 and Comment 78.

TBH I have no idea now that why rcwn leads the failures.
Hence I would leave some annotation in the patch.
And file a follow-up bug if needed.

Thanks!

Comment 96

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/79b945191dea
https://hg.mozilla.org/mozilla-central/rev/9a388409e4a9