In debug mode, commented out script tags are still being run #787

samreid · 2019-08-12T15:15:51Z

Steps to reproduce:

Build Energy Skate Park
Run the debug version, like http://localhost/energy-skate-park/build/phet/energy-skate-park_all_phet_debug.html

Observe an error like so:

query.yahooapis.com/v1/public/yql?q=use%22http://davidbau.com/encode/srandom.xml%22;select*from%20srandom%20where%20nbytes%3D@nbytes;&format=json&diagnostics=false&callback=Math.seedrandom&nbytes=512:1 Failed to load resource: net::ERR_NAME_NOT_RESOLVED
urandom:1 Failed to load resource: the server responded with a status of 503 ()

seedrandom is putting this code in the HTML:

//   <script src="http://bit.ly/srandom-512"></script>
//                             Seeds using physical random bits downloaded
//                             from random.org.
//
//   <script src="https://jsonlib.appspot.com/urandom?callback=Math.seedrandom">
//   </script>                 Seeds using urandom bits from call.jsonlib.com,
//                             which is faster than random.org.

And somehow those scripts are running. The bit.ly script redirects to :

view-source:https://query.yahooapis.com/v1/public/yql?q=use%22http://davidbau.com/encode/srandom.xml%22;select*from%20srandom%20where%20nbytes%3D@nbytes;&format=json&diagnostics=false&callback=Math.seedrandom&nbytes=512

jessegreenberg · 2019-08-12T15:24:24Z

Here is the script that is being loaded, can see it is no longer commented out in the source. Text coloring makes it look like it is respecting the closing script tag on line 34465.

SaurabhTotey · 2019-08-12T15:45:46Z

Is that a JS file or an HTML file? May it be that HTML comments use  instead of //?

samreid · 2019-08-12T15:50:14Z

Here's a simplified HTML example:

<head></head>
<body>
<script type="application/javascript">
  console.log( 'hello' );
  // Usage:
  //   <script src=http://davidbau.com/encode/seedrandom-min.js></script>
console.log('bye');
</script>

</body>

samreid · 2019-08-12T15:54:58Z

This problem seems to be noted in the top answer in https://stackoverflow.com/questions/28259389/how-to-put-script-in-a-javascript-string

jessegreenberg · 2019-08-12T15:59:22Z

Nice, replacing </script> with <\/script> as recommended in that issue makes the problem go away.

jessegreenberg · 2019-08-12T16:14:17Z

So we could escape for these lines manually. But is there a way that the build could insert escapes if we find a closing tag within a javascript comment?

jonathanolson · 2019-08-12T16:28:15Z

The minification process does escape these, we provide inline_script: true to uglify's output options. Maybe we could consider minification for debug builds (but with beautified output and without stripping assertions or mangling)?

samreid · 2019-09-09T14:30:55Z

Tagging for developer meeting to decide how to proceed.

Denz1994 · 2019-09-19T18:47:10Z

Discussed at dev meeting: 09/19/19

We will proceed with this comment here will handle escaping, but would get rid of line comments.

If debug builds are used just for assertions then having comments may be helpful. We can use stack trace to track relevant code to the assertion or search for code snippet globally in code.

@Denz1994 is assigned and will reference @jonathanolson or @samreid if needed.

jessegreenberg · 2020-10-29T15:49:37Z

This came up again in phetsims/sherpa#80 during an RC of ESP. Adding the blocks-publication label.

pixelzoom · 2020-10-30T15:40:41Z

I'm assuming that this does not block publication of the dev test for Natural Selection, for phetsims/natural-selection#251. @ariel-phet please correct me asap if that's incorrect.

…t> tags in html comment blocks. See #787.

Denz1994 · 2020-11-02T15:46:10Z

The debugMinifyOptions in buildRunnable.js was adjusted to minify and beautify the debug builds as suggested above. Assertions are still included in these builds. I tested a few debug builds in energy-skate-park, energy-skate-park-basics, and fluid-and-pressure-flow. The errors have ceased in the console, as described in phetsims/sherpa#80. Assigning to @jonathanolson for a final review.

samreid · 2020-11-02T22:50:24Z

I compared the build version before and after the changes, it noticed that after the changes, the debug version is missing all of the code comments.

Is there a way to run uglify so that it only runs "inline_script"?

This reverts commit 65b7959, see phetsims/gravity-and-orbits#345

jessegreenberg · 2020-11-10T22:31:56Z

Thanks @KatieWoe - I discussed this issue with @jonathanolson over slack and he reviewed saying that the changes are safe to proceed with for a publication so I am cherry-picking them to the energy-skate-park-1.1 branch of chipper. Also removing the blocks publication label.

