Refactor downloads into a ProfileKeyedService.

Refactor downloads into a ProfileKeyedService.

BUG=94383
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105115

[Randy Smith (Not in Mondays), 2011-10-05 18:43:02]

May as well get this in people's review queues.

mirandac@: Primary reviewer.  Most of this is boilerplate, so I'm not pulling in
owners from all the different directories--please let me know if you think I
should pull in some owners.  

ahendrickson@: Please take a look at download_browsertest.cc; it's almost
boilerplate, but not quite.

jam@: Please take a look at content/browser; it's boilerplate, but requires
OWNERS approval.  While you're here, you might want to look at content/shell and
content/test, or you might not.

Let me know what you think; thanks!

[jam, 2011-10-05 19:27:12]

http://codereview.chromium.org/8135017/diff/1034/content/browser/download/dow...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8135017/diff/1034/content/browser/download/dow...
content/browser/download/download_manager.h:303: friend class DownloadService;
two issues come to mind:
1) this isn't a test class
2) we shouldn't friend chrome classes in content. what methods does it need?

[Randy Smith (Not in Mondays), 2011-10-05 19:56:47]

On 2011/10/05 19:27:12, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/8135017/diff/1034/content/browser/download/dow...
> File content/browser/download/download_manager.h (right):
> 
>
http://codereview.chromium.org/8135017/diff/1034/content/browser/download/dow...
> content/browser/download/download_manager.h:303: friend class DownloadService;
> two issues come to mind:
> 1) this isn't a test class
> 2) we shouldn't friend chrome classes in content. what methods does it need?

Fair enough; I'll see if I can find a way to fix it.  The problem I was trying
to solve was that I want tests to be able to substitute a non-standard
DownloadManagerDelegate on the download manager.  I set things up in this CL so
that DownloadService owns the ChromeDMD, and I put an accessor on
DownloadService so that that could be reset as needed, and I put that friend
declaration in so that resetting it at the DownloadService level would reset it
for the DownloadManager as well (just so that I didn't have to do the same set
of relinking all over the place).

I think what I'm going to try to do instead is say that the DownloadManager is
the owner of the DownloadManagerDelegate, and just get DownloadService out of
the game of knowing about the DMD at all (except that it'll create the thing on
initialization).  But if you have other ideas, I'd welcome them--I'm not certain
that that's the best idea.  It also makes DownloadService a pretty empty
class--it's only job is to be something at the Profile level which creates a
DownloadManager.  But that's probably ok.

[ahendrickson, 2011-10-06 17:50:08]

download_browsertest.cc LGTM.

[Miranda Callahan, 2011-10-06 20:07:32]

Much cleaner than I was afraid -- I think I only have nits. I don't think there
is any reason to pull in OWNERs -- most of the changes in the non-core files are
standard.

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/download/down...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:601: ->GetDownloadManager();
nit: I think this is nonstandard wrapping (at least in chrome code); perhaps put
the whole argument on the next line?

return DownloadServiceFactory::GetForProfile(
    browser->profile()->GetDownloadManager());

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/download/down...
File chrome/browser/download/download_service.h (right):

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/download/down...
chrome/browser/download/download_service.h:16: class
ChromeDownloadManagerDelegate;
nit: alphasorting

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/download/down...
chrome/browser/download/download_service.h:25: // Get the download manager.
Could you add to this comment that this function has the side effect of creating
the DownloadManager if it does not already exist?

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/download/down...
chrome/browser/download/download_service.h:31: // Set the
DownloadManagerDelegate associated with this object and
Parallel construction nit: could you change to "Sets" (as "Takes" begins the
next sentence, making this comment descriptive rather than imperative ;-)

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/download/down...
File chrome/browser/download/download_service_factory.h (right):

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/download/down...
chrome/browser/download/download_service_factory.h:37: 
nit: extra blank line

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_downloads_api.cc (right):

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_downloads_api.cc:421: ->GetDownloadManager()
: NULL) {
hmm, again, wrapping before the -> is not kosher, I believe; what if we indent
line 420 four spaces, and then fit the whole second argument of the ternary
operator on this line?

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:98: #include
"content/browser/download/download_manager.h"
This could just be a forward declaration of DownloadManager now, I think.

[jam, 2011-10-06 23:16:08]

On 2011/10/06 20:07:32, Miranda Callahan wrote:
> Much cleaner than I was afraid -- I think I only have nits. I don't think
there
> is any reason to pull in OWNERs -- most of the changes in the non-core files
are
> standard.
> 

as one of the OWNERs of content, I don't think this should go in with a friend
to a chrome class :)

