Closed Bug 672111 Opened 13 years ago Closed 12 years ago

Zombie compartment with AdBlock Plus due to redirect detection

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: jo.hermans, Unassigned)

References

()

Details

(Keywords: memory-leak, Whiteboard: [MemShrink:P2][see comment 5])

Mozilla/5.0 (Windows NT 5.1; rv:8.0a1) Gecko/20110716 Firefox/8.0a1

Since bug 671053 was fixed, most of my compartment leaks appear to have been fixed. I only notice them again when I'm using AdBlock Plus. At first I could not find a good repeatable testcase, but maps.google.be seems to trigger it for me everytime.

preparation :
- make a clean profile
- no add-ons of any kind
- I'm using Flash 10.3r181
- install AdBlock Plus 1.3.9 and enable it
- make the default home-page empty (about:home)
- create an app-tab with about:memory
- go to maps.google.be, and make sure that it's in satellite mode (not the regular map)

test :
- start the browser, it will go directly into about:home, with the about:memory app-tab on the left
- go to the locationbar and load maps.google.be. Don't interact with it though.
- click on the app-tab, so that maps.google.be is not the front tab anymore
- close maps.google.tab wile it's still in the background
- refresh the about:memory app-tab, to see the compartment
- you'll notice that maps.google.be still use about 7 or 8 MB of memory
- wait a while and try again
- the compartment will still be there, even after waiting 5 minutes

control test :
- disable Adblock Plus and restart
- if you now do the test above, you'll notice that the maps.google.be quickly shrinks to about 1.5MB, and disappears after 10 seconds or so

