Fixed a Maximum call stack exceeded err in getAlternateSourceIdForPosition

[Andarist]

I noticed this error in the console and reported it a couple of days ago on Discord:

I couldn't repro it in Replay (it only happens for me in Chrome) - I'm not sure what exactly leads to it. However, when asked by @bvaughn for some more details about this I started to dig deeper. I was able to confirm that this happens for a minified code line that looks like this:

, i = Math.min(...o.filter((e=>e.line >= n.line)).map((e=>e.line)))

and I was able to trace it back to this in the source code:

devtools/src/devtools/client/debugger/src/utils/sourceVisualizations.ts

Lines 163 to 165 in 8772827

	const breakableLine = Math.min(
	...lineLocations.filter(ll => ll.line >= position.line).map(ll => ll.line)
	);

The problem with fn(...arr) is that it pushes all array elements onto the stack and that can blow up the stack. Usually, this is not an issue but it turns out that o is quite huge here:

Note that on this screenshot we see both the original o and the result of the filter+map - we can notice that their lengths are the same. This might indicate a bug - but I don't know what exactly those code lines are meant to do so I'm not sure.

In case this isn't a bug on its own... I've prepared this simple fix that just doesn't rely on the spread.

You can also test the "issue" with spread by evaluating this in the Chrome's console:

var huge = Array.from({ length: 150000 }, (_, i) => i)
console.log(Math.min(...huge)) // throws Maximum call stack exceeded

Note that FF has a bigger allowed stack and it might be the reason why it doesn't happen in Replay. I've recorded the situation in Replay and added a comment to the call site receiving this huge array, you might find it helpful: https://app.replay.io/recording/max-call-stack-exceeded-in-replay-just-in-chrome--018c0ea7-1ffe-446e-8020-e7e45f14d228

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
devtools	✅ Ready (Inspect)	Visit Preview	Sep 20, 2022 at 9:48PM (UTC)

[Andarist]

this matches Math.min(...[])

[replay-io]

52 replays were recorded for 55f1566.

2 Failed

• logpoint-01.js
• doc_exceptions_bundle.html

50 Passed

• console_warp-02.js
• breakpoints-07.js
• object_preview-03.js
• scopes_rerender.js
• react_devtools.js
• node_console-02.js
• sourcemap_stacktrace.js
• console_messages.js
• repaint.js
• stepping-03.js
• node_worker-01.js
• highlighter.js
• breakpoints-02.js
• object_preview-05.js
• stepping-04.js
• inspector-06.js
• inspector-02.js
• logpoint-04.js
• inspector-07.js
• console_async_eval.js
• inspector-04.js
• console_eval.js
• node_logpoint-01.js
• logpoint-03.js
• node_console-01.js
• logpoint-02.js
• object_preview-02.js
• breakpoints-06.js
• region_loading-01.js
• node_object_preview-01.js
• settings.js
• node_spawn-01.js
• stepping-05.js
• breakpoints-05.js
• logpoint-05.js
• node_control_flow.js
• node_logpoint-02.js
• breakpoints-03.js
• autocomplete.js
• inspector-03.js
• object_preview-01.js
• worker-01.js
• node_stepping-01.js
• inspector-05.js
• stepping-06.js
• stacking.js
• stepping-01.js
• breakpoints-04.js
• stepping-02.js
• breakpoints-01.js

View test run on Replay ↗︎

[markerikson]

I like this as a temporary workaround and we can merge it now, but I think we should come up with an alternate implementation. I already made a couple tweaks to the "breakable positions" handling that use a normalized lookup table by line instead of searching a giant array in order to fix a couple perf hotspots, and this looks like another good candidate.

[Uzlopak]

Why not

function minOptimized(arr) {
  const length = arr.length;
  if (length === 0) {
    return Infinity;
  } else if (length === 1) {
  	return arr[0]
  } else {
   	let lowest = arr[0];
 	for (let i = 1; i < length; ++i) {
    	(arr[i] < lowest) && (lowest = arr[i]);
	    if (lowest === 1) {
    		break
    	}
	  }
  	return lowest;
  }
}

[Andarist]

Because this one is less readable, it looks like the microoptimization. It's also not correct:

minOptimized([10, 1, -1]) // actual 1, expected -1

It could be thought of as correct for this very specific use case, but I didn't bother to overoptimize this for this case here (could be done though)

[Uzlopak]

You can have negative lines in a file?

[Andarist]

No, that’s why i’ve said that it would be OK here but i was just trying to replace the current implementation with the one that wouldnt blow up the stack. So i’ve wanted to match the previous code 1 to 1 and just went for a generic implementation for the min

[Uzlopak]

Yeah you are right. Probably the best optimization would be

  let breakableLine = Infinity
  const positionLine = position.line
  for (let i = 0, il = lineLocations.length; i < il; ++i) {
    const line = lineLocations[i].line
    if (
      line >= positionLine &&
      line < breakableLine
    ) {
      breakableLine = line
    }
  }

[markerikson]

Yeah, per comment above, I think the best improvement here is to already have a { [lineNumber] : columns[] } lookup table so we don't have to do a .filter().map(), etc.

[bvaughn]

I don't have a ton of context here but I agree that we should avoid iterating over big arrays– especially in code that's hot. Looking forward to seeing what Mark has in mind 😁