[Randy Smith (Not in Mondays), 2011-10-07 15:02:23]

On 2011/10/06 23:16:08, John Abd-El-Malek wrote:
> On 2011/10/06 20:07:32, Miranda Callahan wrote:
> > Much cleaner than I was afraid -- I think I only have nits. I don't think
> there
> > is any reason to pull in OWNERs -- most of the changes in the non-core files
> are
> > standard.
> > 
> 
> as one of the OWNERs of content, I don't think this should go in with a friend
> to a chrome class :)

Oh, don't worry, that feedback was accepted, just not implemented yet :-}.  

I think what Miranda was responding to was my request for her to evaluate if
anyone else needed to be pulled in on the rest of the review--you were already a
reviewer.

[Miranda Callahan, 2011-10-07 15:05:37]

On 2011/10/07 15:02:23, rdsmith wrote:
> On 2011/10/06 23:16:08, John Abd-El-Malek wrote:
> > On 2011/10/06 20:07:32, Miranda Callahan wrote:
> > > Much cleaner than I was afraid -- I think I only have nits. I don't think
> > there
> > > is any reason to pull in OWNERs -- most of the changes in the non-core
files
> > are
> > > standard.
> > > 
> > 
> > as one of the OWNERs of content, I don't think this should go in with a
friend
> > to a chrome class :)
> 
> Oh, don't worry, that feedback was accepted, just not implemented yet :-}.  
> 
> I think what Miranda was responding to was my request for her to evaluate if
> anyone else needed to be pulled in on the rest of the review--you were already
a
> reviewer.

Oh yes, that's exactly right. I by no means intended to interfere with or
overrule jam's comment! :-)

[jam, 2011-10-07 16:04:48]

On 2011/10/07 15:05:37, Miranda Callahan wrote:
> On 2011/10/07 15:02:23, rdsmith wrote:
> > On 2011/10/06 23:16:08, John Abd-El-Malek wrote:
> > > On 2011/10/06 20:07:32, Miranda Callahan wrote:
> > > > Much cleaner than I was afraid -- I think I only have nits. I don't
think
> > > there
> > > > is any reason to pull in OWNERs -- most of the changes in the non-core
> files
> > > are
> > > > standard.
> > > > 
> > > 
> > > as one of the OWNERs of content, I don't think this should go in with a
> friend
> > > to a chrome class :)
> > 
> > Oh, don't worry, that feedback was accepted, just not implemented yet :-}.  
> > 
> > I think what Miranda was responding to was my request for her to evaluate if
> > anyone else needed to be pulled in on the rest of the review--you were
already
> a
> > reviewer.
> 
> Oh yes, that's exactly right. I by no means intended to interfere with or
> overrule jam's comment! :-)

ah, nm then! :)

[Randy Smith (Not in Mondays), 2011-10-08 23:46:54]

I've incorporated all comments; PTAL.  

I'm still having try bot problems; working on that next.

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/download/down...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:601: ->GetDownloadManager();
On 2011/10/06 20:07:32, Miranda Callahan wrote:
> nit: I think this is nonstandard wrapping (at least in chrome code); perhaps
put
> the whole argument on the next line?
> 
> return DownloadServiceFactory::GetForProfile(
>     browser->profile()->GetDownloadManager());

The problem is that that's not the right parenthesis nesting, and it looked as
wrong to me with the correct parenthesis nesting as what I actually wrote. 
Here's the alternative:

  return DownloadServiceFactory::GetForProfile(
      browser->profile())->GetDownloadManager();

If you think that's better, I'm happy to go with it--just let me know.

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/download/down...
File chrome/browser/download/download_service.h (right):

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/download/down...
chrome/browser/download/download_service.h:16: class
ChromeDownloadManagerDelegate;
On 2011/10/06 20:07:32, Miranda Callahan wrote:
> nit: alphasorting

Done.

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/download/down...
chrome/browser/download/download_service.h:25: // Get the download manager.
On 2011/10/06 20:07:32, Miranda Callahan wrote:
> Could you add to this comment that this function has the side effect of
creating
> the DownloadManager if it does not already exist?

