Issue 675881  resurfacing?

Based on code search on the crashed file "WeakIdentifierMap.h" suspecting the below.
Review-Url: https://codereview.chromium.org/2709033003
pilgrim@: Would you mind taking a look into this.

Seems unrelated to my refactoring, sorry.

This appears to be a crash related to compositor worker.

I'm still trying to get the repro to trigger from fuzzer, but at a glance it looks like DOMNodeIds::nodeForId uses WeakIdentifierMap<Node>::lookup and assumes that that will always work even if the id is invalid. 

Internally, WeakIdentifierMap<Node>::lookup ends up at HashMap<...>::at(..) which does:

  ValueType* entry = const_cast<HashTableType&>(m_impl).lookup(key);
  if (!entry)
    return MappedTraits::peek(MappedTraits::emptyValue());
  return MappedTraits::peek(entry->value);

So it's expecting MappedTraits::peek to succeed even for the empty value, and I would guess it isn't. Will try manually triggering the repro.

Side note: the wtf HashMap/HashTable code could do with a *lot* more documentation. It is *very* complicated with many templated types passed around all over the place :(

Hrm, apparently not. The following test case succeeds even with the code patched to use a garbage element id.

  var div = document.createElement("div");
  var cp = new CompositorProxy(div, ['opacity']);

diff --git a/third_party/WebKit/Source/core/dom/CompositorProxy.cpp b/third_party/WebKit/Source/core/dom/CompositorProxy.cpp
index e715638b804f..6025c548b47d 100644
--- a/third_party/WebKit/Source/core/dom/CompositorProxy.cpp
+++ b/third_party/WebKit/Source/core/dom/CompositorProxy.cpp
@@ -63,7 +63,10 @@ static void incrementCompositorProxiedPropertiesForElement(
     uint64_t elementId,
     uint32_t compositorMutableProperties) {
   DCHECK(isMainThread());
+  LOG(INFO) << "SMCGRUER: incrementCompositorProxiedPropertiesForElement, changing elementId to probably invalid number";
+  elementId = 0xDEADBEEF;
   Node* node = DOMNodeIds::nodeForId(elementId);
+  LOG(INFO) << "SMCGRUER: node is " << node;
   if (!node)
     return;
   Element* element = toElement(node);


Output is:

[1:1:0309/104028.986232:5090346065902:INFO:CompositorProxy.cpp(66)] SMCGRUER: incrementCompositorProxiedPropertiesForElement, changing elementId to probably invalid number
[1:1:0309/104028.986611:5090346066126:INFO:CompositorProxy.cpp(69)] SMCGRUER: node is null

So it correctly handles that case, I'm unclear what the fuzzer might be up to in order to break it.

Oh, I wasn't thinking. We're looking at the *key*, not the value it is mapped to. The case that fails is static T emptyValue() { return T(); } == static int emptyValue() { return int(); } == 0.

Which does crash.

So the question is - is it valid for element id to be 0, and therefore we need to change the map, or was the input invalid and so we need to check for it?

Ah, it also depends where that element_id came from. Note that it is not the Element's actual underlying id, it is the static id assigned by WeakIdentifierMap::identifier, in the CompositorProxy::CompositorProxy(Element&, const Vector<String>&) constructor.

That id is generated from:

static IdentifierType next() {
  static IdentifierType s_lastId = 0;
  return ++s_lastId;
}

IdentifierType here is the default 'typename IdentifierType = int'.

The returned value is prefix-incremented so doesn't start at 0, but it could overflow and eventually reach 0, which is what I suspect happened.

ajuma@ correctly pointed out that that might not be feasible for the fuzzer. A quick JS test takes about a minute for a million CompositorProxy creations, so (2^32 / 10^6) ==> 71 hours.

It might be more sane to just guard against an id of 0 in nodeForId, but we might miss a bug of some sort.

Oh, ok, the test is literally passing random data into deserialization. So here it managed to create an element id of 0, and we aren't correctly handling that.

I'll fix that.

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

commit e5e975b33027b19dc96bfbab42f09c9c7839c63e
Author: smcgruer <smcgruer@chromium.org>
Date: Thu Mar 09 23:44:13 2017

Check for invalid element id when deserializing CompositorProxyTag

The element id is created from WeakIdentifierMap::next() and should
never be '0'.

BUG= 699484 

Review-Url: https://codereview.chromium.org/2742713004
Cr-Commit-Position: refs/heads/master@{#455901}

[modify] https://crrev.com/e5e975b33027b19dc96bfbab42f09c9c7839c63e/third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp
[modify] https://crrev.com/e5e975b33027b19dc96bfbab42f09c9c7839c63e/third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializerTest.cpp

ClusterFuzz has detected this issue as fixed in range 455883:455909.

Detailed report: https://clusterfuzz.com/testcase?key=5692306406768640

Fuzzer: libfuzzer_v8_serialized_script_value_fuzzer
Job Type: libfuzzer_chrome_asan_debug
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  (HashTableKeyChecker< HashTranslator, KeyTraits, HashFunctions::safeToCompareToE
  blink::WeakIdentifierMap<>::lookup
  blink::DOMNodeIds::nodeForId
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan_debug&range=455091:455226
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan_debug&range=455883:455909

Reproducer Testcase: https://clusterfuzz.com/download/AMIfv95RjiByIHiOR8KfRkIkc4xUSvmqsS10Bj-tvNO-9btL8HQ6oahDnyvijuBwcL4tyjSUvJhDWcW2g3KHeU4ef1BRjTtzEwuZQSptLD7OJJMuiByRdJgyK1YnNF42oNCJQBSRkyVdFQYPNrhvmzzUVxIe4_c_U5Bzjhdz32m1cJWWs7Ea1QUa-V9-n_O6BsvOc7pWFHXZ1tiMybJikGCBysCAjBVKcVSYat59gmYTwdd5sNjMh7Gl_W2eX12qviLzODn52D6j5OvmecoftCWPncKIUTIFeHCIC24mgrhtPowx5ns2nCs3aezFP82t72Tc4OJ8VoORoEP6UKW5hS23ABkmj2xHCjcDBIMXkMAKmfPuhq2Ts9Y?testcase_id=5692306406768640


See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.