415684 - Firefox leaks nsAutoRepeatBoxFrame when a lot of tabs are open

Status: RESOLVED FIXED

(Core :: General, defect, P4)

Description

[Carsten Book [:Tomcat]]

Attachment: leak log

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008020423 Firefox/3.0b3pre

   0 TOTAL                                          27     2912 19930831       26 (76598.80 +/-     0.00) 40854248        0 (24633.94 +/-     0.00)

Steps to reproduce:
- Open a lot of tabs (over 70) from various topsites like nytimes, latimes, cnn.com, default headlines
- Firefox acts very slow and takes a lot of response time when all this tabs are loaded
- Quit Firefox later (ensure all tabs are finally loaded to avoid the other shutdown leaks)
--> Firefox leaks

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Marking blocking since this sounds like a leak that could accumulate over time. However the leak is really small so giving low priority.

Roc, any ideas here. Could this lead to worse things than just leaks, if so we might want to up priority.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is nsAutoRepeatBoxFrame the only thing that's leaked? Or are we leaking documents, windows, presshells etc too?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Looks like it's just the frame and nothing else. See the attached leak log.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Mostly I was worried about the leak leading to dangerous dangling pointers. Don't know if this is an issue or not.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is probably bogus. nsAutoRepeatBoxFrame does

NS_IMPL_ADDREF_INHERITED(nsAutoRepeatBoxFrame, nsButtonBoxFrame)
NS_IMPL_RELEASE_INHERITED(nsAutoRepeatBoxFrame, nsButtonBoxFrame)

but frames aren't really refcounted. In particular nsFrame::Release always returns a refcount of 1. Frames don't normally do refcount logging but the NS_IMPL_RELEASE_INHERITED adds refcount logging ... but the refcount logging never sees Release returning a zero refcount, so it thinks that all nsAutoRepeatBoxFrames leak.

Having said that, we should fix things so that these frames don't show up in leak logs, preferably by making nsAutoRepeatBoxFrame not implement a refcounted interface (nsITimerCallback in this case). We can probably do this by making nsRepeatService use a custom not-refcounted interface since most of its clients are frames and it's designed so that its clients remove themselves on destruction instead of the repeatservice keeping the clients alive.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

As described ... fixes nsRepeatService by removing its dependency on nsITimerCallback so it works well with frames. This removes the need to override AddRef/Release in nsAutoRepeatBoxFrame, so no more bogus leak reports.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I also noticed a couple of cleanup items that should be taken care of:

-- nsAutoRepeatBoxFrame should not cache the "repeat" attribute value in mIsPressMode, that's a ludicrous "optimization". A bunch of code can be removed if we just check the attribute value whenever we need it.

-- nsSliderFrame should use nsCOMPtr for its mMediator.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase

While looking at this code, I realized that there's an obvious problem: deleting a scrollbar button frame, or an nsAutoRepeatBoxFrame, will stop the repeat service. So in this testcase if script asynchronously deletes a scrollable element while we're scrolling another element by holding down a scrollbar button, scrolling mysteriously stops. This is a trunk bug.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Updated patch that only stops the nsRepeatService if it's currently repeating the frame that's being destroyed.

In an ideal world we probably wouldn't have nsRepeatService, we'd just have content setting up its own timers, but oh well...

Comment 10

[Jeff Walden [:Waldo]]

Note the bug 399235 dependency I just added; also see bug 322940 (further up the dependency chain) for the general problem.

Also note that the dependency I added is basically a dup, yet that one was marked blocking1.9- while this one was marked blocking1.9+.  I'm not entirely sure what the significance of that is...

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 302270 [details] [diff] [review]
fix v2

>+++ layout/xul/base/src/nsRepeatService.cpp
>-: mFirstCall(PR_FALSE)
>+: mCallback(nsnull), mCallbackData(nsnull), mFirstCall(PR_FALSE)

You can remove mFirstCall, it's not used.


>+void nsRepeatService::Start(Callback aCallback, void* aCallbackData)
> {
>   NS_PRECONDITION(aCallback != nsnull, "null ptr");
>-  if (! aCallback)
>+  if (!aCallback)
>     return;

No need for the early return since we assert that?


> NS_IMETHODIMP nsRepeatService::Notify(nsITimer *timer)
> {
>    // if the repeat delay is the initial one reset it.
>   if (mRepeatTimer) {
>      mRepeatTimer->Cancel();
>   }

No need to Cancel() here since we only create TYPE_ONE_SHOT timers?
BTW, why do we do that?  Wouldn't be more efficient if the second one
were repeating?

>      mRepeatTimer = do_CreateInstance("@mozilla.org/timer;1");
>      mRepeatTimer->InitWithCallback(...

Need a NS_SUCCEEDED/null check here.


> NS_INTERFACE_MAP_BEGIN(nsSliderMediator)
>   NS_INTERFACE_MAP_ENTRY(nsIDOMMouseListener)
>-  NS_INTERFACE_MAP_ENTRY(nsITimerCallback)
>-  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsITimerCallback)
> NS_INTERFACE_MAP_END
> 
> NS_IMPL_ADDREF(nsSliderMediator)
> NS_IMPL_RELEASE(nsSliderMediator)

NS_IMPL_ISUPPORTS1(nsSliderMediator, nsIDOMMouseListener) ?


r+sr=mats with the above addressed as you see fit, but please post
a new patch for review if you decide to do a repeating 2nd timer.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> >      mRepeatTimer = do_CreateInstance("@mozilla.org/timer;1");
> >      mRepeatTimer->InitWithCallback(...
>
> Need a NS_SUCCEEDED/null check here.

Actually I don't need to recreate the timer. I'll just reinit it.

Checked in with that and the other comments fixed.