Done.

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/download/down...
chrome/browser/download/download_service.h:31: // Set the
DownloadManagerDelegate associated with this object and
On 2011/10/06 20:07:32, Miranda Callahan wrote:
> Parallel construction nit: could you change to "Sets" (as "Takes" begins the
> next sentence, making this comment descriptive rather than imperative ;-)

No problem--thanks for calling it out.  Done.

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/download/down...
File chrome/browser/download/download_service_factory.h (right):

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/download/down...
chrome/browser/download/download_service_factory.h:37: 
On 2011/10/06 20:07:32, Miranda Callahan wrote:
> nit: extra blank line

Done.

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_downloads_api.cc (right):

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_downloads_api.cc:421: ->GetDownloadManager()
: NULL) {
On 2011/10/06 20:07:32, Miranda Callahan wrote:
> hmm, again, wrapping before the -> is not kosher, I believe; what if we indent
> line 420 four spaces, and then fit the whole second argument of the ternary
> operator on this line?

Done.

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8135017/diff/1034/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:98: #include
"content/browser/download/download_manager.h"
On 2011/10/06 20:07:32, Miranda Callahan wrote:
> This could just be a forward declaration of DownloadManager now, I think.

Done.

http://codereview.chromium.org/8135017/diff/1034/content/browser/download/dow...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8135017/diff/1034/content/browser/download/dow...
content/browser/download/download_manager.h:303: friend class DownloadService;
On 2011/10/05 19:27:13, John Abd-El-Malek wrote:
> two issues come to mind:
> 1) this isn't a test class
> 2) we shouldn't friend chrome classes in content. what methods does it need?

I'm pretty sure I've cleaned up both issues (by giving up on enforcing the
restriction that only tests call SetDownloadManagerDelegate() as the compile
level, but that's ok).  Let me know what you think.

[Randy Smith (Not in Mondays), 2011-10-09 23:52:31]

Miranda: After thinking about it, I decided I preferred the wonkiness of the
unbalanced parenthesis indentation (see my response to your comments on
indentation) and changed over to that format.  The one exception is in
downloads_dom_handler.cc, where I couldn't figure out a way to stay on 80
columns in that style, so I did a variation of my old indentation.  Let me know
what you think

[jam, 2011-10-10 18:19:33]

http://codereview.chromium.org/8135017/diff/21082/content/browser/download/do...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8135017/diff/21082/content/browser/download/do...
content/browser/download/download_manager.h:298: void
SetDownloadManagerDelegate(DownloadManagerDelegate* delegate);
nit: please add a "ForTesting" suffix. we have presubmit checks that ensure that
they're only called by test code.

http://codereview.chromium.org/8135017/diff/21082/content/browser/download/do...
File content/browser/download/download_manager_delegate.h (right):

http://codereview.chromium.org/8135017/diff/21082/content/browser/download/do...
content/browser/download/download_manager_delegate.h:111: virtual void
SetDownloadManager(DownloadManager* manager) = 0;
this pattern of having the delegate be told by the object that it's been set is
not something we do with our other delegates. is this really necessary or can we
just do this manually? not all embedders might care to have the DownloadManager
pointer

[Miranda Callahan, 2011-10-11 14:14:42]

On 2011/10/09 23:52:31, rdsmith wrote:
> Miranda: After thinking about it, I decided I preferred the wonkiness of the
> unbalanced parenthesis indentation (see my response to your comments on
> indentation) and changed over to that format.  The one exception is in
> downloads_dom_handler.cc, where I couldn't figure out a way to stay on 80
> columns in that style, so I did a variation of my old indentation.  Let me
know
> what you think

I think you're right -- I missed a paren when I was proposing the new
indentation.

[Miranda Callahan, 2011-10-11 14:36:03]

On 2011/10/11 14:14:42, Miranda Callahan wrote:
> On 2011/10/09 23:52:31, rdsmith wrote:
> > Miranda: After thinking about it, I decided I preferred the wonkiness of the
> > unbalanced parenthesis indentation (see my response to your comments on
> > indentation) and changed over to that format.  The one exception is in
> > downloads_dom_handler.cc, where I couldn't figure out a way to stay on 80
> > columns in that style, so I did a variation of my old indentation.  Let me
> know
> > what you think
> 
> I think you're right -- I missed a paren when I was proposing the new
> indentation.

LGTM if jam is satisfied. And my apologies for not responding earlier --
recovering from a flu I've had for the past few days.

[Miranda Callahan, 2011-10-11 14:53:03]

http://codereview.chromium.org/8135017/diff/21082/content/browser/download/do...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8135017/diff/21082/content/browser/download/do...
content/browser/download/download_manager.h:298: void
SetDownloadManagerDelegate(DownloadManagerDelegate* delegate);
On 2011/10/10 18:19:33, John Abd-El-Malek wrote:
> nit: please add a "ForTesting" suffix. we have presubmit checks that ensure
that
> they're only called by test code.

Whoa, I didn't know that we had a presubmit check for that -- that's great! I
added a line to the Chromium style guide on naming to make that clear.

[Randy Smith (Not in Mondays), 2011-10-11 18:24:42]

Responding to John's latest round of comments.  Note that I haven't yet updated
the code, as I have one question I'd like John's response on before I do.

http://codereview.chromium.org/8135017/diff/21082/content/browser/download/do...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8135017/diff/21082/content/browser/download/do...
content/browser/download/download_manager.h:298: void
SetDownloadManagerDelegate(DownloadManagerDelegate* delegate);
On 2011/10/10 18:19:33, John Abd-El-Malek wrote:
> nit: please add a "ForTesting" suffix. we have presubmit checks that ensure
that
> they're only called by test code.

Can I convince you to rescind this request?  The problem is that I want to keep
the delegates in sync between DownloadService and DownloadManager, and so I call
DownloadManager::SetDownloadManagerDelegate(ForTesting :-}) from
DownloadService::SetDownloadManagerDelegateForTesting.  I can (and will) change
the name of the DownloadService call, but I'm already having to make the test
code wonkier and more prone to error than I'd like (see response to other
comment), and I'd rather have DownloadService call the DownloadManager version.

