Repository navigation
fix(extension): evict stale element-registry entries across analyze cycles - #92
Merged
Merged
Conversation
…ycles elementRegistry/quickRegistry (content.js) were never cleared or bounded. Content scripts persist across a client-side route change (they only reset on a full page load), and every page_analyze call added entries with nothing removing them - unbounded growth over a long session, which is exactly the shape of the long-lived SPA sessions (Twitter/X, LinkedIn, Facebook) the anti-detection bypass targets. A stale element_id surviving a route change could also silently resolve to a detached DOM node instead of a clean "not found". Fix, two parts: - quickDiscovery() (the "discover" phase, the start of a new analysis cycle) now clears both maps first. A "detailed" call within the same cycle still sees the ids a prior discover call handed out - only a new discover call resets the state. - getElementById() now treats a resolved-but-detached node (!element.isConnected) the same as a cache miss, so a stale id across a route change throws the existing "Element not found" error callers already handle, instead of handing back a dead node. No behavioral test harness exists for content.js (confirmed: no jest/ jsdom/mocha/vitest anywhere in this repo; test-extension.js only does string-presence checks against the built output, not logic). Verified the fix's logic directly instead, with a disposable zero-dependency script replicating just the two functions under test (registerElement/ getElementById/clear): live lookups still resolve, a detached node now misses, an unrelated unknown id still misses as before, and registry size stays bounded (1 entry, not 50) across 50 simulated discover cycles. Also confirmed node --check, npm run build, node build.js validate, and node test-extension.js all stay green.
…scover Clearing both registries on every discover broke multi-step flows: discover "username", discover "password", then fill the first id threw "Element not found". It also left the detailed path unbounded. Now every analyze call (discover and detailed) first drops entries whose node is no longer connected, then the oldest past 1000 per registry. Live ids from earlier calls keep resolving; detached nodes are still freed. The isConnected miss in getElementById stays. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
elementRegistry/quickRegistry in content.js never get cleared. content scripts persist across client-side route changes, so every page_analyze call adds entries with nothing removing them — unbounded growth on long sessions, which is exactly what the anti-detection bypass targets (twitter/linkedin/facebook). a stale element_id after a route change can also resolve to a detached node instead of a clean "not found."
fix:
quickDiscovery()(the start of a new analyze cycle) clears both maps firstgetElementById()treats a detached node (!element.isConnected) as a missno test harness exists for content.js (no jest/jsdom/mocha/vitest anywhere, test-extension.js only checks build output strings). verified the logic with a small disposable script instead of adding a test framework for a two-function fix — happy to add proper jsdom tests as a follow-up if wanted.
build/validate/test-extension still pass.