Enable ThreadRestrictionVerifier for StringImpl

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

Index: third_party/WebKit/Source/core/dom/Node.cpp
diff --git a/third_party/WebKit/Source/core/dom/Node.cpp b/third_party/WebKit/Source/core/dom/Node.cpp
index caf5618827c3424f3f6e8939958d3761287ee1f4..4a286877b838a1fcd260c8a7e529575ab1b1505b 100644
--- a/third_party/WebKit/Source/core/dom/Node.cpp
+++ b/third_party/WebKit/Source/core/dom/Node.cpp
@@ -109,6 +109,24 @@
 
 namespace blink {
 
+namespace {
+
+// Unsafe hack to avoid triggering the ThreadRestrictionVerifier on StringImpl.
+// crbug.com/545926 should be fixed completely, and we should always avoid
+// accessing these strings from the impl thread. Currently code that calls into
+// this method from the impl thread tries to make sure that the main thread is
+// not running at this time.
+void appendUnsafe(StringBuilder& builder, const String& offThreadString) {

[esprehn, 2017/01/12 23:00:00]

This needs a TODO and a bug against the paint team. Accessing anything about the
DOM from another thread is a serious bug, calling getIdAttribute() or
classNames() isn't safe either. You can't touch anything across threads.

This seems like an okay hack for now, could we instead do:

builder.append(offThreadString.isolatedCopy()) ?

[haraken, 2017/01/13 04:39:04]

+1 to removing the use case of appendUnsafe.

[Charlie Harrison, 2017/01/13 17:42:25]

I have added the TODO(crbug.com/545926), but haven't removed this method call.

My reasoning is that for a couple of these callsites, we're dealing with
AtomicStrings, so rather than 
appendUnsafe(name, thisElement.classNames()[i]);

We need to write

name.append(StringView(thisElement.classNames()[i].impl()->isolatedCopy()));

I think appendUnsafe is more readable and it doesn't require an extra copy (in
potentially performance sensitive tracing code).

+  StringImpl* impl = offThreadString.impl();
+  if (impl) {
+    builder.append(impl->is8Bit()
+                       ? StringView(impl->characters8(), impl->length())
+                       : StringView(impl->characters16(), impl->length()));
+  }
+}
+
+}  // namespace
+
 using namespace HTMLNames;
 
 struct SameSizeAsNode : EventTarget {
@@ -1488,12 +1506,12 @@ unsigned short Node::compareDocumentPosition(
 
 String Node::debugName() const {
   StringBuilder name;
-  name.append(debugNodeName());
+  appendUnsafe(name, debugNodeName());
   if (isElementNode()) {
     const Element& thisElement = toElement(*this);
     if (thisElement.hasID()) {
       name.append(" id=\'");
-      name.append(thisElement.getIdAttribute());
+      appendUnsafe(name, thisElement.getIdAttribute());
       name.append('\'');
     }
 
@@ -1502,7 +1520,7 @@ String Node::debugName() const {
       for (size_t i = 0; i < thisElement.classNames().size(); ++i) {
         if (i > 0)
           name.append(' ');
-        name.append(thisElement.classNames()[i]);
+        appendUnsafe(name, thisElement.classNames()[i]);
       }
       name.append('\'');
     }