Closed Bug 415684 Opened 13 years ago Closed 13 years ago

Firefox leaks nsAutoRepeatBoxFrame when a lot of tabs are open

Categories

(Core :: General, defect, P4)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: cbook, Assigned: roc)

References

Details

(Keywords: memory-leak)

Attachments

(3 files, 1 obsolete file)

Attached file 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
Flags: blocking1.9?
Version: unspecified → Trunk
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.
Assignee: nobody → roc
Flags: blocking1.9? → blocking1.9+
Priority: -- → P4
Is nsAutoRepeatBoxFrame the only thing that's leaked? Or are we leaking documents, windows, presshells etc too?
Looks like it's just the frame and nothing else. See the attached leak log.
Mostly I was worried about the leak leading to dangerous dangling pointers. Don't know if this is an issue or not.
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.
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #302259 - Flags: superreview?(mats.palmgren)
Attachment #302259 - Flags: review?(mats.palmgren)
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.
Whiteboard: [needs review]
Attached file 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.
Attached patch fix v2Splinter Review
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...
Attachment #302259 - Attachment is obsolete: true
Attachment #302270 - Flags: superreview?(mats.palmgren)
Attachment #302270 - Flags: review?(mats.palmgren)
Attachment #302259 - Flags: superreview?(mats.palmgren)
Attachment #302259 - Flags: review?(mats.palmgren)
Blocks: 399235
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 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.
Attachment #302270 - Flags: superreview?(mats.palmgren)
Attachment #302270 - Flags: superreview+
Attachment #302270 - Flags: review?(mats.palmgren)
Attachment #302270 - Flags: review+
OS: Mac OS X → All
Hardware: PC → All
Whiteboard: [needs review]
Whiteboard: [needs landing]
> >      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.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
You need to log in before you can comment on or make changes to this bug.