Domiii / Domiii/dbux

fix dynamic callback patcher: make things more versatile

Open
#640 0 comments 0 reactions 1 assignee Claimed by @Domiii View on GitHub
async bug projects
Dominant language
JavaScript
Stars
171
Forks
13
PR merge metrics
No merged PRs in 30d

Description

* [x] use `Proxy` instead of `function`, so property modification also gets taken care of correctly.
* [ ] add analysis for multi-patching of same function/callback
* → We have started adding that info to `GlobalView` → `Patched_Callbacks`
* [ ] ideally: don't nest patched functions/callbacks under any circumstances
* (e.g. we might get: `patchedCallback` -> `patchedThenCb`)
* NOTE: `patchedFunctions` are instrumented un-instrumented functions while `patchedCbs` are instrumented recorded functions. The two sets should never overlap.
* [ ] test: `javascript-algorithms` errors out
* might be caused by `jest-snapshot`
* [x] test: `require.resolve` should never be patched

## Remaining Problems

Some problems are still generally unsolved (and require some hacky workaround):

* Consumer of a function relies on equality checks (e.g. `addEventListener` vs. `removeEventListener`)
* [ ] add custom fix for `removeEventListener` and similar
E.g., using our current proxy solution, `f` does not retain identity when handled by uninstrumented code:
```js
var s = new Set();
function f() {}

var p1 = new Proxy(f, {});
var p2 = new Proxy(f, {});

s.add(p1);

console.assert(s.has(p2)); // assertion fails
```

## More Future Work

* [ ] determine heuristics to detect and avoid "instrumenting the instrumentors"
* Generally speaking, it is a bad idea to instrument the instrumenters, since that can lead to infinite loops and other bugs.
* E.g. the old problem of `graceful-fs` instrumenting `process.cwd` → which in Node@14 was used internally by Node when reading sourcemaps when generating stacktraces → which triggered Dbux recording

## Done

* [x] fix: `react-tic-tac-toe` does not render if `CallbackPatcher` is `Enabled`.
* [x] fix `addEventListener` -> `removeEventListener` callback identity

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.