Skip to content

refactor: Enhance ContentBlocker with improved error handling, memory management, and session management #149

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: Enhance ContentBlocker with improved error handling, memory management, and session management #149

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
227 changes: 174 additions & 53 deletions src/main/modules/content-blocker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,21 @@ type BlockerInstanceType = "all" | "adsAndTrackers" | "adsOnly";

const SESSION_KEY = "content-blocker";

interface BlockerConfig {
type: BlockerInstanceType;
enabled: boolean;
}

/**
* ContentBlocker class manages ad and tracking content blocking functionality
* with improved memory management, error handling, and performance optimizations
*/
class ContentBlocker {
private blockerInstancePromise: Promise<ElectronBlocker> | undefined = undefined;
private blockerInstanceType: BlockerInstanceType | undefined = undefined;
private blockedSessions: Session[] = [];
private blockedSessions = new Set<Session>();
private isInitialized = false;
private updateTimeout: NodeJS.Timeout | undefined;

/**
* Creates or returns existing blocker instance of the specified type
Expand All @@ -30,26 +38,37 @@ class ContentBlocker {
}

debugPrint("CONTENT_BLOCKER", "Creating blocker instance:", type);
switch (type) {
case "all":
this.blockerInstancePromise = ElectronBlocker.fromPrebuiltFull();
break;
case "adsAndTrackers":
this.blockerInstancePromise = ElectronBlocker.fromPrebuiltAdsAndTracking();
break;
case "adsOnly":
this.blockerInstancePromise = ElectronBlocker.fromPrebuiltAdsOnly();
break;
}

this.blockerInstancePromise.then((blocker) => {
try {
let promise: Promise<ElectronBlocker>;

switch (type) {
case "all":
promise = ElectronBlocker.fromPrebuiltFull();
break;
case "adsAndTrackers":
promise = ElectronBlocker.fromPrebuiltAdsAndTracking();
break;
case "adsOnly":
promise = ElectronBlocker.fromPrebuiltAdsOnly();
break;
}

this.blockerInstancePromise = promise;
this.blockerInstanceType = type;

const blocker = await promise;
blocker.on("request-blocked", (request) => {
debugPrint("CONTENT_BLOCKER", "Request blocked:", request.url);
});
Comment on lines +60 to 63
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential memory leak: Event listeners not cleaned up

The event listener is attached to the blocker instance but never removed. If createBlockerInstance is called multiple times (e.g., when switching blocker types), old blocker instances may not be garbage collected due to lingering event listeners.

Consider storing a reference to remove the listener when creating a new instance:

 private async createBlockerInstance(type: BlockerInstanceType): Promise<ElectronBlocker> {
   if (this.blockerInstancePromise && this.blockerInstanceType === type) {
     return this.blockerInstancePromise;
   }

   if (this.blockerInstancePromise) {
+    // Clean up previous blocker instance
+    const previousBlocker = await this.blockerInstancePromise;
+    previousBlocker.removeAllListeners();
     await this.disableBlocker();
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const blocker = await promise;
blocker.on("request-blocked", (request) => {
debugPrint("CONTENT_BLOCKER", "Request blocked:", request.url);
});
private async createBlockerInstance(type: BlockerInstanceType): Promise<ElectronBlocker> {
if (this.blockerInstancePromise && this.blockerInstanceType === type) {
return this.blockerInstancePromise;
}
if (this.blockerInstancePromise) {
+ // Clean up previous blocker instance
+ const previousBlocker = await this.blockerInstancePromise;
+ previousBlocker.removeAllListeners();
await this.disableBlocker();
}
// ... rest of the method unchanged
}
🤖 Prompt for AI Agents 
In src/main/modules/content-blocker.ts around lines 60 to 63, the event listener
added to the blocker instance is never removed, which can cause a memory leak if
createBlockerInstance is called multiple times. To fix this, store a reference
to the event listener function and ensure you remove it using blocker.off or
equivalent before replacing or discarding the blocker instance. This cleanup
will allow old blocker instances to be garbage collected properly.

});

this.blockerInstanceType = type;
return this.blockerInstancePromise as Promise<ElectronBlocker>;
return blocker;
} catch (error) {
debugPrint("CONTENT_BLOCKER", "Failed to create blocker instance:", error);
this.blockerInstancePromise = undefined;
this.blockerInstanceType = undefined;
throw error;
}
}

/**
Expand All @@ -58,74 +77,171 @@ class ContentBlocker {
private async disableBlocker(): Promise<void> {
if (!this.blockerInstancePromise) return;

const blocker = await this.blockerInstancePromise;
for (const session of this.blockedSessions) {
blocker.disableBlockingInSession(createBetterSession(session, SESSION_KEY));
}
try {
const blocker = await this.blockerInstancePromise;

// Disable blocking for all sessions
const disablePromises = Array.from(this.blockedSessions).map((session) =>
blocker.disableBlockingInSession(createBetterSession(session, SESSION_KEY))
);

this.blockedSessions = [];
this.blockerInstancePromise = undefined;
this.blockerInstanceType = undefined;
await Promise.allSettled(disablePromises);

this.blockedSessions.clear();
this.blockerInstancePromise = undefined;
this.blockerInstanceType = undefined;

debugPrint("CONTENT_BLOCKER", "Content blocker disabled for all sessions");
} catch (error) {
debugPrint("CONTENT_BLOCKER", "Error disabling blocker:", error);
}
}

/**
* Enables content blocking for a specific session
*/
private async enableBlockerForSession(blockerType: BlockerInstanceType, session: Session): Promise<void> {
const blocker = await this.createBlockerInstance(blockerType);
if (!blocker) return;
try {
const blocker = await this.createBlockerInstance(blockerType);

// check if session is already blocked
if (this.blockedSessions.includes(session)) return;
// Skip if already blocked
if (this.blockedSessions.has(session)) return;

// add session to blocked sessions
this.blockedSessions.push(session);
// Enable blocking
await blocker.enableBlockingInSession(createBetterSession(session, SESSION_KEY));

// enable blocking in session
blocker.enableBlockingInSession(createBetterSession(session, SESSION_KEY));
// Track blocked session
this.blockedSessions.add(session);

debugPrint("CONTENT_BLOCKER", `Enabled ${blockerType} blocking for session`);
} catch (error) {
debugPrint("CONTENT_BLOCKER", "Failed to enable blocker for session:", error);
}
}

/**
* Updates content blocker configuration based on user settings
* Removes a session from blocking
*/
public async updateConfig(): Promise<void> {
if (!browser) return;
public async removeSession(session: Session): Promise<void> {
if (!this.blockedSessions.has(session)) return;

try {
if (this.blockerInstancePromise) {
const blocker = await this.blockerInstancePromise;
await blocker.disableBlockingInSession(createBetterSession(session, SESSION_KEY));
}

this.blockedSessions.delete(session);
debugPrint("CONTENT_BLOCKER", "Removed session from content blocking");
} catch (error) {
debugPrint("CONTENT_BLOCKER", "Error removing session:", error);
}
}

/**
* Gets current blocker configuration
*/
private getBlockerConfig(): BlockerConfig {
const contentBlocker = getSettingValueById("contentBlocker") as string | undefined;
const profiles = browser.getLoadedProfiles();

switch (contentBlocker) {
case "all":
case "adsAndTrackers":
case "adsOnly":
for (const profile of profiles) {
this.enableBlockerForSession(contentBlocker as BlockerInstanceType, profile.session);
}
break;
return { type: contentBlocker as BlockerInstanceType, enabled: true };
default:
this.disableBlocker();
return { type: "adsOnly", enabled: false };
}
}

debugPrint("CONTENT_BLOCKER", "Content blocker configuration updated:", contentBlocker);
/**
* Updates content blocker configuration based on user settings
*/
public async updateConfig(): Promise<void> {
if (!browser) return;

// Debounce rapid configuration changes
if (this.updateTimeout) {
clearTimeout(this.updateTimeout);
}

this.updateTimeout = setTimeout(async () => {
try {
const config = this.getBlockerConfig();
const profiles = browser?.getLoadedProfiles() ?? [];
Comment on lines +161 to +171
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider moving browser null check inside timeout callback

The browser null check at line 161 might become stale by the time the debounced callback executes. Consider moving it inside the timeout callback:

 public async updateConfig(): Promise<void> {
-  if (!browser) return;
-
   // Debounce rapid configuration changes
   if (this.updateTimeout) {
     clearTimeout(this.updateTimeout);
   }

   this.updateTimeout = setTimeout(async () => {
     try {
+      if (!browser) return;
       const config = this.getBlockerConfig();
       const profiles = browser?.getLoadedProfiles() ?? [];
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!browser) return;
// Debounce rapid configuration changes
if (this.updateTimeout) {
clearTimeout(this.updateTimeout);
}
this.updateTimeout = setTimeout(async () => {
try {
const config = this.getBlockerConfig();
const profiles = browser?.getLoadedProfiles() ?? [];
public async updateConfig(): Promise<void> {
// Debounce rapid configuration changes
if (this.updateTimeout) {
clearTimeout(this.updateTimeout);
}
this.updateTimeout = setTimeout(async () => {
try {
if (!browser) return;
const config = this.getBlockerConfig();
const profiles = browser?.getLoadedProfiles() ?? [];
// …rest of logic…
} catch (e) {
// …error handling…
}
}, 100);
}
🤖 Prompt for AI Agents 
In src/main/modules/content-blocker.ts around lines 161 to 171, the null check
for the browser variable is done before setting the debounce timeout, which may
become outdated when the callback runs. Move the browser null check inside the
setTimeout callback function to ensure the browser is still valid when the
debounced code executes.


if (config.enabled) {
// Enable blocking for all profiles
const enablePromises = profiles.map((profile) => this.enableBlockerForSession(config.type, profile.session));
await Promise.allSettled(enablePromises);
} else {
// Disable blocking entirely
await this.disableBlocker();
}

debugPrint("CONTENT_BLOCKER", "Content blocker configuration updated:", config);
} catch (error) {
debugPrint("CONTENT_BLOCKER", "Error updating configuration:", error);
}
}, 100);
}

/**
* Cleans up resources and event listeners
*/
public async cleanup(): Promise<void> {
if (this.updateTimeout) {
clearTimeout(this.updateTimeout);
this.updateTimeout = undefined;
}

await this.disableBlocker();
this.isInitialized = false;
}
Comment on lines +192 to 200
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Missing event listener cleanup

The cleanup method should also remove the event listeners that were added during initialization to prevent memory leaks and unexpected behavior:

 public async cleanup(): Promise<void> {
   if (this.updateTimeout) {
     clearTimeout(this.updateTimeout);
     this.updateTimeout = undefined;
   }

+  // Remove event listeners
+  settingsEmitter.off("settings-changed", this.handleSettingsChanged);
+  browser?.off("profile-loaded", this.handleProfileLoaded);
+  browser?.off("profile-unloaded", this.handleProfileRemoved);
+
   await this.disableBlocker();
   this.isInitialized = false;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public async cleanup(): Promise<void> {
if (this.updateTimeout) {
clearTimeout(this.updateTimeout);
this.updateTimeout = undefined;
}
await this.disableBlocker();
this.isInitialized = false;
}
public async cleanup(): Promise<void> {
if (this.updateTimeout) {
clearTimeout(this.updateTimeout);
this.updateTimeout = undefined;
}
// Remove event listeners
settingsEmitter.off("settings-changed", this.handleSettingsChanged);
browser?.off("profile-loaded", this.handleProfileLoaded);
browser?.off("profile-unloaded", this.handleProfileRemoved);
await this.disableBlocker();
this.isInitialized = false;
}
🤖 Prompt for AI Agents 
In src/main/modules/content-blocker.ts around lines 192 to 200, the cleanup
method currently clears the updateTimeout and disables the blocker but does not
remove event listeners added during initialization. To fix this, identify all
event listeners registered in the initialization phase and explicitly remove
them in the cleanup method to prevent memory leaks and unexpected behavior.


/**
* Initializes content blocker and sets up event listeners
*/
public async initialize(): Promise<void> {
// Initial configuration
await this.updateConfig();

// Listen for setting changes
settingsEmitter.on("settings-changed", () => {
this.updateConfig();
});

// Listen for profile changes
browser?.on("profile-loaded", () => {
this.updateConfig();
});
if (this.isInitialized) return;

try {
// Initial configuration
await this.updateConfig();

// Listen for setting changes
settingsEmitter.on("settings-changed", this.handleSettingsChanged);

// Listen for profile changes
browser?.on("profile-loaded", this.handleProfileLoaded);
browser?.on("profile-unloaded", this.handleProfileRemoved);

this.isInitialized = true;
debugPrint("CONTENT_BLOCKER", "Content blocker initialized successfully");
} catch (error) {
debugPrint("CONTENT_BLOCKER", "Failed to initialize content blocker:", error);
}
}

/**
* Event handlers bound to maintain proper context
*/
private handleSettingsChanged = () => {
this.updateConfig();
};

private handleProfileLoaded = () => {
this.updateConfig();
};

private handleProfileRemoved = (profileId: string) => {
// Find the session for this profile and remove it
if (!browser) return;
const profile = browser.getLoadedProfile(profileId);
if (profile) {
this.removeSession(profile.session);
}
};
}

// Export singleton instance
Expand All @@ -136,3 +252,8 @@ onSettingsCached().then(() => {
debugPrint("CONTENT_BLOCKER", "Initializing content blocker");
contentBlocker.initialize();
});

// Handle app shutdown
process.on("beforeExit", async () => {
await contentBlocker.cleanup();
});
Loading