nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.79k stars 29.69k forks source link

Restart frame is broken #28493

Open ulitink opened 5 years ago

ulitink commented 5 years ago

Restart frame action doesn't work since node 11: RestartFrame

addaleax commented 5 years ago

I might be doing something wrong, but I can’t reproduce this on v12.5.0.

/cc @nodejs/inspector

alexkozy commented 5 years ago

Restart frame might fail for different reasons: source. @ulitink could you share content of each function on the stack (listO... and proc...)?

ulitink commented 5 years ago

@ak239 These are just built-in node functions from internal/timers.js

timers.js ``` function processTimers(now) { debug('process timer lists %d', now); nextExpiry = Infinity; let list; let ranAtLeastOneList = false; while (list = timerListQueue.peek()) { if (list.expiry > now) { nextExpiry = list.expiry; return refCount > 0 ? nextExpiry : -nextExpiry; } if (ranAtLeastOneList) runNextTicks(); else ranAtLeastOneList = true; listOnTimeout(list, now); } return 0; } function listOnTimeout(list, now) { const msecs = list.msecs; debug('timeout callback %d', msecs); var diff, timer; let ranAtLeastOneTimer = false; while (timer = L.peek(list)) { diff = now - timer._idleStart; // Check if this loop iteration is too early for the next timer. // This happens if there are more timers scheduled for later in the list. if (diff < msecs) { list.expiry = Math.max(timer._idleStart + msecs, now + 1); list.id = timerListId++; timerListQueue.percolateDown(1); debug('%d list wait because diff is %d', msecs, diff); return; } if (ranAtLeastOneTimer) runNextTicks(); else ranAtLeastOneTimer = true; // The actual logic for when a timeout happens. L.remove(timer); const asyncId = timer[async_id_symbol]; if (!timer._onTimeout) { if (timer[kRefed]) refCount--; timer[kRefed] = null; if (destroyHooksExist() && !timer._destroyed) { emitDestroy(asyncId); timer._destroyed = true; } continue; } emitBefore(asyncId, timer[trigger_async_id_symbol]); let start; if (timer._repeat) start = getLibuvNow(); try { const args = timer._timerArgs; if (args === undefined) timer._onTimeout(); else timer._onTimeout(...args); } finally { if (timer._repeat && timer._idleTimeout !== -1) { timer._idleTimeout = timer._repeat; if (start === undefined) start = getLibuvNow(); insert(timer, timer[kRefed], start); } else if (!timer._idleNext && !timer._idlePrev) { if (timer[kRefed]) refCount--; timer[kRefed] = null; if (destroyHooksExist() && !timer._destroyed) { emitDestroy(timer[async_id_symbol]); timer._destroyed = true; } } } emitAfter(asyncId); } // If `L.peek(list)` returned nothing, the list was either empty or we have // called all of the timer timeouts. // As such, we can remove the list from the object map and // the PriorityQueue. debug('%d list empty', msecs); // The current list may have been removed and recreated since the reference // to `list` was created. Make sure they're the same instance of the list // before destroying. if (list === timerListMap[msecs]) { delete timerListMap[msecs]; timerListQueue.shift(); } } ```
Same with strict call without `setInterval` ![image](https://user-images.githubusercontent.com/1112897/60445016-0e5acc80-9c27-11e9-8270-aeb9d51d1d1f.png)

We've checked it on 4 different PCs, the same exception happens Request Debugger.restartFrame failed. {"code":-32603,"message":"Internal error"}

alexkozy commented 5 years ago

It looks like it is regression between 10 and 11 and most likely I broke it when I was refactoring live edit code in V8.

alexkozy commented 5 years ago

I introduced a bug that is even worse than plus minus one kind of bug. V8 fix is coming: https://chromium-review.googlesource.com/c/v8/v8/+/1683101/

alexkozy commented 5 years ago

It is fixed in upstream V8, I will wait for couple weeks until attempt to backport.

jasnell commented 4 years ago

Is this still an issue? Does this need to remain open?