[SPARK-31534][WEBUI] Text for tooltip should be escaped. #28317

sarutak · 2020-04-23T17:15:13Z

What changes were proposed in this pull request?

This PR escapes text for tooltip for DAG Viz and Timeline View.

Why are the changes needed?

This is a bug.
Normally, DAG Viz and Timeline View show tooltip like as follows.

They contain a callsite properly.
However, if a callsite contains characters which should be escaped for HTML without escaping , the corresponding tooltips wouldn't show the callsite and its following text properly.

The reason of this issue is that the source texts of the tooltip texts are not escaped.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

I tested manually.
First, I ran a job sc.parallelize(1 to 10).collect in Spark Shell then, visited AllJobsPage and JobPage and confirmed tooltip texts.

I also added a testcase.

dongjoon-hyun · 2020-04-23T17:23:45Z

Thank you, @sarutak . I'll take a look.

dongjoon-hyun · 2020-04-23T17:27:54Z

core/src/main/scala/org/apache/spark/ui/jobs/AllJobsPage.scala

@@ -88,7 +88,8 @@ private[ui] class AllJobsPage(parent: JobsTab, store: AppStatusStore) extends We
      // The timeline library treats contents as HTML, so we have to escape them. We need to add
      // extra layers of escaping in order to embed this in a Javascript string literal.
      val escapedDesc = Utility.escape(jobDescription)
-      val jsEscapedDesc = StringEscapeUtils.escapeEcmaScript(escapedDesc)
+      val jsEscapedDescForTooltip = StringEscapeUtils.escapeEcmaScript(Utility.escape(escapedDesc))


Ur, this is triple escaping since we are using escapedDesc?
Technically, this is equivalent StringEscapeUtils.escapeEcmaScript(Utility.escape(Utility.escape(jobDescription))).

If we'd like to get a tooltip text <console>, the value of the corresponding attribute data-title should be <console>.
With single Utility.escape, the value of attribute will be <console> so additional Utility.escape is needed.

Got it. Thanks~

dongjoon-hyun · 2020-04-23T17:28:48Z

core/src/main/resources/org/apache/spark/ui/static/spark-dag-viz.js

-    .replace(/&gt;/g, ">")
-    .replace(/&quot;/g, "\"");
-  var g = graphlibDot.read(escaped_dot);
+  var g = graphlibDot.read(dot);


So, did we handle all callers of renderDot in this PR?

Ah, I've noticed StagePage needs those escaping.

In the current implementation, a source text is used for tooltip text as HTML and used for node label as plain text for DAG Viz.
I changed the usage of the source text for node label to interpret it as HTML.
So, I believe these escaping I remove from renderDot are not needed.

SparkQA · 2020-04-23T20:42:19Z

Test build #121692 has finished for PR 28317 at commit e01d458.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-04-24T01:53:47Z

Test build #121698 has finished for PR 28317 at commit d3690bd.

This patch fails from timeout after a configured wait of 400m.
This patch merges cleanly.
This patch adds no public classes.

sarutak · 2020-04-24T02:06:07Z

retest this please.

SparkQA · 2020-04-24T05:08:28Z

Test build #121709 has finished for PR 28317 at commit d3690bd.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

sarutak · 2020-04-24T08:18:33Z

retest this please.

SparkQA · 2020-04-24T11:26:22Z

Test build #121740 has finished for PR 28317 at commit d3690bd.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-04-24T14:56:15Z

Test build #121755 has finished for PR 28317 at commit 3b8b071.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-04-24T15:33:38Z

Retest this please.

dongjoon-hyun · 2020-04-24T17:49:57Z

core/src/test/scala/org/apache/spark/ui/UISeleniumSuite.scala

@@ -772,6 +773,33 @@ class UISeleniumSuite extends SparkFunSuite with WebBrowser with Matchers with B
    }
  }