…t> tags in html comment blocks. See #787.

jessegreenberg · 2020-11-10T22:35:13Z

Over slack @jonathanolson said that the changes are safe, but if people are relying on comments in debug files then we should look into restoring them. Adding to developer meeting to see if anyone is relying on comments in the debug build.

KatieWoe · 2020-11-12T21:25:35Z

I am not seeing these errors in ESP rc3 anymore.

samreid · 2020-11-19T20:28:31Z

@zepumph: It's not important to me that the code comments are in the debug version.
@Denz1994: It's more important that the assertions are not stripped out
@samreid: Preferably the code comments would remain in.
@zepumph: Yes, but it is rare to look at a debug version for context--we try to move to master quickly to reproduce the problem
@chrisklus: I agree
@jonathanolson: But what about problems where a PhET-iO client produces a problem where we cannot easily replicate the problem?
@samreid: I think the debug version should be used for more than just assertions--would be nice to allow users to take a look around and see comments in the debug file.. Why not just remove this bad line from numeric-1.2.6.js?
@zepumph: It would be even better if we could auto detect this kind of problem. We could reach out to that developer and ask if they could remove it?

@samreid: I'll see if there's a new version without that.

samreid · 2020-11-23T12:43:31Z

@zepumph: We could reach out to that developer and ask if they could remove it?
@samreid: I'll see if there's a new version without that.

numeric.js has not been maintained since 2012.

samreid · 2020-11-23T13:45:21Z

I tried putting output:{comments:'all'}, but it increased the production minify time from

Debug minification complete: 13309ms (4082787 bytes)

to

Debug minification complete: 73311ms (6822163 bytes)

which is too long, in my opinion. Also, the formatting is not very readable:

This reverts commit 65b7959

… </script> tags in html comment blocks. See #787." This reverts commit cb2e957

…#787

samreid · 2020-11-23T14:08:04Z

I manually escaped the closing tags in the comments of numeric-1.2.6.js, and reverted the changes to the minification process. I noticed this speeds up the builds by about 13 seconds.

Before:

Debug minification complete: 13309ms (4082787 bytes)

After:

Debug minification complete: 0ms (6721183 bytes)

This also fixes the formatting:

And there are no HTTP requests when starting the debug version of energy skate park. The main part this solution is missing is how to prevent this kind of problem from being reintroduced. Some ideas for that:

