[issues] Get target for each issue individually (instead of main target) to get correct node in case of out of process iframes

We lose information about the target in aggregated issue, so for now create primary key from issue details to get the right target.

Bug: chromium:1115421
Change-Id: I2cde1f3db599ee989e45f36518316ba9693e76ab
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2352726
Reviewed-by: Sigurd Schneider <sigurds@chromium.org>
Commit-Queue: Kateryna Prokopenko <kprokopenko@google.com>

front_end/issues/IssueAggregator.js

@@ -27,8 +27,8 @@ export class AggregatedIssue extends SDK.Issue.Issue {
     this._mixedContents = new Map();
     /** @type {!Map<string, !Protocol.Audits.HeavyAdIssueDetails>} */
     this._heavyAdIssueDetails = new Map();
-    /** @type {!Set<!Protocol.Audits.ContentSecurityPolicyIssueDetails>} */
-    this._cspViolations = new Set();
+    /** @type {!Set<!SDK.ContentSecurityPolicyIssue.ContentSecurityPolicyIssue>} */
+    this._cspIssues = new Set();
     /** @type {!Map<string, !Protocol.Audits.BlockedByResponseIssueDetails>} */
     this._blockedByResponseDetails = new Map();
     this._aggregatedIssuesCount = 0;
@@ -82,11 +82,10 @@ export class AggregatedIssue extends SDK.Issue.Issue {
   }
 
   /**
-   * @override
-   * @returns {!Set<!Protocol.Audits.ContentSecurityPolicyIssueDetails>}
+   * @returns {!Iterable<!SDK.ContentSecurityPolicyIssue.ContentSecurityPolicyIssue>}
    */
-  cspViolations() {
-    return this._cspViolations;
+  cspIssues() {
+    return this._cspIssues;
   }
 
   /**
@@ -164,13 +163,13 @@ export class AggregatedIssue extends SDK.Issue.Issue {
       const key = JSON.stringify(heavyAds);
       this._heavyAdIssueDetails.set(key, heavyAds);
     }
-    for (const cspViolation of issue.cspViolations()) {
-      this._cspViolations.add(cspViolation);
-    }
     for (const details of issue.blockedByResponseDetails()) {
       const key = JSON.stringify(details, ['parentFrame', 'blockedFrame', 'requestId', 'frameId', 'reason', 'request']);
       this._blockedByResponseDetails.set(key, details);
     }
+    if (issue instanceof SDK.ContentSecurityPolicyIssue.ContentSecurityPolicyIssue) {
+      this._cspIssues.add(issue);
+    }
   }
 }
 

front_end/issues/IssuesPane.js

@@ -410,28 +410,33 @@ class AffectedDirectivesView extends AffectedResourcesView {
   /**
    * @param {!Element} element
    * @param {number | undefined} nodeId
+   * @param {?SDK.SDKModel.Target} target
    */
-  _appendBlockedElement(element, nodeId) {
+  _appendBlockedElement(element, nodeId, target) {
     const violatingNode = document.createElement('td');
     violatingNode.classList.add('affected-resource-csp-info-node');
 
     if (nodeId) {
       const violatingNodeId = nodeId;
       const icon = UI.Icon.Icon.create('largeicon-node-search', 'icon');
+      icon.classList.add('element-reveal-icon');
 
-      const target = /** @type {!SDK.SDKModel.Target} */ (SDK.SDKModel.TargetManager.instance().mainTarget());
       icon.onclick = () => {
-        const deferredDOMNode = new SDK.DOMModel.DeferredDOMNode(target, violatingNodeId);
-        Common.Revealer.reveal(deferredDOMNode);
+        if (target) {
+          const deferredDOMNode = new SDK.DOMModel.DeferredDOMNode(target, violatingNodeId);
+          Common.Revealer.reveal(deferredDOMNode);
+        }
       };
 
       UI.Tooltip.Tooltip.install(icon, ls`Click to reveal the violating DOM node in the Elements panel`);
       violatingNode.appendChild(icon);
 
       violatingNode.onmouseenter = () => {
-        const deferredDOMNode = new SDK.DOMModel.DeferredDOMNode(target, violatingNodeId);
-        if (deferredDOMNode) {
-          deferredDOMNode.highlight();
+        if (target) {
+          const deferredDOMNode = new SDK.DOMModel.DeferredDOMNode(target, violatingNodeId);
+          if (deferredDOMNode) {
+            deferredDOMNode.highlight();
+          }
         }
       };
       violatingNode.onmouseleave = () => SDK.OverlayModel.OverlayModel.hideDOMNodeHighlight();
@@ -457,9 +462,9 @@ class AffectedDirectivesView extends AffectedResourcesView {
   }
 
   /**
-   * @param {!Set<!Protocol.Audits.ContentSecurityPolicyIssueDetails>} cspViolations
+   * @param {!Iterable<!SDK.ContentSecurityPolicyIssue.ContentSecurityPolicyIssue>} cspIssues
    */
-  _appendAffectedContentSecurityPolicyDetails(cspViolations) {
+  _appendAffectedContentSecurityPolicyDetails(cspIssues) {
     const header = document.createElement('tr');
     if (this._issue.code() === SDK.ContentSecurityPolicyIssue.inlineViolationCode) {
       this._appendDirectiveColumnTitle(header);
@@ -481,34 +486,35 @@ class AffectedDirectivesView extends AffectedResourcesView {
     }
     this._affectedResources.appendChild(header);
     let count = 0;
-    for (const cspViolation of cspViolations) {
+    for (const cspIssue of cspIssues) {
       count++;
-      this._appendAffectedContentSecurityPolicyDetail(cspViolation);
+      this._appendAffectedContentSecurityPolicyDetail(cspIssue);
     }
     this.updateAffectedResourceCount(count);
   }
 
   /**
-   * @param {!Protocol.Audits.ContentSecurityPolicyIssueDetails} cspViolation
+   * @param {!SDK.ContentSecurityPolicyIssue.ContentSecurityPolicyIssue} cspIssue
    */
-  _appendAffectedContentSecurityPolicyDetail(cspViolation) {
+  _appendAffectedContentSecurityPolicyDetail(cspIssue) {
     const element = document.createElement('tr');
     element.classList.add('affected-resource-directive');
 
+    const cspIssueDetails = cspIssue.details();
     if (this._issue.code() === SDK.ContentSecurityPolicyIssue.inlineViolationCode) {
-      this._appendViolatedDirective(element, cspViolation.violatedDirective);
-      this._appendBlockedElement(element, cspViolation.violatingNodeId);
-      this._appendSourceLocation(element, cspViolation.sourceCodeLocation);
+      this._appendViolatedDirective(element, cspIssueDetails.violatedDirective);
+      this._appendBlockedElement(element, cspIssueDetails.violatingNodeId, cspIssue.model().getTargetIfNotDisposed());
+      this._appendSourceLocation(element, cspIssueDetails.sourceCodeLocation);
       this._appendBlockedStatus(element);
     } else if (this._issue.code() === SDK.ContentSecurityPolicyIssue.urlViolationCode) {
-      const url = cspViolation.blockedURL ? cspViolation.blockedURL : '';
+      const url = cspIssueDetails.blockedURL ? cspIssueDetails.blockedURL : '';
       this._appendBlockedURL(element, url);
       this._appendBlockedStatus(element);
-      this._appendViolatedDirective(element, cspViolation.violatedDirective);
-      this._appendSourceLocation(element, cspViolation.sourceCodeLocation);
+      this._appendViolatedDirective(element, cspIssueDetails.violatedDirective);
+      this._appendSourceLocation(element, cspIssueDetails.sourceCodeLocation);
     } else if (this._issue.code() === SDK.ContentSecurityPolicyIssue.evalViolationCode) {
-      this._appendSourceLocation(element, cspViolation.sourceCodeLocation);
-      this._appendViolatedDirective(element, cspViolation.violatedDirective);
+      this._appendSourceLocation(element, cspIssueDetails.sourceCodeLocation);
+      this._appendViolatedDirective(element, cspIssueDetails.violatedDirective);
       this._appendBlockedStatus(element);
     } else {
       return;
@@ -522,7 +528,7 @@ class AffectedDirectivesView extends AffectedResourcesView {
    */
   update() {
     this.clear();
-    this._appendAffectedContentSecurityPolicyDetails(this._issue.cspViolations());
+    this._appendAffectedContentSecurityPolicyDetails(this._issue.cspIssues());
   }
 }
 

front_end/sdk/ContentSecurityPolicyIssue.js

@@ -3,19 +3,22 @@
 // found in the LICENSE file.
 
 import {ls} from '../platform/platform.js';
-
 import {Issue, IssueCategory, IssueDescription, IssueKind} from './Issue.js';  // eslint-disable-line no-unused-vars
+import {IssuesModel} from './IssuesModel.js';                                  // eslint-disable-line no-unused-vars
+
 
 export class ContentSecurityPolicyIssue extends Issue {
   /**
    * @param {!Protocol.Audits.ContentSecurityPolicyIssueDetails} issueDetails
+   * @param {!IssuesModel} issuesModel
    */
-  constructor(issueDetails) {
+  constructor(issueDetails, issuesModel) {
     const issue_code = [
       Protocol.Audits.InspectorIssueCode.ContentSecurityPolicyIssue, issueDetails.contentSecurityPolicyViolationType
     ].join('::');
     super(issue_code);
     this._issueDetails = issueDetails;
+    this._issuesModel = issuesModel;
   }
 
   /**
@@ -50,11 +53,17 @@ export class ContentSecurityPolicyIssue extends Issue {
   }
 
   /**
-   * @override
-   * @returns {!Iterable<!Protocol.Audits.ContentSecurityPolicyIssueDetails>}
+   * @returns {!IssuesModel}
    */
-  cspViolations() {
-    return [this._issueDetails];
+  model() {
+    return this._issuesModel;
+  }
+
+  /**
+   * @returns {!Protocol.Audits.ContentSecurityPolicyIssueDetails}
+   */
+  details() {
+    return this._issueDetails;
   }
 }
 

front_end/sdk/Issue.js

@@ -126,13 +126,6 @@ export class Issue extends Common.ObjectWrapper.ObjectWrapper {
     return [];
   }
 
-  /**
-   * @returns {!Iterable<!Protocol.Audits.ContentSecurityPolicyIssueDetails>}
-   */
-  cspViolations() {
-    return [];
-  }
-
   /**
    * @return {!Iterable<!Protocol.Audits.AffectedRequest>}
    */

front_end/sdk/IssuesModel.js

@@ -28,6 +28,7 @@ export class IssuesModel extends SDKModel {
     /** @type {*} */
     this._auditsAgent = null;
     this.ensureEnabled();
+    this._disposed = false;
   }
 
   /**
@@ -81,6 +82,24 @@ export class IssuesModel extends SDKModel {
     console.warn(`No handler registered for issue code ${inspectorIssue.code}`);
     return [];
   }
+
+  /**
+   * @override
+   */
+  dispose() {
+    super.dispose();
+    this._disposed = true;
+  }
+
+  /**
+   * @returns {?Target}
+   */
+  getTargetIfNotDisposed() {
+    if (!this._disposed) {
+      return this.target();
+    }
+    return null;
+  }
 }
 
 /**
@@ -124,7 +143,7 @@ function createIssuesForContentSecurityPolicyIssue(issuesModel, inspectorDetails
     console.warn('Content security policy issue without details received.');
     return [];
   }
-  return [new ContentSecurityPolicyIssue(cspDetails)];
+  return [new ContentSecurityPolicyIssue(cspDetails, issuesModel)];
 }
 
 

test/e2e/elements/BUILD.gn

@@ -12,6 +12,7 @@ node_ts_library("elements") {
     "element-breadcrumbs_test.ts",
     "layout-pane_test.ts",
     "pseudo-states_test.ts",
+    "reveal-correct-node_test.ts",
     "selection-after-delete_test.ts",
     "shadowroot-styles_test.ts",
     "sidebar-event-listeners-remove_test.ts",

test/e2e/elements/reveal-correct-node_test.ts

@@ -0,0 +1,22 @@
+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import {goToResource} from '../../shared/helper.js';
+import {describe, it} from '../../shared/mocha-extensions.js';
+import {waitForSelectedTreeElementSelectorWhichIncludesText} from '../helpers/elements-helpers.js';
+import {expandIssue, navigateToIssuesTab, revealNodeInElementsPanel} from '../helpers/issues-helpers.js';
+
+// TODO: Add a second node reveal test, where am issue is produced by an OOPIF
+
+describe('The Issues tab', async () => {
+  it('should reveal an element in the Elements panelwhen the node icon is clicked', async () => {
+    await goToResource('elements/element-reveal-inline-issue.html');
+
+    await navigateToIssuesTab();
+    await expandIssue();
+    await revealNodeInElementsPanel();
+
+    await waitForSelectedTreeElementSelectorWhichIncludesText('alert("This should be blocked by CSP");');
+  });
+});

test/e2e/helpers/BUILD.gn

@@ -16,6 +16,7 @@ node_ts_library("helpers") {
     "elements-helpers.ts",
     "emulation-helpers.ts",
     "event-listeners-helpers.ts",
+    "issues-helpers.ts",
     "layers-helpers.ts",
     "lighthouse-helpers.ts",
     "media-helpers.ts",

test/e2e/helpers/elements-helpers.ts

@@ -110,6 +110,14 @@ export const waitForSelectedTreeElementSelectorWithTextcontent = async (expected
   });
 };
 
+export const waitForSelectedTreeElementSelectorWhichIncludesText = async (expectedTextContent: string) => {
+  await waitForFunction(async () => {
+    const selectedNode = await waitFor(SELECTED_TREE_ELEMENT_SELECTOR);
+    const selectedTextContent = await selectedNode.evaluate(node => node.textContent);
+    return selectedTextContent && selectedTextContent.includes(expectedTextContent);
+  });
+};
+
 export const waitForChildrenOfSelectedElementNode = async () => {
   await waitFor(`${SELECTED_TREE_ELEMENT_SELECTOR} + ol > li`);
 };

test/e2e/helpers/issues-helpers.ts

@@ -0,0 +1,26 @@
+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import {click, waitFor} from '../../shared/helper.js';
+import {openPanelViaMoreTools} from './settings-helpers';
+
+export const ISSUE = '.issue';
+export const AFFECTED_ELEMENT_ICON = '.affected-resource-csp-info-node';
+export const ELEMENT_REVEAL_ICON = '.element-reveal-icon';
+export const ELEMENTS_PANEL_SELECTOR = '.panel[aria-label="elements"]';
+
+export async function navigateToIssuesTab() {
+  await openPanelViaMoreTools('Issues');
+}
+
+export async function expandIssue() {
+  await waitFor(ISSUE);
+  await click(ISSUE);
+  await waitFor('.message');
+}
+
+export async function revealNodeInElementsPanel() {
+  const revealIcon = await waitFor(ELEMENT_REVEAL_ICON);
+  await revealIcon.click();
+}

test/e2e/resources/elements/BUILD.gn

@@ -10,6 +10,7 @@ copy_to_gen("elements") {
     "css-grid.html",
     "css-variables.html",
     "element-breadcrumbs.html",
+    "element-reveal-inline-issue.html",
     "focus.html",
     "hover.html",
     "multiple-constructed-stylesheets.html",

test/e2e/resources/elements/element-reveal-inline-issue.html

@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<meta http-equiv="Content-Security-Policy" content="script-src 'self'" />
+
+<html>
+    <body>
+        <h2>Webpage with an oopif causing an issue of inline violation type</h2>
+
+        <div>
+            <script class="violating-script">
+                alert("This should be blocked by CSP");
+            </script>
+        </div>
+    </body>
+
+</html>