Blocking https://chromium-review.googlesource.com/c/chromium/src/+/1354741.

Is ValidKey is defined as follows.

template <typename T,
          typename U,
          typename V,
          typename W,
          typename X,
          typename Y>
inline bool HashMap<T, U, V, W, X, Y>::IsValidKey(KeyPeekInType key) {
  if (KeyTraits::IsDeletedValue(key))
    return false;

  if (HashFunctions::safe_to_compare_to_empty_or_deleted) {
    if (key == KeyTraits::EmptyValue())
      return false;
  } else {
    if (IsHashTraitsEmptyValue<KeyTraits>(key))
      return false;
  }

  return true;
}

In this case, RefPtrValuePeeker is used for KeyPeekInType. 

  class RefPtrValuePeeker {
    DISALLOW_NEW();

   public:
    ALWAYS_INLINE RefPtrValuePeeker(P* p) : ptr_(p) {}
    template <typename U>
    RefPtrValuePeeker(const scoped_refptr<U>& p) : ptr_(p.get()) {}

    ALWAYS_INLINE operator P*() const { return ptr_; }

   private:
    P* ptr_;
  };

Calling IsDeletedValue for a KeyPeekInType is invalid at least for this specific type, as IsDeletedValue needs a const scoped_refptr<>& but RefPtrValuePeeker stores a raw pointer even constructed from a scoped_refptr.

s/even constructed from/even when constructed from/

Hmm, I think it's wrong to pass a scoped_refptr<T> to IsValidKey(), because it
expects KeyPeekInType which is T*. How about calling like
map.IsValidKey(x.get())?

It doesn't matter because IsValidKey takes KeyPeekInType. In this case, both scoped_refptr<A> and A* are converted to RefPtrValuePeeker.

Ah okay, I think RefPtrValuePeeker's point is to make the key peekable by
either scoped_refptr<> or a raw pointer. So we can't replace it with something
else.

In that case, I think we should just add an overloaded IsDeletedValue that
looks like:

static bool IsDeletedValue(const P* value) {
  return value == reinterpret_cast<P*>(-1);
}

Hm, apparently IsValidKey is used only in mojo/public/cpp/bindings/map_traits_wtf_hash_map.h so we can change it relatively safely.

Can we change the signature to
 
  template<typename Key>
  static bool IsValidKey(const Key&);

instead of

  static bool IsValidKey(KeyPeekInType);

?


That way IsDeletedValue will be called with the given type.

Hm yeah, that's also OK, I think.

Thanks, I'll make a CL.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/82ed9409527a75b631d9a2f9b038097113f3ed8a

commit 82ed9409527a75b631d9a2f9b038097113f3ed8a
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Fri Nov 30 08:18:10 2018

Fix HashMap<K, V>::IsValidKey(key) for K = scoped_retptr<T>

Currently HashMap<K, V>::IsValidKey takes PeekInType. This is causing
a problem when K is a scoped_refptr<T>, because PeekInType for
scoped_refptr<T> is essentially a raw pointer while
KeyTraits::IsDeleted which is called in IsValidKey takes
scoped_refptr<T>, not T*.

Bug:  910033 
Change-Id: I60bceb11a3e4f219c40030e0b606d709d4f13cd2
Reviewed-on: https://chromium-review.googlesource.com/c/1351979
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Yuta Kitamura <yutak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612595}
[modify] https://crrev.com/82ed9409527a75b631d9a2f9b038097113f3ed8a/third_party/blink/renderer/platform/wtf/hash_map.h
[modify] https://crrev.com/82ed9409527a75b631d9a2f9b038097113f3ed8a/third_party/blink/renderer/platform/wtf/hash_map_test.cc

Thank you for the quick fix!