Closed Bug 1373198 Opened 2 years ago Closed 2 years ago

Disable RCWN for tests which require deterministic cache behavior

Categories

(Core :: Networking: Cache, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: michal, Assigned: junior)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-active])

Attachments

(15 files, 18 obsolete files)

1.79 KB, patch
michal
: review+
Details | Diff | Splinter Review
9.13 KB, patch
michal
: review+
Details | Diff | Splinter Review
1.41 KB, patch
michal
: review+
Details | Diff | Splinter Review
2.54 KB, text/plain
Details
1.25 KB, patch
junior
: review+
Details | Diff | Splinter Review
791 bytes, patch
michal
: review+
Details | Diff | Splinter Review
4.15 KB, patch
junior
: review+
Details | Diff | Splinter Review
4.82 KB, patch
junior
: review+
Details | Diff | Splinter Review
1.86 KB, patch
junior
: review+
Details | Diff | Splinter Review
4.85 KB, patch
Honza
: review+
Details | Diff | Splinter Review
1.13 KB, patch
Honza
: review+
Details | Diff | Splinter Review
4.43 KB, patch
junior
: review+
Details | Diff | Splinter Review
11.83 KB, patch
Details | Diff | Splinter Review
1.06 KB, patch
rickychien
: review+
Details | Diff | Splinter Review
466 bytes, patch
michal
: review+
Details | Diff | Splinter Review
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.
Junior, could you work on this bug? Have a look at failures in this push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d262ca4d1a4a78f7280faeb02531b53b09a8c65
Flags: needinfo?(juhsu)
(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.
Assignee: nobody → juhsu
Flags: needinfo?(juhsu)
Whiteboard: [necko-active]
(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
(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.
After examine all the cases in comment 1, we might have a test which removes CacheEnrtry::AsyncOpenURI when we rcwn.
(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.
It's intuitive that these tests need deterministic cache behavior.

I can reproduce /cors/304.html, so let's keep watch in the treeherder.
Attachment #8878431 - Flags: review?(michal.novotny)
Attached patch Part2-xpcshellSplinter Review
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
Attachment #8878434 - Flags: review?(michal.novotny)
Attached patch Part3-dom-mochitest-1, v1 (obsolete) — Splinter Review
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.
Attachment #8878437 - Flags: review?(bzbarsky)
> 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.
Attachment #8878570 - Flags: review?(michal.novotny)
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?
Attachment #8878437 - Flags: review?(bzbarsky) → review-
Attached patch Part5-devtools-mochitest - v1 (obsolete) — Splinter Review
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.
Attachment #8878992 - Flags: review?(odvarko)
Attached file Part6-toolkit-mochitest, v1 (obsolete) —
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.
Attachment #8878996 - Flags: review?(mixedpuppy)
Attached patch Part3-dom-mochitest-1, v2 (obsolete) — Splinter Review
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.
Attachment #8878437 - Attachment is obsolete: true
Attachment #8879023 - Flags: review?(bzbarsky)
Comment on attachment 8878992 [details] [diff] [review]
Part5-devtools-mochitest - v1

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

Looks reasonable to me!

R+

Honza
Attachment #8878992 - Flags: review?(odvarko) → review+
(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
> > 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.
(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
Flags: needinfo?(jlaster)
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.
Flags: needinfo?(jlaster)
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.
Attachment #8879023 - Flags: review?(bzbarsky) → review-
Attached patch Part3-dom-mochitest-1, v3 (obsolete) — Splinter Review
Let's try this old-fashioned way :)
Attachment #8879023 - Attachment is obsolete: true
Attachment #8879401 - Flags: review?(bzbarsky)
Comment on attachment 8879401 [details] [diff] [review]
Part3-dom-mochitest-1, v3

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

r=me
Attachment #8879401 - Flags: review?(bzbarsky) → review+
Attachment #8878431 - Flags: review?(michal.novotny) → review+
Attachment #8878570 - Flags: review?(michal.novotny) → review+
Attachment #8878996 - Flags: review?(mixedpuppy) → review+
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
Attached file 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?
Flags: needinfo?(dtownsend)
I have no idea what RCWN is but I no longer own this code either.
Flags: needinfo?(dtownsend) → needinfo?(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.
Flags: needinfo?(rhelmer) → needinfo?(juhsu)
> 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?
Flags: needinfo?(juhsu) → needinfo?(rhelmer)
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.
Attachment #8878434 - Flags: review?(michal.novotny) → review+
Attached patch Part5-devtools-mochitest, v2 (obsolete) — Splinter Review
Update metadata, carry r+
Attachment #8878992 - Attachment is obsolete: true
Attachment #8885126 - Flags: review+
Update metadata, carry r+
Attachment #8878996 - Attachment is obsolete: true
Attachment #8885127 - Flags: review+
Attachment #8885128 - Flags: review?(michal.novotny)
Attached patch Part9-xpcshell-2, v1 (obsolete) — Splinter Review
Attachment #8885131 - Flags: review?(michal.novotny)
Attached patch Part10-devtools-mochitest-2 (obsolete) — Splinter Review
disable rcwn in this test by comment 22
Attachment #8885132 - Flags: review?(odvarko)
Attached patch Part8-dom-mochitest-2, v1 (obsolete) — Splinter Review
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.
Attachment #8885141 - Flags: review?(bugs)
Attachment #8885141 - Attachment is patch: true
See Also: → 1379899
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
Attachment #8885131 - Flags: review?(michal.novotny) → review+
Attachment #8885128 - Flags: review?(michal.novotny) → review+
Fix nit, carry r+
Attachment #8879401 - Attachment is obsolete: true
Attachment #8885158 - Flags: review+
Attached patch Part5-devtools-mochitest, v3 (obsolete) — Splinter Review
Fix nit, carry r+
Attachment #8885126 - Attachment is obsolete: true
Attachment #8885159 - Flags: review+
Attached patch Part9-xpcshell-2, v2 (obsolete) — Splinter Review
Fix nit, carry r+
Attachment #8885131 - Attachment is obsolete: true
Attachment #8885164 - Flags: review+
Attached patch Part8-dom-mochitest-2, v2 (obsolete) — Splinter Review
Fix a nit, sorry for spam
Attachment #8885141 - Attachment is obsolete: true
Attachment #8885141 - Flags: review?(bugs)
Attachment #8885168 - Flags: review?(bugs)
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
Attachment #8885168 - Flags: review?(bugs) → review-
Attach the right file
Attachment #8885164 - Attachment is obsolete: true
Attachment #8885203 - Flags: review+
Attached patch Part8-dom-mochitest-2, v3 (obsolete) — Splinter Review
Attachment #8885168 - Attachment is obsolete: true
Attachment #8885216 - Flags: review?(bugs)
Comment on attachment 8885216 [details] [diff] [review]
Part8-dom-mochitest-2, v3

># HG changeset patch
># User Junior Hsu <juhsu@mozilla.com>
># 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
Attachment #8885216 - Flags: review?(bugs) → review+
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");
});
Attachment #8885132 - Flags: review?(odvarko)
Fix nit, carry r+
Attachment #8885216 - Attachment is obsolete: true
Attachment #8885551 - Flags: review+
Attached patch Part10-devtools-mochitest-2, v2 (obsolete) — Splinter Review
Do you think we should also change Part5?
Attachment #8885132 - Attachment is obsolete: true
Attachment #8885552 - Flags: review?(odvarko)
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
Attachment #8885552 - Flags: review?(odvarko) → review+
Do you think we should also change the pref-setting style in Part5 (Comment 41)?
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)
Attachment #8885159 - Attachment is obsolete: true
Attachment #8885623 - Flags: review?(odvarko)
Attachment #8885552 - Attachment is obsolete: true
Attachment #8885624 - Flags: review?(odvarko)
Comment on attachment 8885623 [details] [diff] [review]
Part5-devtools-mochitest, v4

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

Looks great now, thanks!

Honza
Attachment #8885623 - Flags: review?(odvarko) → review+
Comment on attachment 8885624 [details] [diff] [review]
Part10-devtools-mochitest-2, v3

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

Thanks for the patch!

Honza
Attachment #8885624 - Flags: review?(odvarko) → review+
Please check in from Part 1 to Part 10, thanks!
Note: Patch "pref-on" is a test patch, not for landing.
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
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
Flags: needinfo?(hskupin)
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.
Flags: needinfo?(hskupin)
Attached patch Part11-firefox-ui-test, v1 (obsolete) — Splinter Review
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
Attachment #8887420 - Flags: review?(hskupin)
Yes, this is because of bug 1379906. You can use '--pref security.sandbox.content.level:2' to prevent it for now.
(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.
Attachment #8887420 - Flags: review?(hskupin)
Attached patch Part11-firefox-ui-test, v2 (obsolete) — Splinter Review
Attachment #8887420 - Attachment is obsolete: true
Attachment #8887877 - Flags: review?(hskupin)
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.
Attachment #8887877 - Flags: review?(hskupin) → review+
Thanks!
Attachment #8887877 - Attachment is obsolete: true
Attachment #8888623 - Flags: review+
Please help to land Part11. Thanks!
Keywords: checkin-needed
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.
Attached patch rest - WIP v1 (obsolete) — Splinter Review
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
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Attached patch Part12: rest - v2 (obsolete) — Splinter Review
Attachment #8888717 - Attachment is obsolete: true
Attachment #8889315 - Flags: review?(michal.novotny)
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.
Attachment #8889315 - Flags: review?(michal.novotny) → review-
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.
Attachment #8889315 - Attachment is obsolete: true
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.
Attachment #8889845 - Attachment description: Part12-rest WIP, v3 → Disable the rest of tests in doubt. Patch for reference -- PLEASE DO NOT LAND
Move the discussion 
toolkit/mozapps/extensions/test/xpcshell/test_backgroundupdate.js
to Bug 1384510
Flags: needinfo?(rhelmer)
Please see comment 81.
Attachment #8890306 - Flags: review?(rchien)
Disable wpt preload since it heavily relies on cache behavior.
Attachment #8890308 - Flags: review?(michal.novotny)
Comment on attachment 8890306 [details] [diff] [review]
Part12-devtools-mochitest-3

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

LGTM. thanks!
Attachment #8890306 - Flags: review?(rchien) → review+
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
Flags: needinfo?(valentin.gosu)
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?
Flags: needinfo?(mixedpuppy)
(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
Attachment #8890308 - Flags: review?(michal.novotny) → review+
(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
Please help to land Part12 and Part13. Thanks!
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
(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.
Flags: needinfo?(mixedpuppy)
(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!
https://hg.mozilla.org/mozilla-central/rev/79b945191dea
https://hg.mozilla.org/mozilla-central/rev/9a388409e4a9
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Flags: needinfo?(valentin.gosu)
You need to log in before you can comment on or make changes to this bug.