Lazily create HTMLInputElement's inputType and shadow subtree
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Nov 2014 08:21:54 +0000 (08:21 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Nov 2014 08:21:54 +0000 (08:21 +0000)
https://bugs.webkit.org/show_bug.cgi?id=138524

Reviewed by Ryosuke Niwa.

Source/WebCore:

When an HTMLInputElement was created by the parser, we would first call
HTMLInputElement::create(), then call Element::parserSetAttributes() on
the constructed input. With the previous implementation, this was a bit
inefficient because HTMLInputElement::create() would construct a
TextInputType inputType (as this is the default) as well as its
corresponding shadow subtree. Then, parserSetAttributes() would often
set the |type| attribute and would need to destroy this input type as
well as its subtree if the new |type| is not 'text', to create a new
inputType / shadow subtree of the right type. The profiler showed that
this was fairly expensive.

To improve this, this patch delays the inputType / shadow subtree
creation when the HTMLInputElement is constructed by the parser, until
the attributes are actually set by the parser. This way, we directly
create an inputType / shadow subtree of the right type.

I see a 1.4% speed up on speedometer (73.95 -> 75.0).

Test: fast/dom/HTMLInputElement/border-attribute-crash.html

* dom/Element.cpp:
(WebCore::Element::parserSetAttributes):
(WebCore::Element::parserDidSetAttributes):
* dom/Element.h:
* html/HTMLInputElement.cpp:
(WebCore::HTMLInputElement::HTMLInputElement):
(WebCore::HTMLInputElement::create):
(WebCore::HTMLInputElement::updateType):
(WebCore::HTMLInputElement::runPostTypeUpdateTasks):
(WebCore::HTMLInputElement::initializeInputType):
(WebCore::HTMLInputElement::parseAttribute):
(WebCore::HTMLInputElement::parserDidSetAttributes):
(WebCore::HTMLInputElement::finishParsingChildren):
* html/HTMLInputElement.h:

LayoutTests:

Add a test case to make sure we don't crash when parsing an <input>
Element that has a |border| attribute as first attribute. This is what
was happening with the first version of this patch that landed in
r175852. When attributeChanged() was called for the |border| attribute
HTMLInputElement::isPresentationAttribute() would get called before
m_inputType is initialized, and "name == borderAttr && isImageButton()"
would crash because isImageButton() dereferences m_inputType.

* fast/dom/HTMLInputElement/border-attribute-crash-expected.txt: Added.
* fast/dom/HTMLInputElement/border-attribute-crash.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@176069 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/dom/HTMLInputElement/border-attribute-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/HTMLInputElement/border-attribute-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Element.cpp
Source/WebCore/dom/Element.h
Source/WebCore/html/HTMLInputElement.cpp
Source/WebCore/html/HTMLInputElement.h

index 11d96fe..cd6948b 100644 (file)
@@ -1,3 +1,21 @@
+2014-11-13  Chris Dumez  <cdumez@apple.com>
+
+        Lazily create HTMLInputElement's inputType and shadow subtree
+        https://bugs.webkit.org/show_bug.cgi?id=138524
+
+        Reviewed by Ryosuke Niwa.
+
+        Add a test case to make sure we don't crash when parsing an <input>
+        Element that has a |border| attribute as first attribute. This is what
+        was happening with the first version of this patch that landed in
+        r175852. When attributeChanged() was called for the |border| attribute
+        HTMLInputElement::isPresentationAttribute() would get called before
+        m_inputType is initialized, and "name == borderAttr && isImageButton()"
+        would crash because isImageButton() dereferences m_inputType.
+
+        * fast/dom/HTMLInputElement/border-attribute-crash-expected.txt: Added.
+        * fast/dom/HTMLInputElement/border-attribute-crash.html: Added.
+
 2014-11-12  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         Unreviewed GTK gardening. Skip more test failing after r175776.
diff --git a/LayoutTests/fast/dom/HTMLInputElement/border-attribute-crash-expected.txt b/LayoutTests/fast/dom/HTMLInputElement/border-attribute-crash-expected.txt
new file mode 100644 (file)
index 0000000..2b5413c
--- /dev/null
@@ -0,0 +1,13 @@
+Tests that we don't crash when parsing an input element with a border attribute and image type.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS input.getAttribute('border') is "0"
+PASS input.getAttribute('type') is "image"
+PASS input.getAttribute('src') is "submit.gif"
+PASS input.getAttribute('alt') is "Submit"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/HTMLInputElement/border-attribute-crash.html b/LayoutTests/fast/dom/HTMLInputElement/border-attribute-crash.html
new file mode 100644 (file)
index 0000000..0c49839
--- /dev/null
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<body>
+<script src="../../../resources/js-test-pre.js"></script>
+<div id="testDiv"></div>
+<script>
+description("Tests that we don't crash when parsing an input element with a border attribute and image type.");
+
+var testDiv = document.getElementById("testDiv");
+testDiv.innerHTML = "<input border='0' type='image' src='submit.gif' alt='Submit'>";
+
+var input = testDiv.firstChild;
+shouldBeEqualToString("input.getAttribute('border')", "0");
+shouldBeEqualToString("input.getAttribute('type')", "image");
+shouldBeEqualToString("input.getAttribute('src')", "submit.gif");
+shouldBeEqualToString("input.getAttribute('alt')", "Submit");
+
+</script>
+<script src="../../../resources/js-test-post.js"></script>
+</body>
index 70b7b71..d0224d0 100644 (file)
@@ -1,3 +1,45 @@
+2014-11-13  Chris Dumez  <cdumez@apple.com>
+
+        Lazily create HTMLInputElement's inputType and shadow subtree
+        https://bugs.webkit.org/show_bug.cgi?id=138524
+
+        Reviewed by Ryosuke Niwa.
+
+        When an HTMLInputElement was created by the parser, we would first call
+        HTMLInputElement::create(), then call Element::parserSetAttributes() on
+        the constructed input. With the previous implementation, this was a bit
+        inefficient because HTMLInputElement::create() would construct a
+        TextInputType inputType (as this is the default) as well as its
+        corresponding shadow subtree. Then, parserSetAttributes() would often
+        set the |type| attribute and would need to destroy this input type as
+        well as its subtree if the new |type| is not 'text', to create a new
+        inputType / shadow subtree of the right type. The profiler showed that
+        this was fairly expensive.
+
+        To improve this, this patch delays the inputType / shadow subtree
+        creation when the HTMLInputElement is constructed by the parser, until
+        the attributes are actually set by the parser. This way, we directly
+        create an inputType / shadow subtree of the right type.
+
+        I see a 1.4% speed up on speedometer (73.95 -> 75.0).
+
+        Test: fast/dom/HTMLInputElement/border-attribute-crash.html
+
+        * dom/Element.cpp:
+        (WebCore::Element::parserSetAttributes):
+        (WebCore::Element::parserDidSetAttributes):
+        * dom/Element.h:
+        * html/HTMLInputElement.cpp:
+        (WebCore::HTMLInputElement::HTMLInputElement):
+        (WebCore::HTMLInputElement::create):
+        (WebCore::HTMLInputElement::updateType):
+        (WebCore::HTMLInputElement::runPostTypeUpdateTasks):
+        (WebCore::HTMLInputElement::initializeInputType):
+        (WebCore::HTMLInputElement::parseAttribute):
+        (WebCore::HTMLInputElement::parserDidSetAttributes):
+        (WebCore::HTMLInputElement::finishParsingChildren):
+        * html/HTMLInputElement.h:
+
 2014-11-12  Chris Dumez  <cdumez@apple.com>
 
         Have DOMTimer deal with more ScriptExecutionContext references
index 5381bc8..a2d7993 100644 (file)
@@ -1233,17 +1233,23 @@ void Element::parserSetAttributes(const Vector<Attribute>& attributeVector)
     ASSERT(!parentNode());
     ASSERT(!m_elementData);
 
-    if (attributeVector.isEmpty())
-        return;
+    if (!attributeVector.isEmpty()) {
+        if (document().sharedObjectPool())
+            m_elementData = document().sharedObjectPool()->cachedShareableElementDataWithAttributes(attributeVector);
+        else
+            m_elementData = ShareableElementData::createWithAttributes(attributeVector);
 
-    if (document().sharedObjectPool())
-        m_elementData = document().sharedObjectPool()->cachedShareableElementDataWithAttributes(attributeVector);
-    else
-        m_elementData = ShareableElementData::createWithAttributes(attributeVector);
+    }
+
+    parserDidSetAttributes();
 
     // Use attributeVector instead of m_elementData because attributeChanged might modify m_elementData.
-    for (unsigned i = 0; i < attributeVector.size(); ++i)
-        attributeChanged(attributeVector[i].name(), nullAtom, attributeVector[i].value(), ModifiedDirectly);
+    for (const auto& attribute : attributeVector)
+        attributeChanged(attribute.name(), nullAtom, attribute.value(), ModifiedDirectly);
+}
+
+void Element::parserDidSetAttributes()
+{
 }
 
 bool Element::hasAttributes() const