+  test("SPARK-31534: text for tooltip should be escaped") {


Thank you for adding this.

SparkQA · 2020-04-24T18:33:46Z

Test build #121767 has finished for PR 28317 at commit 3b8b071.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

sarutak · 2020-04-24T21:09:16Z

retest this please.

SparkQA · 2020-04-24T23:28:51Z

Test build #121781 has finished for PR 28317 at commit 3b8b071.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

sarutak · 2020-04-25T00:07:01Z

retest this please.

dongjoon-hyun · 2020-04-25T01:00:14Z

Wow. The current Jenkins environment is really flaky.

SparkQA · 2020-04-25T03:49:46Z

Test build #121788 has finished for PR 28317 at commit 3b8b071.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-04-25T04:44:13Z

We reverted 9faad07 from the master. Let's trigger once more tomorrow.

sarutak · 2020-04-25T10:25:16Z

retest this please.

SparkQA · 2020-04-25T12:50:07Z

Test build #121803 has finished for PR 28317 at commit 3b8b071.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-04-25T20:48:21Z

@sarutak . I've been digging the following error message, but didn't find a clue. Do you have any idea about this ERROR message in your new test case? It's not a failure, but I just want to understand the reason why this complains.

$ build/sbt "core/testOnly *.UISeleniumSuite -- -z SPARK-31534"
[info] UISeleniumSuite:
Error: TOOLTIP: Option "sanitizeFn" provided type "window" but expected type "(null|function)". (http://localhost:54471/static/bootstrap.bundle.min.js#6)
[info] - SPARK-31534: text for tooltip should be escaped (7 seconds, 627 milliseconds)

sarutak · 2020-04-26T17:58:33Z

@dongjoon-hyun I inspected the error message by replacing bootstrap.bundle.min.js with bootstrap.bundle.js and inserting logging code before the error message shown like as follows.

if (!new RegExp(expectedTypes).test(valueType)) {
  console.warn("Property Name: " + property + ", value of the property: " + config[property] + ", type of the property: " + toType(config[property]));
  throw new Error(componentName.toUpperCase() + ": " + ("Option \"" + property + "\" provided type \"" + valueType + "\" ") + ("but expected type \"" + expectedTypes + "\"."));
}

Then, I found the reason was the type of null is regarded as window by HtmlUnit (it's a surprising behavior...).

WARN WebConsole: Property Name: sanitizeFn, value of the property: null, type of the property: window

dongjoon-hyun · 2020-04-26T18:33:17Z

Oh, thank you for the instruction, @sarutak . It's helpful.

dongjoon-hyun · 2020-04-26T18:34:16Z

So, it sounds like we are good to go.

dongjoon-hyun · 2020-04-26T18:35:15Z

Feel free to proceed to merge, @sarutak ! Thanks.

sarutak · 2020-04-26T20:06:15Z

Thanks all !
I've noticed that if we merge this into branch-3.0, we'll get DAG with white colored font.

So, I'll merge this into only master for now.

dongjoon-hyun · 2020-04-27T00:00:08Z

I merged #28352 now. Could you reconsider about backporting?

### What changes were proposed in this pull request? This PR escapes text for tooltip for DAG Viz and Timeline View. ### Why are the changes needed? This is a bug. Normally, DAG Viz and Timeline View show tooltip like as follows. <img width="278" alt="dag-viz-tooltip" src="https://user-images.githubusercontent.com/4736016/80127481-5a6c6880-85cf-11ea-8daf-cfd59aa3ba09.png"> <img width="477" alt="timeline-tooltip" src="https://user-images.githubusercontent.com/4736016/80127500-60624980-85cf-11ea-9b0f-cce301019e3a.png"> They contain a callsite properly. However, if a callsite contains characters which should be escaped for HTML without escaping , the corresponding tooltips wouldn't show the callsite and its following text properly. <img width="179" alt="dag-viz-tooltip-before-fixed" src="https://user-images.githubusercontent.com/4736016/80128480-b1267200-85d0-11ea-8035-ad68ae5fbcab.png"> <img width="261" alt="timeline-tooltip-before-fixed" src="https://user-images.githubusercontent.com/4736016/80128492-b5eb2600-85d0-11ea-9556-c48490110244.png"> The reason of this issue is that the source texts of the tooltip texts are not escaped. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? I tested manually. First, I ran a job `sc.parallelize(1 to 10).collect` in Spark Shell then, visited AllJobsPage and JobPage and confirmed tooltip texts. <img width="196" alt="dag-viz-tooltip-fixed" src="https://user-images.githubusercontent.com/4736016/80128813-2db95080-85d1-11ea-82f8-90a1f4547f30.png"> <img width="363" alt="timeline-tooltip-fixed" src="https://user-images.githubusercontent.com/4736016/80128824-31e56e00-85d1-11ea-9818-492b72b1c56e.png"> I also added a testcase. Closes apache#28317 from sarutak/fix-tooltip. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Kousuke Saruta <sarutak@oss.nttdata.com>

gengliangwang · 2020-04-27T08:21:45Z

Late LGTM, nice catch!

### What changes were proposed in this pull request? This PR backports #28317 which escapes text for tooltip for DAG Viz and Timeline View. ### Why are the changes needed? This is a bug. Normally, DAG Viz and Timeline View show tooltip like as follows. <img width="278" alt="dag-viz-tooltip" src="https://user-images.githubusercontent.com/4736016/80127481-5a6c6880-85cf-11ea-8daf-cfd59aa3ba09.png"> <img width="477" alt="timeline-tooltip" src="https://user-images.githubusercontent.com/4736016/80127500-60624980-85cf-11ea-9b0f-cce301019e3a.png"> They contain a callsite properly. However, if a callsite contains characters which should be escaped for HTML without escaping , the corresponding tooltips wouldn't show the callsite and its following text properly. <img width="179" alt="dag-viz-tooltip-before-fixed" src="https://user-images.githubusercontent.com/4736016/80128480-b1267200-85d0-11ea-8035-ad68ae5fbcab.png"> <img width="261" alt="timeline-tooltip-before-fixed" src="https://user-images.githubusercontent.com/4736016/80128492-b5eb2600-85d0-11ea-9556-c48490110244.png"> The reason of this issue is that the source texts of the tooltip texts are not escaped. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? I tested manually. First, I ran a job `sc.parallelize(1 to 10).collect` in Spark Shell then, visited AllJobsPage and JobPage and confirmed tooltip texts. <img width="277" alt="timeline-tooltip-fixed-3 0" src="https://user-images.githubusercontent.com/4736016/80332616-41411180-8886-11ea-8d93-28e1c5265115.png"> <img width="191" alt="dag-tooltip-fixed-3 0" src="https://user-images.githubusercontent.com/4736016/80332625-44d49880-8886-11ea-8f2e-de8df1369e62.png"> And I confirmed that the appearance of the label of DAG-viz in StagePage is not changed. <img width="313" alt="stage-page-dag" src="https://user-images.githubusercontent.com/4736016/80332725-9b41d700-8886-11ea-9adb-40d50ad29f86.png"> I also added a testcase. With this testcase, an error message related to JavaScript is shown. `TypeError: Cannot call method "indexOf" of undefined (http://192.168.1.209:59376/static/spark-dag-viz.js#378)` This is thrown from `interpretLineBreak` in `spark-dag-viz.js`. HtmlUnit seems not to support `innerHTML` for text content (I tried replacing it with `textContent` and the error message is not shown). But DOMs which is needed by the testcase added is already rendered before `interpretLineBreak` is called. So the testcase successfully passes. Closes #28359 from sarutak/fix-tooltip-branch-3.0. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

sarutak added 2 commits April 23, 2020 23:16

Escaped text for tooltip.

a40056e

Added a testcase.

e01d458

probot-autolabeler bot added CORE WEB UI labels Apr 23, 2020

dongjoon-hyun reviewed Apr 23, 2020

View reviewed changes

Fixed node label for StagePage.

d3690bd

HyukjinKwon removed the CORE label Apr 24, 2020

Fixed a testcase.

3b8b071

probot-autolabeler bot added the CORE label Apr 24, 2020

dongjoon-hyun reviewed Apr 24, 2020

View reviewed changes

srowen approved these changes Apr 25, 2020

View reviewed changes

dongjoon-hyun approved these changes Apr 26, 2020

View reviewed changes

sarutak closed this in d61c621 Apr 26, 2020

sarutak mentioned this pull request Apr 26, 2020

[SPARK-31565][WEBUI] Unify the font color of label among all DAG-viz #28352

Closed

sarutak mentioned this pull request Apr 27, 2020

[SPARK-31534][WEBUI][3.0] Text for tooltip should be escaped #28359

Closed

[SPARK-31534][WEBUI] Text for tooltip should be escaped. #28317

[SPARK-31534][WEBUI] Text for tooltip should be escaped. #28317

Uh oh!

Conversation

sarutak commented Apr 23, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

dongjoon-hyun commented Apr 23, 2020

Uh oh!

dongjoon-hyun Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

sarutak Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

sarutak Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

sarutak Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 23, 2020

Uh oh!

SparkQA commented Apr 24, 2020

Uh oh!

sarutak commented Apr 24, 2020

Uh oh!

SparkQA commented Apr 24, 2020

Uh oh!

sarutak commented Apr 24, 2020

Uh oh!

SparkQA commented Apr 24, 2020

Uh oh!

SparkQA commented Apr 24, 2020

Uh oh!

dongjoon-hyun commented Apr 24, 2020

Uh oh!

dongjoon-hyun Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 24, 2020

Uh oh!

sarutak commented Apr 24, 2020

Uh oh!

SparkQA commented Apr 24, 2020

Uh oh!

sarutak commented Apr 25, 2020

Uh oh!

dongjoon-hyun commented Apr 25, 2020

Uh oh!

SparkQA commented Apr 25, 2020

Uh oh!

dongjoon-hyun commented Apr 25, 2020

Uh oh!

sarutak commented Apr 25, 2020

Uh oh!

SparkQA commented Apr 25, 2020

Uh oh!

dongjoon-hyun commented Apr 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sarutak commented Apr 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Apr 26, 2020

Uh oh!

dongjoon-hyun commented Apr 26, 2020

Uh oh!

dongjoon-hyun commented Apr 26, 2020

Uh oh!

sarutak commented Apr 26, 2020

dongjoon-hyun commented Apr 25, 2020 •

edited

Loading

sarutak commented Apr 26, 2020 •

edited

Loading