Merge pull request #1361 from ampproject/fix-security

Fix test flakiness caused by Puppeteer state management

Author: erwinmombay

package-lock.json

@@ -27,7 +27,7 @@
   "devDependencies": {
     "@ampproject/rollup-plugin-closure-compiler": "0.27.0",
     "@babel/core": "7.13.10",
-    "@types/cheerio": "0.22.28",
+    "@types/cheerio": "0.22.35",
     "@types/debug": "4.1.5",
     "@types/diff": "5.0.0",
     "@types/escape-html": "1.0.0",

package.json

@@ -17,7 +17,7 @@
     "@ampproject/toolbox-validator-rules": "2.9.0",
     "amphtml-validator": "1.0.35",
     "chalk": "4.1.0",
-    "cheerio": "1.0.0-rc.5",
+    "cheerio": "1.0.0-rc.12",
     "commander": "9.3.0",
     "cross-fetch": "3.1.5",
     "debug": "4.3.1",

packages/linter/package.json

@@ -15,7 +15,7 @@ export class AmpImgUsesSrcSet extends Rule {
       let layout = $(e).attr('layout');
       const srcset = $(e).attr('srcset');
       const parent = $(e).parent();
-      if (parent.prop('tagName').startsWith('AMP-')) {
+      if (parent.prop('tagName')?.startsWith('AMP-')) {
         const parentLayout = $(parent).attr('layout');
         if (parentLayout) {
           layout = parentLayout;

packages/linter/src/rules/AmpImgUsesSrcSet.ts

@@ -9,7 +9,7 @@
   "author": "AMPHTML team",
   "license": "Apache-2.0",
   "dependencies": {
-    "cheerio": "^1.0.0-rc.2"
+    "cheerio": "^1.0.0-rc.22"
   },
   "devDependencies": {
     "@ampproject/toolbox-optimizer": "2.5.14"

packages/optimizer/demo/cheerio/package.json

@@ -6,7 +6,6 @@ let instance = null;
 module.exports = {
   get: () => {
     if (!instance) {
-      console.error('Validator instance created: ' + path.join(__dirname, 'validator.js'));
       instance = validator.getInstance(path.join(__dirname, 'validator.js'));
     }
     return instance;

packages/optimizer/spec/helpers/validatorInstance.js

@@ -58,20 +58,16 @@ class PageAnalyzer {
       throw new Error('Puppeteer not running, please call `start` first.');
     }
     const {page, remoteStyles, responsePromise} = await this.setupPage();
-    try {
-      await page.goto(url, {waitUntil: 'load'});
+    await page.goto(url, {waitUntil: 'load'});
 
-      const response = await responsePromise;
-      if (!response) {
-        throw new Error('Failed loading url', url);
-      }
-      const {html, headers} = response;
-      return await this.gatherPageData(page, {remoteStyles, html, headers});
-    } finally {
-      if (page) {
-        page.close();
-      }
+    const response = await responsePromise;
+    if (!response) {
+      throw new Error('Failed loading url', url);
     }
+    const {html, headers} = response;
+    const data = await this.gatherPageData(page, {remoteStyles, html, headers});
+    await page.close();
+    return data;
   }
 
   /**
@@ -80,6 +76,7 @@ class PageAnalyzer {
   async shutdown() {
     try {
       await this.browser.close();
+      this.browser = null;
     } catch (e) {
       console.log(e);
     }

packages/page-experience/lib/PageDataGatherer.js

@@ -39,7 +39,7 @@
   "homepage": "https://github.com/ampproject/amp-toolbox/tree/main/packages/page-experience",
   "dependencies": {
     "@ampproject/toolbox-linter": "2.9.0",
-    "cheerio": "1.0.0-rc.5",
+    "cheerio": "1.0.0-rc.12",
     "css-font-face-src": "1.0.0",
     "postcss": "8.2.8",
     "postcss-safe-parser": "5.0.2",