index c82ce97..7aa1beb 100644 (file)
@@ -555,6 +555,7 @@ protected:
     virtual void removedFrom(ContainerNode&) override;
     virtual void childrenChanged(const ChildChange&) override;
     virtual void removeAllEventListeners() override final;
+    virtual void parserDidSetAttributes();
 
     void clearTabIndexExplicitlyIfNeeded();    
     void setTabIndexExplicitly(short);
index 16177ba..d052861 100644 (file)
@@ -121,7 +121,9 @@ HTMLInputElement::HTMLInputElement(const QualifiedName& tagName, Document& docum
 #if ENABLE(TOUCH_EVENTS)
     , m_hasTouchEventHandler(false)
 #endif
-    , m_inputType(InputType::createText(*this))
+    // m_inputType is lazily created when constructed by the parser to avoid constructing unnecessarily a text inputType and
+    // its shadow subtree, just to destroy them when the |type| attribute gets set by the parser to something else than 'text'.
+    , m_inputType(createdByParser ? nullptr : InputType::createText(*this))
 {
     ASSERT(hasTagName(inputTag) || hasTagName(isindexTag));
     setHasCustomStyleResolveCallbacks();
@@ -129,8 +131,10 @@ HTMLInputElement::HTMLInputElement(const QualifiedName& tagName, Document& docum
 
 PassRefPtr<HTMLInputElement> HTMLInputElement::create(const QualifiedName& tagName, Document& document, HTMLFormElement* form, bool createdByParser)
 {
+    bool shouldCreateShadowRootLazily = createdByParser;
     RefPtr<HTMLInputElement> inputElement = adoptRef(new HTMLInputElement(tagName, document, form, createdByParser));
-    inputElement->ensureUserAgentShadowRoot();
+    if (!shouldCreateShadowRootLazily)
+        inputElement->ensureUserAgentShadowRoot();
     return inputElement.release();
 }
 
@@ -432,6 +436,7 @@ void HTMLInputElement::setType(const AtomicString& type)
 
 void HTMLInputElement::updateType()
 {
+    ASSERT(m_inputType);
     auto newType = InputType::create(*this, fastGetAttribute(typeAttr));
     bool hadType = m_hasType;
     m_hasType = true;
@@ -457,17 +462,6 @@ void HTMLInputElement::updateType()
     m_inputType->createShadowSubtree();
     updateInnerTextElementEditability();
 
-#if ENABLE(TOUCH_EVENTS)
-    bool hasTouchEventHandler = m_inputType->hasTouchEventHandler();
-    if (hasTouchEventHandler != m_hasTouchEventHandler) {
-        if (hasTouchEventHandler)
-            document().didAddTouchEventHandler(this);
-        else
-            document().didRemoveTouchEventHandler(this);
-        m_hasTouchEventHandler = hasTouchEventHandler;
-    }
-#endif
-
     setNeedsWillValidateCheck();
 
     bool willStoreValue = m_inputType->storesValueSeparateFromAttribute();
@@ -503,6 +497,23 @@ void HTMLInputElement::updateType()
             attributeChanged(alignAttr, nullAtom, align->value());
     }
 
+    runPostTypeUpdateTasks();
+}
+
+inline void HTMLInputElement::runPostTypeUpdateTasks()
+{
+    ASSERT(m_inputType);
+#if ENABLE(TOUCH_EVENTS)
+    bool hasTouchEventHandler = m_inputType->hasTouchEventHandler();
+    if (hasTouchEventHandler != m_hasTouchEventHandler) {
+        if (hasTouchEventHandler)
+            document().didAddTouchEventHandler(this);
+        else
+            document().didRemoveTouchEventHandler(this);
+        m_hasTouchEventHandler = hasTouchEventHandler;
+    }
+#endif
+
     if (renderer())
         setNeedsStyleRecalc(ReconstructRenderTree);
 
@@ -596,8 +607,29 @@ void HTMLInputElement::collectStyleForPresentationAttribute(const QualifiedName&
         HTMLTextFormControlElement::collectStyleForPresentationAttribute(name, value, style);
 }
 
+inline void HTMLInputElement::initializeInputType()
+{
+    ASSERT(m_parsingInProgress);
+    ASSERT(!m_inputType);
+
+    const AtomicString& type = fastGetAttribute(typeAttr);
+    if (type.isNull()) {
+        m_inputType = InputType::createText(*this);
+        ensureUserAgentShadowRoot();
+        return;
+    }
+
+    m_hasType = true;
+    m_inputType = InputType::create(*this, type);
+    ensureUserAgentShadowRoot();
+    registerForSuspensionCallbackIfNeeded();
+    runPostTypeUpdateTasks();
+}
+
 void HTMLInputElement::parseAttribute(const QualifiedName& name, const AtomicString& value)
 {
+    ASSERT(m_inputType);
+
     if (name == nameAttr) {
         removeFromRadioButtonGroup();
         m_name = value;
@@ -705,9 +737,16 @@ void HTMLInputElement::parseAttribute(const QualifiedName& name, const AtomicStr
     m_inputType->attributeChanged();
 }
 
+void HTMLInputElement::parserDidSetAttributes()
+{
+    ASSERT(m_parsingInProgress);
+    initializeInputType();
+}
+
 void HTMLInputElement::finishParsingChildren()
 {
     m_parsingInProgress = false;
+    ASSERT(m_inputType);
     HTMLTextFormControlElement::finishParsingChildren();
     if (!m_stateRestored) {
         bool checked = fastHasAttribute(checkedAttr);
index f045736..0282479 100644 (file)
@@ -359,6 +359,7 @@ private:
     virtual bool isPresentationAttribute(const QualifiedName&) const override final;
     virtual void collectStyleForPresentationAttribute(const QualifiedName&, const AtomicString&, MutableStyleProperties&) override final;
     virtual void finishParsingChildren() override final;
+    virtual void parserDidSetAttributes() override final;
 
     virtual void copyNonAttributePropertiesFromElement(const Element&) override final;
 
@@ -398,7 +399,9 @@ private:
     virtual bool recalcWillValidate() const override final;
     virtual void requiredAttributeChanged() override final;
 
+    void initializeInputType();
     void updateType();
+    void runPostTypeUpdateTasks();
     
     virtual void subtreeHasChanged() override final;