Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix dynamic callback patcher: make things more versatile #640

Open
4 of 9 tasks
Domiii opened this issue Jan 5, 2022 · 0 comments
Open
4 of 9 tasks

fix dynamic callback patcher: make things more versatile #640

Domiii opened this issue Jan 5, 2022 · 0 comments
Assignees
Labels
async bug Something isn't working projects Results of Dbux running on a specific codebase or package.

Comments

@Domiii
Copy link
Owner

Domiii commented Jan 5, 2022

  • 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 GlobalViewPatched_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
  • 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:
    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

  • fix: react-tic-tac-toe does not render if CallbackPatcher is Enabled.
  • fix addEventListener -> removeEventListener callback identity
@Domiii Domiii added bug Something isn't working priority labels Jan 5, 2022
@Domiii Domiii self-assigned this Jan 5, 2022
@Domiii Domiii changed the title Dynamic callback patcher: tracing react (and others) is currently broken Dynamic callback patcher: tracing react (and others) breaks them Jan 9, 2022
Domiii added a commit that referenced this issue Jan 11, 2022
@Domiii Domiii changed the title Dynamic callback patcher: tracing react (and others) breaks them fix dynamic callback patcher: use Proxy instead of function Jan 16, 2022
@Domiii Domiii changed the title fix dynamic callback patcher: use Proxy instead of function fix dynamic callback patcher: make things more versatile Feb 20, 2022
@Domiii Domiii added async projects Results of Dbux running on a specific codebase or package. and removed priority labels Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async bug Something isn't working projects Results of Dbux running on a specific codebase or package.
Projects
None yet
Development

No branches or pull requests

1 participant