caveats :
- it's absolutely important that you close the maps.google.be tab while it's in the background. I never saw the leak when it was in the foreground.
- likewise, I never saw the bug when AdBlock Plus was disabled, despite heavy surfing for days. For me, bug 671053 almost fixes the entire issue, except for this issue with Adblock Plus.
- I saw the bug almost 100% when I repeat the test above, but a couple of time I did not see it. That happened every time when I did something else, like interacting with the map, opening another tab, etc ... I think that I saw the leak cleaned up once when I switched to regular map mode in the test (which loads a bit faster), but it wasn't repeatable. I'm thinking that it might be timing-related.
- I have noticed that if you keep surfing when the leak is present, the compartment sometimes spontaneously disappears (when it's pushed out by the bf-cache or cycle-collect or whatever), but it doesn't happen every time. I've seen the leak even after several hours of surfing.

I don't know if the actual site is important, but it's almost 100% reproducible for me here, while almost 0% using other websites. This is just the simplest testcase I could find.

It's however 100% related to AdBlock Plus (not necessarily due to the add-on, but it affects timing). I repeat that I never saw a leak anymore since bug 671053 was fixed, if I'm not using AdBlock Plus.

I don't claim that this will be repeatable for everybody, as it seems to be partially timing dependent. But at least for me, I can use this testcase to test for compartment leaks.
Whiteboard: [MemShrink]
If I click Minimize Memory the compartment goes away even with ABP (this is on the en-US Google Maps though)
Ah, forgot to include that in the report. It doesn't make a difference to me if I click that button.
Whiteboard: [MemShrink] → [MemShrink:P3]
I was able to reproduce this (somewhat inconsistently) on a Linux-64 nightly build with ABP installed.

This could be bug 669096.
This is intermittent for me (9.0a1 nightly on Windows 7 x64), most of the time the compartment doesn't stay around. When it does going to maps.google.be again and closing it "resolves" the issue.

Bug 669096 doesn't seem related (at least not directly) as Adblock Plus doesn't inject functions into content. It does however attach chrome data to content nodes, I'll check whether that's causing the issue.
Ok, I understood what is happening. The problem is (once again) redirect detection. Adblock Plus needs to connect content policy calls to HTTP channels. For that reason it keeps the parameter of the last content policy call around, until it receives the corresponding "http-on-modify-request" notification. The assumption here is that "http-on-modify-request" comes after the content policy call, this isn't the case for preloading of images/scripts/stylesheets however (two content policy calls here, second happening after the request already started). So Adblock Plus ends up keeping a reference to a DOM node until the next content policy call - if the tab is closed in the meantime this means that the compartment cannot be released. Not sure yet what can be done to resolve this problem (short of using a weak reference to that node which used to cause all kinds of problems when done from JS).
Wladimir, could this explain the other zombie compartment reports for ABP?
Whiteboard: [MemShrink:P3] → [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P2]
Summary: compartment leak with Google Maps and AdBlock Plus → Zombie compartment with AdBlock Plus due to redirect detection
Whiteboard: [MemShrink:P2] → [MemShrink:P2][see comment 5]
Wladimir, would it work to replace previousRequest with a WeakMap whose keys are channels?
No - if I knew the channel in the content policy call there wouldn't be a problem. The issue really is that we have a content policy call and the channel is only created later. So to add content policy info to the channel one needs to store it temporarily.
(In reply to Wladimir Palant from comment #9)
> No - if I knew the channel in the content policy call there wouldn't be a
> problem. The issue really is that we have a content policy call and the
> channel is only created later. So to add content policy info to the channel
> one needs to store it temporarily.

I'm going out on a limb here, but can't you clear the content policy info also after a reasonable amount of time has passed, i.e. on a timer? That way the zombie compartment would go away after, say, a minute.
(In reply to Kurt Pruenner from comment #10)
> I'm going out on a limb here, but can't you clear the content policy info
> also after a reasonable amount of time has passed, i.e. on a timer? That way
> the zombie compartment would go away after, say, a minute.

I considered doing that, will do it if I cannot come up with anything better. The main problem is defining a "reasonable amount", a more reliable approach would be better.
Assignee: general → nobody
Component: JavaScript Engine → Networking: HTTP
OS: Windows XP → All
QA Contact: general → networking.http
Hardware: x86 → All
(In reply to Wladimir Palant from comment #11)
> (In reply to Kurt Pruenner from comment #10)
> > I'm going out on a limb here, but can't you clear the content policy info
> > also after a reasonable amount of time has passed, i.e. on a timer? That way
> > the zombie compartment would go away after, say, a minute.
> 
> I considered doing that, will do it if I cannot come up with anything
> better. The main problem is defining a "reasonable amount", a more reliable
> approach would be better.

AFAIK at this moment http-on-modify-request observers are notified synchronously after the shouldLoad() call, in the same event. Wouldn't dispatching a cleanup event on the current thread just do the trick (of course assuming that nothing calls processNextEvent() in-between)? A timer with a very short delay should work fine anyway, even though it may be more expensive.
(In reply to Wladimir Palant from comment #5)
> Ok, I understood what is happening. The problem is (once again) redirect
> detection. Adblock Plus needs to connect content policy calls to HTTP
> channels. For that reason it keeps the parameter of the last content policy
> call around, until it receives the corresponding "http-on-modify-request"
> notification. The assumption here is that "http-on-modify-request" comes
> after the content policy call, this isn't the case for preloading of
> images/scripts/stylesheets however (two content policy calls here, second
> happening after the request already started). So Adblock Plus ends up
> keeping a reference to a DOM node until the next content policy call - if
> the tab is closed in the meantime this means that the compartment cannot be
> released. Not sure yet what can be done to resolve this problem (short of
> using a weak reference to that node which used to cause all kinds of
> problems when done from JS).

Just a shot in the dark here, but can't it listen for "onbeforeunload"? That way, you're only releasing it when a tab is going to be closed and it's not needed, and you're able to get the event before the tab actually finishes unloading.
(In reply to donrhummy from comment #14)
Yes, I could use progress listeners - but I don't really want to mingle UI-independent code (such as content policies) with UI-dependent code. The request might not be associated with a tab, e.g. when coming from an RSS reader extension opening in a new window.
I've given up on cleaning up data that is attached to channels, simply not attaching the DOM node any more: https://hg.adblockplus.org/adblockplus/rev/76413c22b15e. I guess the minor functionality loss is ok.

The next development build will no longer have this issue. Should this bug be resolved as FIXED now or when the stable version is released?
(In reply to Wladimir Palant from comment #16)
> 
> The next development build will no longer have this issue. Should this bug
> be resolved as FIXED now or when the stable version is released?

Let's wait for the stable version.  Thanks for fixing!
(In reply to Wladimir Palant from comment #16)
> I guess the minor functionality loss is ok.

Hello Wladimir. Can you explain the kind of minor functionality will be lost and its consequence of the functioning of AB+?
Does the new dev build will retrieve this functionality? Thanks.
Yes, Adblock Plus 1.3.11a.3218 has this change already. The functionality loss: a redirect can no longer be associated with a particular DOM node. Consequently, if a redirect is blocked the "Collapse blocked elements" option will have no effect (the node will not be hidden). Also, the list of blockable items will have limited information on this request (size information unavailable, selecting the entry will no highlight the DOM node in the document).
I have been running Nightly with Adblock Plus 1.3.11a.3233 for the last 6 days or so. And it's really a very big difference. Every time when I close my tabs (*), and after waiting 20-30 seconds the explicit counter would drop back to the same amount (85MB in my case).  In particular, I didn't see the problem in Google Maps anymore, the compartement was deleted every time.

(*) I still saw 2 leaks, one in youtube, and one in bugzilla, but they occurred much less frequent than before.
Adblock Plus 2.0.1 has been released yesterday, marking as fixed (feel free to verify).
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Verified in Firefox 8.0 and the latest Nightly, both with Adblock Plus 2.0.1.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.