Hmm, I did touch navigation preload this week, so maybe I broke something.  I'm not sure how my changes could have affected headers, though.

Looks like really flaky. Ben: do you think you can take a look?

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win7%20Tests%20%28dbg%29%281%29/72340
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win7%20Tests%20%28dbg%29%281%29
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Tests%20%28dbg%29%281%29%2832%29/53607
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Tests%20%28dbg%29%281%29%2832%29/53605

I'll look, this test has always been crazy iffy ( issue 876911 )

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/590c394436a04eb8d7fb7cf65552bb1801a6c17b

commit 590c394436a04eb8d7fb7cf65552bb1801a6c17b
Author: Kinuko Yasuda <kinuko@chromium.org>
Date: Thu Oct 18 06:38:17 2018

Disable ServiceWorkerNavigationPreloadTest.NetworkFallback

Looks like this one's really flaky on multiple bots.

TBR=falken@chromium.org

Bug:  896230 
Change-Id: I2e2ca10912d7a75ed52e1e668110444fb0817f6c
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/1288159
Commit-Queue: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600673}
[modify] https://crrev.com/590c394436a04eb8d7fb7cf65552bb1801a6c17b/content/browser/service_worker/service_worker_browsertest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/883e242706b33b81b2a401792881353e6c0b95db

commit 883e242706b33b81b2a401792881353e6c0b95db
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Oct 18 08:11:27 2018

Enable ServiceWorkerNavigationPreloadTest.NetworkFallback.

I suspect the errors were that we were getting no preload request and
two fallback requests, which seems plausible based on the existing
comment. Loosen the test expectations accordingly.

Bug:  896230 
Change-Id: I539b56343d5057975352f74b482b61583174fb13
Reviewed-on: https://chromium-review.googlesource.com/c/1288330
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600688}
[modify] https://crrev.com/883e242706b33b81b2a401792881353e6c0b95db/content/browser/service_worker/service_worker_browsertest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/087a06d7f519a33eb11df43869818e35e1cb3e88

commit 087a06d7f519a33eb11df43869818e35e1cb3e88
Author: Christian Dullweber <dullweber@chromium.org>
Date: Thu Oct 18 11:42:12 2018

Revert "Enable ServiceWorkerNavigationPreloadTest.NetworkFallback."

This reverts commit 883e242706b33b81b2a401792881353e6c0b95db.

Reason for revert: Still failing:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Tests%20%28dbg%29%281%29%2832%29/53613
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Tests%20%28dbg%29%281%29%2832%29/53612

Original change's description:
> Enable ServiceWorkerNavigationPreloadTest.NetworkFallback.
> 
> I suspect the errors were that we were getting no preload request and
> two fallback requests, which seems plausible based on the existing
> comment. Loosen the test expectations accordingly.
> 
> Bug:  896230 
> Change-Id: I539b56343d5057975352f74b482b61583174fb13
> Reviewed-on: https://chromium-review.googlesource.com/c/1288330
> Commit-Queue: Matt Falkenhagen <falken@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#600688}

TBR=falken@chromium.org,kinuko@chromium.org

Change-Id: I7f2ac588a506f1c346db8925b4657b67b5c8b654
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  896230 
Reviewed-on: https://chromium-review.googlesource.com/c/1288593
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600730}
[modify] https://crrev.com/087a06d7f519a33eb11df43869818e35e1cb3e88/content/browser/service_worker/service_worker_browsertest.cc

Is there a reason this test uses the activate event to enable navigation preload?  Would enabling it in the install event perhaps avoid some races?

The spec, for example, does not wait for activation before making the determination whether navigation preload should occur.  See steps 12.7 and 16 here:

https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm

Chrome code seems to match this spec text.

Of course, the WorkerActivatedObserver in the test should still wait for activation, but it does seem like maybe there could be a race between this observer callback and the callbacks used dispatch fetch, etc.  The observer is fired before these other callbacks:

https://cs.chromium.org/chromium/src/content/browser/service_worker/service_worker_version.cc?l=342&rcl=774ca8ad2fa4431905e40c1ad40d2870c3c9fe8d

I don't understand how it could cause a race, but it seems like there is some potential for weird interaction and brittleness there.

Oh, the answer is that enable() is only available for active works.  Sorry for my confusion.

https://w3c.github.io/ServiceWorker/#navigation-preload-manager-enable

The new test failure was here:
EXPECT_EQ(0, fallback_count);

fallback_count was 1. This means a fallback request arrived before the navigation preload request.

Not sure why that's happening.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/03dd4343512f994bce017ce645c0d8b1c764a655

commit 03dd4343512f994bce017ce645c0d8b1c764a655
Author: Matt Falkenhagen <falken@chromium.org>
Date: Mon Oct 22 15:36:20 2018

Enable ServiceWorkerNavigationPreloadTest.NetworkFallback.

Re-enabling with looser expectations so there is still test coverage of
this case. The failure was that the preload request came after the
fallback request. The order actually might not be guaranteed if they
are on different loading pipelines/http connections? Anyway, the
important thing is that the page loads and it's not worth investigating
more as network fallback from navigation preload is an anti-pattern.

Bug:  896230 
Change-Id: Idadfc573fbbeb2c0703c3a3ffa1efef14b991614
Reviewed-on: https://chromium-review.googlesource.com/c/1290573
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601581}
[modify] https://crrev.com/03dd4343512f994bce017ce645c0d8b1c764a655/content/browser/service_worker/service_worker_browsertest.cc