It seems that SaveFileManager also doesn't need to be ref-counted (it has a single owner - ResourceDispatcherHostImpl - there were no other hits when grepping for -i 'ref.*\bSaveFileManager\b' under content/browser).

With awesome help from codereview.chromium.org/2032283002, I took a look at this.

It turns out that a single WebContents can have multiple concurrent SavePackage instances, if a subsequent SavePage action are triggered by the user before the previous one has completed. Today, this works, because the old refcounted references remain valid through all their callbacks, even as WebContents overwrites that reference with a new SavePackage.

Options:

0. Continue refcounting. The lifecycle of a SavePackage is complex enough that  other solutions may be just as complex, but involve more code. Maybe add a comment explaining.

1. Use unique_ptr, amend WebContents to retain references to all in-progress SavePackages - not just the most recent one. With some lazy cleanup.

2. Use unique_ptr, amend all downstream callbacks to use a WeakPtr(?)/Handle, and explicitly cope with it vanishing from underneath them.

My guess is that (0) is the way to go; objects that get passed through callback chains should probably be refcounted. WDYT?

How about serializing SavePage actions?

I don't agree that objects that get passed through callback chains should be refcounted by default; I'd rather have well defined lifetime guarantees between the objects the callback chains are being passed among, and write the callbacks s.t. we can guarantee the existing of objects whose pointers are passed in callbacks.  Failing that, I'd prefer weak pointers to reference counting (the race is at the point of evaluating the weak pointer, not around destruction order--it's usually more tractable).  I do accept that there are times when we we want to use reference counting and/or weak pointers, but I'd like to explore other options before going with those ones.

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot