Fix DCHECK, memory leak, and refactor PasswordStore to use CancelableRequest

content/browser/cancelable_request.h

Index: content/browser/cancelable_request.h
diff --git a/content/browser/cancelable_request.h b/content/browser/cancelable_request.h
index 9dc86d32da4119b7641472285205b992243b2adf..a27747f394b971df5cc876c401ea68826d47379a 100644
--- a/content/browser/cancelable_request.h
+++ b/content/browser/cancelable_request.h
@@ -127,7 +127,16 @@ class CancelableRequestProvider {
   // handle must be for a valid pending (not yet complete or cancelled) request.
   void CancelRequest(Handle handle);
 
+  // Provide factory method so Handle can be treated as opaque.
+  static Handle InvalidHandle();
+
+  // Provide validity method so Handle can be treated as opaque.
+  static bool HandleIsValid(Handle handle);

[brettw, 2011/03/22 21:43:41]

Why not just have calling code compare to InvalidHandle() themselves? This
function is potentially ambiguous since it could be construed to mean that there
is actually a request for the given handle, which isn't necessarily the case for
you.

[Sheridan Rawlins, 2011/03/22 22:04:09]

One could imagine Handles which are not equal to themselves being invalid.  That
said, in this case, only the 0 handle is invalid.  Can you recommend a better
name to be less ambiguous?

[brettw, 2011/03/22 22:11:08]

I'm not sure what you're saying here.

[Sheridan Rawlins, 2011/03/23 00:39:52]

I'm saying maybe there are multiple Invalid Handles -1, 0, or maybe Handle
becomes something other than an int.  In that case, InvalidHandle was meant to
be a factory or singleton at the discretion of the implementor of
CancelableRequestProvider (not called NewInvalidHandle since it doesn't allocate
something in need of freeing).  HandleIsValid is meant to check the validity of
the handle and return true if the handle is valid.

I see your point, however, inasmuch as this doesn't check if there's a request
outstanding, only that the handle is useable...  Maybe HandleIsInitialized()
would be a better name?  I can change the name in a followup CL.

[brettw, 2011/03/23 01:49:00]

Having complicated "multiple invalid handles" seems like a silly design and we
shouldn't design this based on the fact that we may want to change this
many-year-old code in that way.

I think 0 was a fine invalid handle, and I don't even see the point of having
constants or functions. The callers should just do
  if (!handle)
if they want to check handle validity. This is well-defined and obvious when
reading the calling code, and it's worked fine for all the other consumers of
this.

[Sheridan Rawlins, 2011/03/23 03:16:50]

I think you're describing Speculative Generality
(http://sourcemaking.com/refactoring/speculative-generality).

As an extension of your reasoning, and to help me understand the code, why do we
have/need the typedef int Handle?

I didn't realize this was many-year-old code.  I understood that PasswordStore
was modeled after HistoryService, which at the time wasn't factored into the
general cancelable_request framework.  FWIW, before I rewrote PasswordStore to
use CancelableRequestProvider, it had a bug, where the numbering for
PasswordStore started at 0 (not 1 like CancelableRequestProvider), and the first
client doing if (handle) could take the wrong action...

Because of the typedef for Handle, the start @ 0 bug I found in PasswordStore,
and the comments of my reviewers, I strove to make Handle more opaque.

So, what would LGTY?  Would ditching the methods/constants, and commenting the
initialize to 0 and boolean test for validity suffice?  Should we also ditch the
typedef and have all clients use int?

+
  protected:
+  static const Handle kFirstHandle;
+  static const Handle kInvalidHandle;
+
   // Adds a new request and initializes it. This is called by a derived class
   // to add a new request. The request's Init() will be called (which is why
   // the consumer is required. The handle to the new request is returned.