fix loop protection breaking shader strings and p5.strands functions by Nixxx19 · Pull Request #4083 · processing/p5.js-web-editor

Nixxx19 · 2026-04-15T11:25:52Z

issue:

fixes #4080

demo:

changes:

replaced regex-based loop protection with acorn ast-based approach in a new jspreprocess.js module
strings are never visited by the ast parser, so glsl for loops inside template literals and quoted strings are no longer modified
added two-pass detection for p5.strands shader functions: first pass collects function names passed to build*shader() and base*shader().modify(), second pass skips loop protection inside those functions
removed loop-protect and decomment dependencies from embedframe.jsx
added 13 unit tests covering all cases

i have verified that this pull request:

has no linting errors (npm run lint)
has no test errors (npm run test)
has no typecheck errors (npm run typecheck)
is from a uniquely-named feature branch and is up to date with the develop branch.
is descriptively named and links to an issue number, i.e. fixes #4080
meets the standards outlined in the accessibility guidelines

davepagurek

Great work on this! Consider this review nonblocking, just sharing some thoughts for you and @raclim to consider.

davepagurek · 2026-04-15T12:05:10Z

+    callee.property.name === 'modify' &&
+    callee.object.type === 'CallExpression' &&
+    callee.object.callee.type === 'Identifier' &&
+    /^base\w*Shader$/.test(callee.object.callee.name);


Might be worth noting that this might miss some things, e.g. if a plugin author exposes a shader of their own that has its own name or is just a variable and not a function. If we just check that it's a MemberExpression calling a property called modify that would capture all that, but may have some false positives if users have a method called modify in non-p5.strands code. I think either could be a reasonable compromise, @raclim let me know if you have any thoughts on which side to err on!

good point, thank you! updated it to match any .modify() call instead of just base*Shader().modify(), which should cover plugin-defined shaders too. there could be false positives if someone has an unrelated .modify() call, but that feels like the safer tradeoff. happy to hear your thoughts on this.

I think @davepagurek's suggestion sounds good to me! I think we can keep the check broad for now, and refine it later down the line if needed.

thank you both for the input! will keep it broad for now and revisit if false positives come up.

davepagurek · 2026-04-15T12:07:04Z

