Skip to content

Commit

Permalink
Do let grow subframe dictionary grow unbound
Browse files Browse the repository at this point in the history
Related discussion:
- https://bugzilla.mozilla.org/show_bug.cgi?id=1652925

It's not clear the code here will fix the reported
issue, but I did identify that the subframe
dictionary of a very long-lived web page can
theoretically grow unbound.
  • Loading branch information
gorhill committed Jul 18, 2020
1 parent cf31d83 commit feabfe3
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 2 deletions.
1 change: 1 addition & 0 deletions platform/chromium/webext.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ const webext = {
// https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/webNavigation
webNavigation: {
getFrame: promisify(chrome.webNavigation, 'getFrame'),
getAllFrames: promisify(chrome.webNavigation, 'getAllFrames'),
},
// https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/windows
windows: {
Expand Down
37 changes: 35 additions & 2 deletions src/js/pagestore.js
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ const FrameStore = class {
}

init(frameURL) {
this.t0 = Date.now();
this.exceptCname = undefined;
this.rawURL = frameURL;
if ( frameURL !== undefined ) {
Expand Down Expand Up @@ -252,6 +253,7 @@ const PageStore = class {

this.frames = new Map();
this.setFrame(0, tabContext.rawURL);
this.frameAddCount = 0;

// The current filtering context is cloned because:
// - We may be called with or without the current context having been
Expand Down Expand Up @@ -359,8 +361,39 @@ const PageStore = class {
const frameStore = this.frames.get(frameId);
if ( frameStore !== undefined ) {
frameStore.init(frameURL);
} else {
this.frames.set(frameId, FrameStore.factory(frameURL));
return;
}
this.frames.set(frameId, FrameStore.factory(frameURL));
this.frameAddCount += 1;
if ( (this.frameAddCount & 0b111111) !== 0 ) { return; }
this.pruneFrames();
}

// There is no event to tell us a specific subframe has been removed from
// the main document. The code below will remove subframes which are no
// longer present in the root document. Removing obsolete subframes is
// not a critical task, so this is executed just once on a while, to avoid
// bloated dictionary of subframes.
// A TTL is used to avoid race conditions when new iframes are added
// through the webRequest API but still not yet visible through the
// webNavigation API.
async pruneFrames() {
let entries;
try {
entries = await webext.webNavigation.getAllFrames({
tabId: this.tabId
});
} catch(ex) {
}
if ( Array.isArray(entries) === false ) { return; }
const toKeep = new Set();
for ( const { frameId } of entries ) {
toKeep.add(frameId);
}
const obsolete = Date.now() - 60000;
for ( const [ frameId, { t0 } ] of this.frames ) {
if ( toKeep.has(frameId) || t0 >= obsolete ) { continue; }
this.frames.delete(frameId);
}
}

Expand Down

6 comments on commit feabfe3

@gorhill
Copy link
Owner Author

Choose a reason for hiding this comment

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

Argh... Should be "Do not let...".

@jspenguin2017
Copy link
Contributor

@jspenguin2017 jspenguin2017 commented on feabfe3 Jul 18, 2020

Choose a reason for hiding this comment

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

Are you sure this is the root cause? PageStore doesn't store favicons, right? Also, when the tab is closed, it should remove the entire pageStores map entry, which should free the memory. The frame dictionary problem is known for years (jspenguin2017/uBlockProtector#799), but I never saw any actual issues that are caused by it.

Also, you can use http://nowhere.test/ or something for the test page (cf31d83), it doesn't have to fully load for extensions to be notified about the new frames.

@gorhill
Copy link
Owner Author

Choose a reason for hiding this comment

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

At the bugzilla issue I say:

I emphasized "ressembles" because I can't reproduce what I see in your memory measurement when I use a data: URIs for the source property on the iframes. When I use a data: URI, the code path where uBO put the URL in its iframe dictionary is never reached. So I still don't know the exact issue behind the memory leak you see.

@jspenguin2017
Copy link
Contributor

Choose a reason for hiding this comment

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

It smells like a browser bug to me.

@gorhill
Copy link
Owner Author

Choose a reason for hiding this comment

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

you can use http://nowhere.test/

Thanks, I didn't know about that trick.

@jspenguin2017
Copy link
Contributor

Choose a reason for hiding this comment

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

http://nowhere.invalid/ works too, both test and invalid are reserved TLDs:
https://en.wikipedia.org/wiki/.test
https://en.wikipedia.org/wiki/.invalid

Please sign in to comment.