(a) (lazy, low effort) wait until we see this problem again
(b) (manual effort) add a manual step to the RC process where you launch the debug version and check the console. This could be done once per version, once per month, once per year depending on our tolerance for this problem.
(c) (eager, automated) add an automatic step to the RC process which automatically launches the sim and checks the console. This would take a few hours to build with puppeteer, and would slow down builds by a few seconds.
(d) (eager, automated) add a precommit hook that prevents us from committing this kind of problem again (our default lint rules don't work on 3rd party code)

@Denz1994 can you please review and comment?

KatieWoe · 2020-11-23T16:22:55Z

Noting that option b is already (basically) standard for at least one rc in a testing round. I run the debug sim with a fuzz test and the console open for ~10 minutes while testing something else on another device, so that I can catch assertions that might happen.

samreid · 2021-01-29T04:19:21Z

Since (b) is basically in place, I think we can close this issue. Thanks!

samreid assigned samreid and jessegreenberg Aug 12, 2019

samreid added the deprecated:meeting:developer label Sep 9, 2019

samreid removed their assignment Sep 9, 2019

Denz1994 assigned Denz1994 and unassigned jessegreenberg Sep 19, 2019

Denz1994 removed the deprecated:meeting:developer label Sep 19, 2019

jessegreenberg mentioned this issue Mar 12, 2020

Built version doesn't load on Safari phetsims/buoyancy#4

Closed

jessegreenberg mentioned this issue Oct 29, 2020

numericjs causing error in all_phet_debug builds phetsims/sherpa#80

Closed

jessegreenberg added the status:blocks-publication label Oct 29, 2020

Denz1994 added a commit that referenced this issue Nov 2, 2020

Minify and beautify debug builds to support handling escaping </scrip…

cb2e957

…t> tags in html comment blocks. See #787.

Denz1994 added a commit that referenced this issue Nov 2, 2020

Avoid stripping assertions in debug builds. See #787.

65b7959

Denz1994 assigned jonathanolson and unassigned Denz1994 Nov 2, 2020

samreid added the status:ready-for-review label Nov 2, 2020

samreid mentioned this issue Nov 2, 2020

publish PhET-iO dev version for the client phetsims/gravity-and-orbits#345

Closed

8 tasks

samreid added a commit that referenced this issue Nov 2, 2020

Revert "Avoid stripping assertions in debug builds. See #787."

226b0e2

This reverts commit 65b7959, see phetsims/gravity-and-orbits#345

jessegreenberg removed the status:blocks-publication label Nov 10, 2020

jessegreenberg pushed a commit that referenced this issue Nov 10, 2020

Minify and beautify debug builds to support handling escaping </scrip…

c7038bd

…t> tags in html comment blocks. See #787.

jessegreenberg pushed a commit that referenced this issue Nov 10, 2020

Avoid stripping assertions in debug builds. See #787.

ddd8f65

jessegreenberg added deprecated:meeting:developer and removed status:ready-for-review labels Nov 10, 2020

jessegreenberg unassigned jonathanolson and jessegreenberg Nov 10, 2020

jessegreenberg mentioned this issue Nov 10, 2020

RC Spot Check: Energy Skate Park 1.1.0-rc.3 phetsims/qa#578

Closed

2 tasks

KatieWoe assigned jessegreenberg Nov 12, 2020

jessegreenberg removed their assignment Nov 19, 2020

samreid self-assigned this Nov 20, 2020

samreid removed the deprecated:meeting:developer label Nov 20, 2020

samreid added a commit that referenced this issue Nov 23, 2020

Revert "Avoid stripping assertions in debug builds. See #787."

55bb700

This reverts commit 65b7959

samreid added a commit that referenced this issue Nov 23, 2020

Revert "Minify and beautify debug builds to support handling escaping…

1d825f3

… </script> tags in html comment blocks. See #787." This reverts commit cb2e957

samreid added a commit to phetsims/sherpa that referenced this issue Nov 23, 2020

Manually escape closing script tags in comments, see phetsims/chipper…

2381af0

…#787

samreid assigned Denz1994 and unassigned samreid Nov 23, 2020

samreid assigned samreid and unassigned Denz1994 Jan 28, 2021

samreid closed this as completed Jan 29, 2021

samreid mentioned this issue Aug 10, 2021

Suppress Three.js output phetsims/density-buoyancy-common#21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In debug mode, commented out script tags are still being run #787

In debug mode, commented out script tags are still being run #787

samreid commented Aug 12, 2019

jessegreenberg commented Aug 12, 2019

SaurabhTotey commented Aug 12, 2019

samreid commented Aug 12, 2019

samreid commented Aug 12, 2019

jessegreenberg commented Aug 12, 2019

jessegreenberg commented Aug 12, 2019

jonathanolson commented Aug 12, 2019

samreid commented Sep 9, 2019

Denz1994 commented Sep 19, 2019 •

edited

Loading

jessegreenberg commented Oct 29, 2020 •

edited

Loading

pixelzoom commented Oct 30, 2020

Denz1994 commented Nov 2, 2020

samreid commented Nov 2, 2020 •

edited

Loading

jessegreenberg commented Nov 10, 2020

jessegreenberg commented Nov 10, 2020

KatieWoe commented Nov 12, 2020

samreid commented Nov 19, 2020 •

edited

Loading

samreid commented Nov 23, 2020

samreid commented Nov 23, 2020

samreid commented Nov 23, 2020

KatieWoe commented Nov 23, 2020

samreid commented Jan 29, 2021

In debug mode, commented out script tags are still being run #787

In debug mode, commented out script tags are still being run #787

Comments

samreid commented Aug 12, 2019

jessegreenberg commented Aug 12, 2019

SaurabhTotey commented Aug 12, 2019

samreid commented Aug 12, 2019

samreid commented Aug 12, 2019

jessegreenberg commented Aug 12, 2019

jessegreenberg commented Aug 12, 2019

jonathanolson commented Aug 12, 2019

samreid commented Sep 9, 2019

Denz1994 commented Sep 19, 2019 • edited Loading

jessegreenberg commented Oct 29, 2020 • edited Loading

pixelzoom commented Oct 30, 2020

Denz1994 commented Nov 2, 2020

samreid commented Nov 2, 2020 • edited Loading

jessegreenberg commented Nov 10, 2020

jessegreenberg commented Nov 10, 2020

KatieWoe commented Nov 12, 2020

samreid commented Nov 19, 2020 • edited Loading

samreid commented Nov 23, 2020

samreid commented Nov 23, 2020

samreid commented Nov 23, 2020

KatieWoe commented Nov 23, 2020

samreid commented Jan 29, 2021

Denz1994 commented Sep 19, 2019 •

edited

Loading

jessegreenberg commented Oct 29, 2020 •

edited

Loading

samreid commented Nov 2, 2020 •

edited

Loading

samreid commented Nov 19, 2020 •

edited

Loading