Crash in blink::getTypeExtension

third_party/WebKit/Source/core/dom/Document.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  *           (C) 2006 Alexey Proskuryakov (ap@webkit.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2011, 2012 Apple Inc. All
  * rights reserved.
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  * (http://www.torchmobile.com/)
  * Copyright (C) 2008, 2009, 2011, 2012 Google Inc. All rights reserved.
@@ skipping 681 matching lines @@
     UseCounter::count(document,
                       UseCounter::DocumentCreateElement2ndArgStringHandling);
     return stringOrOptions.getAsString();
   }
 
   if (stringOrOptions.isDictionary()) {
     Dictionary dict = stringOrOptions.getAsDictionary();
     ElementCreationOptions impl;
     V8ElementCreationOptions::toImpl(dict.isolate(), dict.v8Value(), impl,
                                      exceptionState);
     if (impl.hasIs())

[sof, 2016/11/26 07:36:30]

Is |exceptionState| intentionally ignored here?

[Anton Obzhirov, 2016/11/27 19:49:53]

Previously getTypeExtension was called in createElement/createElementNS with
unmodified state. Now it is not the case and it is better to do anyway so I will
add the check, thanks for spotting. 

       return impl.is();
 
-    return toCoreString(dict.v8Value()->ToString());
+    v8::Local<v8::String> stringObject;

[sof, 2016/11/26 07:36:30]

The spec says "Let 'is' be the value of |is| member of 'options', or null if no
such member exists", so the need for stringification of the dict is backward
compat motivated, presumably..? Is that a common use of createElement(), why
isn't it being use-counted?

A comment explaining why this is done, would be helpful for someone looking at
the code.

(V8StringResource<> is useful for doing toString() conversions, but I don't yet
fully understand the motivation for this case.)

[Anton Obzhirov, 2016/11/27 19:49:53]

Yes, stringification of the dictionary is for backward compatibility only.
Before introduction of the dictionary the second argument was converted to
string for all cases automatically. I think I missed use-counting here because
technically it wasn't a string argument but you are right, it has to be done
here as well.

For the code to be aligned with the spec the string argument should be
deprecated first but plz correct me if I am wrong.

+    if (dict.v8Value()->IsString()) {
+      stringObject = dict.v8Value().As<v8::String>();
+    } else {
+      v8::TryCatch block(dict.isolate());
+      if (!v8Call(dict.v8Value()->ToString(dict.isolate()->GetCurrentContext()),
+                  stringObject, block)) {
+        exceptionState.rethrowV8Exception(block.Exception());
+        return emptyString();
+      }
+    }
+
+    if (!stringObject.IsEmpty())
+      return toCoreString(stringObject);
   }
 
   return emptyString();
 }
 
 // https://dom.spec.whatwg.org/#dom-document-createelement
 Element* Document::createElement(const AtomicString& localName,
                                  const StringOrDictionary& stringOrOptions,
                                  ExceptionState& exceptionState) {
   // 1. If localName does not match Name production, throw InvalidCharacterError
@@ skipping 5827 matching lines @@
 }
 
 void showLiveDocumentInstances() {
   WeakDocumentSet& set = liveDocumentSet();
   fprintf(stderr, "There are %u documents currently alive:\n", set.size());
   for (Document* document : set)
     fprintf(stderr, "- Document %p URL: %s\n", document,
             document->url().getString().utf8().data());
 }
 #endif