core(main-resource): fix protocol error when page is reloaded

GoogleChrome · Nov 10, 2022 · 9859e0a · 9859e0a
1 parent 1b843de
commit 9859e0a
Show file tree

Hide file tree

Showing 5 changed files with 98 additions and 17 deletions.
diff --git a/cli/test/fixtures/sw-reloaded.html b/cli/test/fixtures/sw-reloaded.html
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<html lang="en">
+
+<head>
+  <meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1">
+  <title>Service worker, reloaded</title>
+</head>
+
+<body>
+  Service worker, reloaded
+  <script>
+    navigator.serviceWorker.addEventListener('controllerchange', () => {
+      console.log('reloading');
+      window.location.reload();
+    });
+    navigator.serviceWorker.register('./offline-ready-sw.js');
+  </script>
+</body>
+
+</html>
diff --git a/cli/test/smokehouse/core-tests.js b/cli/test/smokehouse/core-tests.js
@@ -14,6 +14,8 @@ import errorsExpiredSsl from './test-definitions/errors-expired-ssl.js';
 import errorsIframeExpiredSsl from './test-definitions/errors-iframe-expired-ssl.js';
 import errorsInfiniteLoop from './test-definitions/errors-infinite-loop.js';
 import formsAutoComplete from './test-definitions/forms-autocomplete.js';
+import fpsMax from './test-definitions/fps-max.js';
+import fpsScaled from './test-definitions/fps-scaled.js';
 import issuesMixedContent from './test-definitions/issues-mixed-content.js';
 import lanternFetch from './test-definitions/lantern-fetch.js';
 import lanternIdleCallbackLong from './test-definitions/lantern-idle-callback-long.js';
@@ -49,21 +51,20 @@ import pwaChromestatus from './test-definitions/pwa-chromestatus.js';
 import pwaRocks from './test-definitions/pwa-rocks.js';
 import pwaSvgomg from './test-definitions/pwa-svgomg.js';
 import redirectsClientPaintServer from './test-definitions/redirects-client-paint-server.js';
+import redirectScripts from './test-definitions/redirects-scripts.js';
 import redirectsHistoryPushState from './test-definitions/redirects-history-push-state.js';
 import redirectsMultipleServer from './test-definitions/redirects-multiple-server.js';
-import redirectScripts from './test-definitions/redirects-scripts.js';
+import redirectsSelf from './test-definitions/redirects-self.js';
 import redirectsSingleClient from './test-definitions/redirects-single-client.js';
 import redirectsSingleServer from './test-definitions/redirects-single-server.js';
-import redirectsSelf from './test-definitions/redirects-self.js';
 import screenshot from './test-definitions/screenshot.js';
 import seoFailing from './test-definitions/seo-failing.js';
 import seoPassing from './test-definitions/seo-passing.js';
 import seoStatus403 from './test-definitions/seo-status-403.js';
 import seoTapTargets from './test-definitions/seo-tap-targets.js';
+import serviceWorkerReloaded from './test-definitions/service-worker-reloaded.js';
 import sourceMaps from './test-definitions/source-maps.js';
 import timing from './test-definitions/timing.js';