http://codereview.chromium.org/8135017/diff/21082/content/browser/download/do...
File content/browser/download/download_manager_delegate.h (right):

http://codereview.chromium.org/8135017/diff/21082/content/browser/download/do...
content/browser/download/download_manager_delegate.h:111: virtual void
SetDownloadManager(DownloadManager* manager) = 0;
On 2011/10/10 18:19:33, John Abd-El-Malek wrote:
> this pattern of having the delegate be told by the object that it's been set
is
> not something we do with our other delegates. is this really necessary or can
we
> just do this manually? not all embedders might care to have the
DownloadManager
> pointer

Yeah.  This is to make the test code simpler--it allows us to replace delegates
on the download manage and have the right thing happen with a single call (set
the DM delegate and have the DM tell the delegate what DM it's delegating for). 
But I agree that it's worth a fair amount of complexity in test code to make
production code simpler, so I'll change it.  It's just producing more fragile
test code.

[Randy Smith (Not in Mondays), 2011-10-11 18:51:06]

Updated to incorporate John's comments, minus the change of
DownloadManager::SetDownloadDelegate to
DownloadManager::SetDownloadDelegateForTesting for reasons mentioned previously.

[jam, 2011-10-11 19:31:12]

lgtm

http://codereview.chromium.org/8135017/diff/21082/content/browser/download/do...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8135017/diff/21082/content/browser/download/do...
content/browser/download/download_manager.h:298: void
SetDownloadManagerDelegate(DownloadManagerDelegate* delegate);
On 2011/10/11 18:24:43, rdsmith wrote:
> On 2011/10/10 18:19:33, John Abd-El-Malek wrote:
> > nit: please add a "ForTesting" suffix. we have presubmit checks that ensure
> that
> > they're only called by test code.
> 
> Can I convince you to rescind this request?  The problem is that I want to
keep
> the delegates in sync between DownloadService and DownloadManager, and so I
call
> DownloadManager::SetDownloadManagerDelegate(ForTesting :-}) from
> DownloadService::SetDownloadManagerDelegateForTesting.  I can (and will)
change
> the name of the DownloadService call, but I'm already having to make the test
> code wonkier and more prone to error than I'd like (see response to other
> comment), and I'd rather have DownloadService call the DownloadManager
version. 
> 

sure, sounds reasonable. ideally, the presubmit check wouldn't count calls from
other ForTest functions :)

http://codereview.chromium.org/8135017/diff/35004/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.h (right):

http://codereview.chromium.org/8135017/diff/35004/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.h:47: virtual void
SetDownloadManager(DownloadManager* dm);
nit: not virtual

http://codereview.chromium.org/8135017/diff/35004/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8135017/diff/35004/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:138: class DownloadManager;
this isn't needed