+  const loops = [];
+
+  function visitNode(node, ancestors) {
+    const isInsideShader = ancestors.some((ancestor, idx) => {


nice, looks good!

davepagurek · 2026-04-15T12:10:10Z

+  try {
+    ast = acorn.parse(jsText, {
+      ecmaVersion: 'latest',
+      sourceType: 'script',


Maybe worth considering that users could use <script type="module"> scripts, e.g. https://editor.p5js.org/davepagurek/sketches/-foIv7eQa -- worth testing to see if parsing fails in those if you use sourceType: 'script' and you have an import statement, or if we'd need to pass in the source type based on how the script tag imports it.

thank you for catching this! added a fallback that tries sourceType: 'script' first, and if that throws (e.g. for a module script with import statements), it retries with sourceType: 'module'. added a test for this case too.

davepagurek · 2026-04-15T12:15:52Z

+  const insertions = [];
+
+  loops.forEach((loop) => {
+    const { line } = loop.loc.start;


Possibly worth considering whether it's preferable to still do line-based insertions like this, which may have issues with multiple loops on the same line like what's implied by the comment in the current implementation:

p5.js-web-editor/client/modules/Preview/EmbedFrame.jsx

Lines 66 to 69 in 37bc396

// Detect and fix multiple consecutive loops on the same line (e.g. "for(){}for(){}")

// which can bypass loop protection. Add semicolons between them so each loop

// is properly wrapped by loopProtect. See #3891.

// Match: for/while/do-while loops followed immediately by another loop

...or if instead it would be better to make changes at the AST level using Acorn by modifying the AST nodes, and then use a tool like escodegen to re-emit the full source code from that. (This is what we do in p5.strands after transpiling shader functions: https://github.com/processing/p5.js/blob/9498619e7b31f1b3a4e0fda5fcdc19d38e7eff20/src/strands/strands_transpiler.js#L1772) The downside of this approach is that escodegen doesn't preserve everything about your source code like whitespace and semicolon usage, so it might look a little different if you put a debugger statement and look at the code actually being run by the browser. Also it would mean adding another library, not sure how much that adds to the bundle size. But it would mean more targeted insertions and less likelihood for it happening incorrectly.

thank you for the suggestion! switched to the escodegen approach. we now modify the ast nodes directly and regenerate the source using escodegen.generate(). added escodegen as a dependency. the tradeoff on whitespace/formatting is real but feels worth it for the correctness guarantees. please let me know if the bundle size is a concern and we can fall back to character-offset string insertions instead, which also handles multiple loops on the same line correctly since we apply them back-to-front.

davepagurek · 2026-04-15T17:20:51Z

+  });
+
+  walk.simple(ast, {
+    BlockStatement(node) {


Do you think we can simplify this step and modify the nodes in place in the earlier pass where we collect the loops to modify? Then we wouldn't need to find them again with another pass here? Or correct me if there's something that that would miss!

i chose the two-pass approach initially to keep the collection and injection steps separate, but you are right that since we already have the ancestors available in the walk, we can do it all in one pass. simplified it to a single walk.ancestor call that both checks for shader functions and injects the protection in place. updated in the latest commit. thank you for the suggestion!

raclim · 2026-04-15T17:48:03Z

+  walk.ancestor(ast, {
+    ForStatement: visitNode,
+    WhileStatement: visitNode,
+    DoWhileStatement: visitNode


Is there anything else(i.e for...of loops) we might want to cover here? Also okay with leaving it as is!

for...of and for...in loops are parsed as separate ast node types (ForOfStatement and ForInStatement), so they are not currently covered. happy to add support for both if you think that would be a good addition!

Nixxx19 · 2026-04-16T21:38:44Z

added --forceExit to the jest config to fix the ci hang — escodegen keeps an open handle after tests finish which prevented jest from exiting cleanly

davepagurek · 2026-04-16T21:46:52Z

Seems suspicious that escodegen, which is I think a synchronous function call, would hang the tests. Is there the potential that the AST has a loop in the data after it gets processed? that could potentially cause it to infinite loop while generating code.

Nixxx19 · 2026-04-16T21:55:53Z

Seems suspicious that escodegen, which is I think a synchronous function call, would hang the tests. Is there the potential that the AST has a loop in the data after it gets processed? that could potentially cause it to infinite loop while generating code.

yeah i was thinking along the same lines, looking into the codebase right now to trace through what might actually be happening. will report back once i have a clearer picture of the issue

Nixxx19 · 2026-04-16T22:03:02Z

@davepagurek, the infinite loop got introduced when we moved to the single-pass approach where modifications happened inline during the walk. walk.ancestor iterates each block's body with a plain for loop, so inserting a var declaration before the current loop made the walker's index bump forward and land on the same loop again, revisiting it endlessly.

the walk now collects each loop along with its parent block from the ancestors, and all modifications are applied after the walk completes so we never mutate arrays the walker is still iterating. verified locally on single, nested, sibling, and in-function loop cases. pushing the fix now.

…mpletes

Nixxx19 · 2026-04-16T22:08:28Z

works now, finally phew!

raclim · 2026-04-16T23:27:37Z

    "build:server": "cross-env NODE_ENV=production webpack --config webpack/config.server.js",
    "build:examples": "cross-env NODE_ENV=production webpack --config webpack/config.examples.js",
-    "test": "NODE_ENV=test jest",
+    "test": "NODE_ENV=test jest --forceExit",


Since it seems like the issue with running tests got resolved, could we remove the --forceExit?

yes, removed it!

Nixxx19 · 2026-04-17T07:10:21Z

good to go from my side now! all feedback addressed and ci is green. thanks for the thorough review @davepagurek @raclim!

raclim

Thanks for the quick turnaround on this! I think this looks good to me!

Nixxx19 · 2026-04-18T17:40:48Z

Thanks for the quick turnaround on this! I think this looks good to me!

thanks for the review, happy to get this merged!

Nixxx19 added 2 commits April 15, 2026 16:55

fix loop protection breaking shader strings and p5.strands functions

6273dca

Merge branch 'develop' into nityam/fix-loop-protect-shader-strings-4080

78eee42

Nixxx19 mentioned this pull request Apr 15, 2026

Loop protection breaks shaders #4080

Closed

davepagurek reviewed Apr 15, 2026

View reviewed changes

Nixxx19 added 2 commits April 15, 2026 22:25

address review feedback on loop protection

cc3ef43

Merge branch 'develop' into nityam/fix-loop-protect-shader-strings-4080

a0b8d98

davepagurek reviewed Apr 15, 2026

View reviewed changes

raclim reviewed Apr 15, 2026

View reviewed changes

simplify loop protection to single ast pass

a1872ad

Nixxx19 requested review from davepagurek and raclim April 16, 2026 15:21

raclim and others added 2 commits April 16, 2026 14:45

Merge branch 'develop' into nityam/fix-loop-protect-shader-strings-4080

e967dfc

add forceExit to jest to prevent ci hang

47b6bf2

fix infinite loop in ast walk by applying modifications after walk co…

bb853d3

…mpletes

raclim reviewed Apr 16, 2026

View reviewed changes

remove forceExit flag now that infinite loop is fixed

7cc69ea

raclim approved these changes Apr 17, 2026

View reviewed changes

raclim merged commit c5cdecb into processing:develop Apr 20, 2026
1 check passed

	// Detect and fix multiple consecutive loops on the same line (e.g. "for(){}for(){}")
	// which can bypass loop protection. Add semicolons between them so each loop
	// is properly wrapped by loopProtect. See #3891.
	// Match: for/while/do-while loops followed immediately by another loop

Uh oh!

Conversation

Nixxx19 commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

issue:

demo:

changes:

i have verified that this pull request:

Uh oh!

davepagurek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nixxx19 Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nixxx19 Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nixxx19 Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nixxx19 Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nixxx19 Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nixxx19 commented Apr 16, 2026

Uh oh!

davepagurek commented Apr 16, 2026

Uh oh!

Nixxx19 commented Apr 16, 2026

Uh oh!

Nixxx19 commented Apr 16, 2026

Uh oh!

Nixxx19 commented Apr 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nixxx19 commented Apr 17, 2026

Uh oh!

raclim left a comment

Choose a reason for hiding this comment

Uh oh!

Nixxx19 commented Apr 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Nixxx19 commented Apr 15, 2026 •

edited

Loading

Nixxx19 Apr 15, 2026 •

edited

Loading

Nixxx19 Apr 15, 2026 •

edited

Loading

Nixxx19 Apr 15, 2026 •

edited

Loading

Nixxx19 Apr 15, 2026 •

edited

Loading

Nixxx19 Apr 15, 2026 •

edited

Loading