-import fpsScaled from './test-definitions/fps-scaled.js';
-import fpsMax from './test-definitions/fps-max.js';
 
 /** @type {ReadonlyArray<Smokehouse.TestDfn>} */
 const smokeTests = [
@@ -77,19 +78,21 @@ const smokeTests = [
   errorsIframeExpiredSsl,
   errorsInfiniteLoop,
   formsAutoComplete,
+  fpsMax,
+  fpsScaled,
   issuesMixedContent,
+  lanternFetch,
+  lanternIdleCallbackLong,
+  lanternIdleCallbackShort,
   lanternOnline,
   lanternSetTimeout,
-  lanternFetch,
   lanternXhr,
-  lanternIdleCallbackShort,
-  lanternIdleCallbackLong,
   legacyJavascript,
   metricsDebugger,
   metricsDelayedFcp,
   metricsDelayedLcp,
-  metricsTrickyTtiLateFcp,
   metricsTrickyTti,
+  metricsTrickyTtiLateFcp,
   offlineOnlineOnly,
   offlineReady,
   offlineSwBroken,
@@ -107,26 +110,25 @@ const smokeTests = [
   perfTraceElements,
   pubads,
   pwaAirhorner,
-  pwaChromestatus,
-  pwaSvgomg,
   pwaCaltrain,
+  pwaChromestatus,
   pwaRocks,
+  pwaSvgomg,
   redirectsClientPaintServer,
+  redirectScripts,
   redirectsHistoryPushState,
   redirectsMultipleServer,
-  redirectScripts,
+  redirectsSelf,
   redirectsSingleClient,
   redirectsSingleServer,
-  redirectsSelf,
   screenshot,
   seoFailing,
   seoPassing,
   seoStatus403,
   seoTapTargets,
+  serviceWorkerReloaded,
   sourceMaps,
   timing,
-  fpsScaled,
-  fpsMax,
 ];
 
 export default smokeTests;
diff --git a/cli/test/smokehouse/test-definitions/service-worker-reloaded.js b/cli/test/smokehouse/test-definitions/service-worker-reloaded.js
@@ -0,0 +1,33 @@
+/**
+ * @license Copyright 2022 The Lighthouse Authors. All Rights Reserved.
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
+ */
+
+/** @type {LH.Config.Json} */
+const config = {
+  extends: 'lighthouse:default',
+  settings: {
+    onlyCategories: ['best-practices'],
+  },
+};
+
+/**
+ * @type {Smokehouse.ExpectedRunnerResult}
+ */
+const expectations = {
+  artifacts: {
+    MainDocumentContent: /reloaded/,
+  },
+  lhr: {
+    requestedUrl: 'http://localhost:10200/sw-reloaded.html',
+    finalDisplayedUrl: 'http://localhost:10200/sw-reloaded.html',
+    audits: {},
+  },
+};
+
+export default {
+  id: 'service-worker-reloaded',
+  expectations,
+  config,
+};
diff --git a/core/computed/main-resource.js b/core/computed/main-resource.js
@@ -5,12 +5,12 @@
  */
 
 import {makeComputedArtifact} from './computed-artifact.js';
-import {NetworkAnalyzer} from '../lib/dependency-graph/simulator/network-analyzer.js';
 import {NetworkRecords} from './network-records.js';
+import UrlUtils from '../lib/url-utils.js';
 
 /**
  * @fileoverview This artifact identifies the main resource on the page. Current solution assumes
- * that the main resource is the first non-rediected one.
+ * that the main resource is the first non-redirected one.
  */
 class MainResource {
   /**
@@ -22,7 +22,16 @@ class MainResource {
     const {mainDocumentUrl} = data.URL;
     if (!mainDocumentUrl) throw new Error('mainDocumentUrl must exist to get the main resource');
     const requests = await NetworkRecords.request(data.devtoolsLog, context);
-    const mainResource = NetworkAnalyzer.findResourceForUrl(requests, mainDocumentUrl);
+
+    const mainResourceRequests = requests.filter(request =>
+      UrlUtils.equalWithExcludedFragments(request.url, mainDocumentUrl)
+    );
+    // We could have more than one record matching the main doucment url,
+    // if the page did `location.reload()`. Since `mainDocumentUrl` refers to the _last_
+    // document request, we should return the last candidate here. Besides, the browser
+    // would have evicted the first request by the time `MainDocumentRequest` (a consumer
+    // of this computed artifact) attempts to fetch the contents, resulting in a protocol error.
+    const mainResource = mainResourceRequests[mainResourceRequests.length - 1];
     if (!mainResource) {
       throw new Error('Unable to identify the main resource');
     }

diff --git a/core/test/computed/main-resource-test.js b/core/test/computed/main-resource-test.js
@@ -71,4 +71,21 @@ describe('MainResource computed artifact', () => {
       assert.equal(output.url, 'https://beta.httparchive.org/reports/state-of-the-web');
     });
   });
+
+  it('should identify correct main resource with multiple candidates', () => {
+    const networkRecords = [
+      {url: 'https://example.com'},
+      {url: 'https://example.com/sw.js'},
+      {url: 'https://example.com#pickme'},
+    ];
+
+    const URL = {mainDocumentUrl: 'https://example.com'};
+    const devtoolsLog = networkRecordsToDevtoolsLog(networkRecords);
+    const artifacts = {URL, devtoolsLog};
+
+    const context = {computedCache: new Map()};
+    return MainResource.request(artifacts, context).then(output => {
+      assert.equal(output.url, 'https://example.com#pickme');
+    });
+  });
 });