From 314d6b7155ca554c64b95bbc2ea2bb68f41f0a50 Mon Sep 17 00:00:00 2001 From: Kateryna Prokopenko Date: Wed, 19 Aug 2020 06:33:00 +0000 Subject: [PATCH] [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 Commit-Queue: Kateryna Prokopenko --- front_end/issues/IssueAggregator.js | 17 +++---- front_end/issues/IssuesPane.js | 50 +++++++++++-------- front_end/sdk/ContentSecurityPolicyIssue.js | 21 +++++--- front_end/sdk/Issue.js | 7 --- front_end/sdk/IssuesModel.js | 21 +++++++- test/e2e/elements/BUILD.gn | 1 + test/e2e/elements/reveal-correct-node_test.ts | 22 ++++++++ test/e2e/helpers/BUILD.gn | 1 + test/e2e/helpers/elements-helpers.ts | 8 +++ test/e2e/helpers/issues-helpers.ts | 26 ++++++++++ test/e2e/resources/elements/BUILD.gn | 1 + .../elements/element-reveal-inline-issue.html | 15 ++++++ 12 files changed, 145 insertions(+), 45 deletions(-) create mode 100644 test/e2e/elements/reveal-correct-node_test.ts create mode 100644 test/e2e/helpers/issues-helpers.ts create mode 100644 test/e2e/resources/elements/element-reveal-inline-issue.html diff --git a/front_end/issues/IssueAggregator.js b/front_end/issues/IssueAggregator.js index 78e1c9c6ac..c5cebc67fb 100644 --- a/front_end/issues/IssueAggregator.js +++ b/front_end/issues/IssueAggregator.js @@ -27,8 +27,8 @@ export class AggregatedIssue extends SDK.Issue.Issue { this._mixedContents = new Map(); /** @type {!Map} */ this._heavyAdIssueDetails = new Map(); - /** @type {!Set} */ - this._cspViolations = new Set(); + /** @type {!Set} */ + this._cspIssues = new Set(); /** @type {!Map} */ this._blockedByResponseDetails = new Map(); this._aggregatedIssuesCount = 0; @@ -82,11 +82,10 @@ export class AggregatedIssue extends SDK.Issue.Issue { } /** - * @override - * @returns {!Set} + * @returns {!Iterable} */ - 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); + } } } diff --git a/front_end/issues/IssuesPane.js b/front_end/issues/IssuesPane.js index d46f97af69..c6b0405314 100644 --- a/front_end/issues/IssuesPane.js +++ b/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} cspViolations + * @param {!Iterable} 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()); } } diff --git a/front_end/sdk/ContentSecurityPolicyIssue.js b/front_end/sdk/ContentSecurityPolicyIssue.js index a3ff92343b..509016d679 100644 --- a/front_end/sdk/ContentSecurityPolicyIssue.js +++ b/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} + * @returns {!IssuesModel} */ - cspViolations() { - return [this._issueDetails]; + model() { + return this._issuesModel; + } + + /** + * @returns {!Protocol.Audits.ContentSecurityPolicyIssueDetails} + */ + details() { + return this._issueDetails; } } diff --git a/front_end/sdk/Issue.js b/front_end/sdk/Issue.js index 608dc18f89..db4034611f 100644 --- a/front_end/sdk/Issue.js +++ b/front_end/sdk/Issue.js @@ -126,13 +126,6 @@ export class Issue extends Common.ObjectWrapper.ObjectWrapper { return []; } - /** - * @returns {!Iterable} - */ - cspViolations() { - return []; - } - /** * @return {!Iterable} */ diff --git a/front_end/sdk/IssuesModel.js b/front_end/sdk/IssuesModel.js index 2f820efb26..57e3eac21a 100644 --- a/front_end/sdk/IssuesModel.js +++ b/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)]; } diff --git a/test/e2e/elements/BUILD.gn b/test/e2e/elements/BUILD.gn index fa77ff21bb..8107fed651 100644 --- a/test/e2e/elements/BUILD.gn +++ b/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", diff --git a/test/e2e/elements/reveal-correct-node_test.ts b/test/e2e/elements/reveal-correct-node_test.ts new file mode 100644 index 0000000000..af6bde1a81 --- /dev/null +++ b/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");'); + }); +}); diff --git a/test/e2e/helpers/BUILD.gn b/test/e2e/helpers/BUILD.gn index 5ba85bdbb5..e4740bb4f0 100644 --- a/test/e2e/helpers/BUILD.gn +++ b/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", diff --git a/test/e2e/helpers/elements-helpers.ts b/test/e2e/helpers/elements-helpers.ts index e431281adb..6c006195b8 100644 --- a/test/e2e/helpers/elements-helpers.ts +++ b/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`); }; diff --git a/test/e2e/helpers/issues-helpers.ts b/test/e2e/helpers/issues-helpers.ts new file mode 100644 index 0000000000..7311119472 --- /dev/null +++ b/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(); +} diff --git a/test/e2e/resources/elements/BUILD.gn b/test/e2e/resources/elements/BUILD.gn index 83c098d13c..32c836dc78 100644 --- a/test/e2e/resources/elements/BUILD.gn +++ b/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", diff --git a/test/e2e/resources/elements/element-reveal-inline-issue.html b/test/e2e/resources/elements/element-reveal-inline-issue.html new file mode 100644 index 0000000000..0a6fe1af8c --- /dev/null +++ b/test/e2e/resources/elements/element-reveal-inline-issue.html @@ -0,0 +1,15 @@ + + + + + +

Webpage with an oopif causing an issue of inline violation type

+ +
+ +
+ + + \ No newline at end of file