http://codereview.chromium.org/8135017/diff/35004/content/browser/download/do...
File content/browser/download/download_manager_delegate.h (right):

http://codereview.chromium.org/8135017/diff/35004/content/browser/download/do...
content/browser/download/download_manager_delegate.h:14: class DownloadManager;
nit: not needed anymore

http://codereview.chromium.org/8135017/diff/35004/content/shell/shell_downloa...
File content/shell/shell_download_manager_delegate.h (right):

http://codereview.chromium.org/8135017/diff/35004/content/shell/shell_downloa...
content/shell/shell_download_manager_delegate.h:24: virtual void
SetDownloadManager(DownloadManager* manager);
nit: not virtual

[Randy Smith (Not in Mondays), 2011-10-11 20:28:27]

All comments incorporated; believe I'm ok to commit with a good try bot run.  
Thanks!

And yes, it'd be awesome if the presubmit checks allowed calls from other
ForTesting routines :-}.

http://codereview.chromium.org/8135017/diff/35004/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.h (right):

http://codereview.chromium.org/8135017/diff/35004/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.h:47: virtual void
SetDownloadManager(DownloadManager* dm);
On 2011/10/11 19:31:13, John Abd-El-Malek wrote:
> nit: not virtual

Done.

http://codereview.chromium.org/8135017/diff/35004/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8135017/diff/35004/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:138: class DownloadManager;
On 2011/10/11 19:31:13, John Abd-El-Malek wrote:
> this isn't needed

Done.

http://codereview.chromium.org/8135017/diff/35004/content/browser/download/do...
File content/browser/download/download_manager_delegate.h (right):

http://codereview.chromium.org/8135017/diff/35004/content/browser/download/do...
content/browser/download/download_manager_delegate.h:14: class DownloadManager;
On 2011/10/11 19:31:13, John Abd-El-Malek wrote:
> nit: not needed anymore

Done.

http://codereview.chromium.org/8135017/diff/35004/content/shell/shell_downloa...
File content/shell/shell_download_manager_delegate.h (right):

http://codereview.chromium.org/8135017/diff/35004/content/shell/shell_downloa...
content/shell/shell_download_manager_delegate.h:24: virtual void
SetDownloadManager(DownloadManager* manager);
On 2011/10/11 19:31:13, John Abd-El-Malek wrote:
> nit: not virtual

Done.

[commit-bot: I haz the power, 2011-10-12 16:11:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/8135017/36005

[commit-bot: I haz the power, 2011-10-12 17:43:38]

Change committed as 105115

[tony, 2011-10-12 21:40:08]

The heapchecker is complaining about leaks with ProfileKeyedService in the
stack.
E.g.,:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20Heap...

Can you take a look?

[Randy Smith (Not in Mondays), 2011-10-12 23:00:25]

On 2011/10/12 21:40:08, tony wrote:
> The heapchecker is complaining about leaks with ProfileKeyedService in the
> stack.
> E.g.,:
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%2520OS%2520...
> 
> Can you take a look?

Sorry, I guess I was relying on the ASAN bot.  I'll try and dig into this.  Are
there bugs filed with suppressions I should assign to myself?  

(Glancing at the stacks, I'm not having any immediate "Oh, I get it!" reactions,
but that'll presumably change w/ time.)

[Randy Smith (Not in Mondays), 2011-10-12 23:03:45]

On 2011/10/12 23:00:25, rdsmith wrote:
> On 2011/10/12 21:40:08, tony wrote:
> > The heapchecker is complaining about leaks with ProfileKeyedService in the
> > stack.
> > E.g.,:
> >
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%252520OS%25...
> > 
> > Can you take a look?
> 
> Sorry, I guess I was relying on the ASAN bot.  I'll try and dig into this. 
Are
> there bugs filed with suppressions I should assign to myself?  
> 
> (Glancing at the stacks, I'm not having any immediate "Oh, I get it!"
reactions,
> but that'll presumably change w/ time.)

Also: Where can I find the suppressions associated with heapcheck?  Looking at
the stack, I think there's at least a chance that what's happened is that this
was an existing suppression which is now showing through because I changed the
top part of the callstack.  I didn't change anything that I would think would be
related to the bottom portion of the stack.

[tony, 2011-10-12 23:10:39]

I didn't add a suppression or file a bug for it-- please do if it's a
non-obvious fix.

The suppressions file is src/tools/heapcheck/suppressions.txt.

Thanks!