HTMLInputElement can delete an ImageLoader while it's still needed
authorschenney@chromium.org <schenney@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 11 Mar 2013 23:18:44 +0000 (23:18 +0000)
committerschenney@chromium.org <schenney@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 11 Mar 2013 23:18:44 +0000 (23:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=110621

Reviewed by Darin Adler.

Source/WebCore:

ImageLoader objects may fire events for HTMLInputElements that are of
type ImageInputType that own the loader. These events may cause script
to run that changes the type of the input element and hence causes the
ImageLoader to be deleted, while the image loader is still processing
the event dispatch. Bad things ensue.

This change moves ownership of the ImageLoader from the ImageInputType
onto the HTMLImageElement which is already protected from deletion during
event processing.

Test: fast/forms/image/image-error-event-modifies-type-crash.html

* html/HTMLInputElement.cpp:
(WebCore::HTMLInputElement::imageLoader): Method to return the
  ImageLoader, creating it if not already created.
* html/HTMLInputElement.h:
(WebCore::HTMLInputElement::hasImageLoader): Return true if the
  ImageLoader has been created.
(HTMLInputElement): Define ImageLoader access methods and the OwnPtr
  for the HTMLImageLoader.
* html/ImageInputType.cpp:
(WebCore::ImageInputType::srcAttributeChanged): Use the element's ImageLoader.
(WebCore::ImageInputType::attach): Use the element's ImageLoader.
(WebCore::ImageInputType::willMoveToNewOwnerDocument): Use the element's ImageLoader.
(WebCore::ImageInputType::height): Use the element's ImageLoader.
(WebCore::ImageInputType::width): Use the element's ImageLoader.
* html/ImageInputType.h:
(ImageInputType): Remove the declaration of the ImageLoader.

LayoutTests:

* fast/forms/image/image-error-event-modifies-type-crash-expected.txt: Added.
* fast/forms/image/image-error-event-modifies-type-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/forms/image/image-error-event-modifies-type-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/forms/image/image-error-event-modifies-type-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/html/HTMLInputElement.cpp
Source/WebCore/html/HTMLInputElement.h
Source/WebCore/html/ImageInputType.cpp
Source/WebCore/html/ImageInputType.h
Source/WebCore/html/InputType.cpp
Source/WebCore/html/InputType.h

index 1c32904..7101de4 100644 (file)
@@ -1,3 +1,13 @@
+2013-03-11  Stephen Chenney  <schenney@chromium.org>
+
+        HTMLInputElement can delete an ImageLoader while it's still needed
+        https://bugs.webkit.org/show_bug.cgi?id=110621
+
+        Reviewed by Darin Adler.
+
+        * fast/forms/image/image-error-event-modifies-type-crash-expected.txt: Added.
+        * fast/forms/image/image-error-event-modifies-type-crash.html: Added.
+
 2013-03-11  Alok Priyadarshi  <alokp@chromium.org>
 
         Revert "Mark GraphicsLayers as opaque when possible"
diff --git a/LayoutTests/fast/forms/image/image-error-event-modifies-type-crash-expected.txt b/LayoutTests/fast/forms/image/image-error-event-modifies-type-crash-expected.txt
new file mode 100644 (file)
index 0000000..10d9c73
--- /dev/null
@@ -0,0 +1 @@
+   Test Passes if it does not crash.
diff --git a/LayoutTests/fast/forms/image/image-error-event-modifies-type-crash.html b/LayoutTests/fast/forms/image/image-error-event-modifies-type-crash.html
new file mode 100644 (file)
index 0000000..7c1d15d
--- /dev/null
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<html>
+  <body>
+    <script>
+      if (window.testRunner) {
+        testRunner.waitUntilDone();
+        testRunner.dumpAsText();
+      }
+      function eventhandler() {
+        inputNode = document.getElementById("imageInput");
+        inputNode.type = "";
+        setTimeout("cleanup()", 10);
+      }
+
+      function cleanup() {
+        if (window.testRunner)
+        {
+            testRunner.notifyDone();
+        }
+      }
+    </script>
+    <input>
+      <input id="imageInput" type="image" onerror="eventhandler()" src="http://127.0.0.1/not-likely-to-be-found.html" />
+    </input>
+    Test Passes if it does not crash.
+  </body>
+</html>
\ No newline at end of file
index c94fc0e..386bf77 100644 (file)
@@ -1,3 +1,39 @@
+2013-03-11  Stephen Chenney  <schenney@chromium.org>
+
+        HTMLInputElement can delete an ImageLoader while it's still needed
+        https://bugs.webkit.org/show_bug.cgi?id=110621
+
+        Reviewed by Darin Adler.
+
+        ImageLoader objects may fire events for HTMLInputElements that are of
+        type ImageInputType that own the loader. These events may cause script
+        to run that changes the type of the input element and hence causes the
+        ImageLoader to be deleted, while the image loader is still processing
+        the event dispatch. Bad things ensue.
+
+        This change moves ownership of the ImageLoader from the ImageInputType
+        onto the HTMLImageElement which is already protected from deletion during
+        event processing.
+
+        Test: fast/forms/image/image-error-event-modifies-type-crash.html
+
+        * html/HTMLInputElement.cpp:
+        (WebCore::HTMLInputElement::imageLoader): Method to return the
+          ImageLoader, creating it if not already created.
+        * html/HTMLInputElement.h:
+        (WebCore::HTMLInputElement::hasImageLoader): Return true if the
+          ImageLoader has been created.
+        (HTMLInputElement): Define ImageLoader access methods and the OwnPtr
+          for the HTMLImageLoader.
+        * html/ImageInputType.cpp:
+        (WebCore::ImageInputType::srcAttributeChanged): Use the element's ImageLoader.
+        (WebCore::ImageInputType::attach): Use the element's ImageLoader.
+        (WebCore::ImageInputType::willMoveToNewOwnerDocument): Use the element's ImageLoader.
+        (WebCore::ImageInputType::height): Use the element's ImageLoader.
+        (WebCore::ImageInputType::width): Use the element's ImageLoader.
+        * html/ImageInputType.h:
+        (ImageInputType): Remove the declaration of the ImageLoader.
+
 2013-03-11  Alok Priyadarshi  <alokp@chromium.org>
 
         Revert "Mark GraphicsLayers as opaque when possible"
index 3a8d8b3..8acabe6 100644 (file)
@@ -47,6 +47,7 @@
 #include "HTMLCollection.h"
 #include "HTMLDataListElement.h"
 #include "HTMLFormElement.h"
+#include "HTMLImageLoader.h"
 #include "HTMLNames.h"
 #include "HTMLOptionElement.h"
 #include "HTMLParserIdioms.h"
@@ -147,6 +148,13 @@ PassRefPtr<HTMLInputElement> HTMLInputElement::create(const QualifiedName& tagNa
     return inputElement.release();
 }
 
+HTMLImageLoader* HTMLInputElement::imageLoader()
+{
+    if (!m_imageLoader)
+        m_imageLoader = adoptPtr(new HTMLImageLoader(this));
+    return m_imageLoader.get();
+}
+
 void HTMLInputElement::didAddUserAgentShadowRoot(ShadowRoot*)
 {
     m_inputType->createShadowSubtree();
@@ -1513,7 +1521,9 @@ void HTMLInputElement::removedFrom(ContainerNode* insertionPoint)
 
 void HTMLInputElement::didMoveToNewDocument(Document* oldDocument)
 {
-    m_inputType->willMoveToNewOwnerDocument();
+    if (hasImageLoader())
+        imageLoader()->elementDidMoveToNewDocument();
+
     bool needsSuspensionCallback = this->needsSuspensionCallback();
     if (oldDocument) {
         // Always unregister for cache callbacks when leaving a document, even if we would otherwise like to be registered
index f7afe52..c93a8f8 100644 (file)
@@ -35,6 +35,7 @@ class CheckedRadioButtons;
 class DragData;
 class FileList;
 class HTMLDataListElement;
+class HTMLImageLoader;
 class HTMLOptionElement;
 class Icon;
 class InputType;
@@ -294,6 +295,9 @@ public:
     virtual void setRangeText(const String& replacement, ExceptionCode&) OVERRIDE;
     virtual void setRangeText(const String& replacement, unsigned start, unsigned end, const String& selectionMode, ExceptionCode&) OVERRIDE;
 
+    bool hasImageLoader() const { return m_imageLoader; }
+    HTMLImageLoader* imageLoader();
+
 #if ENABLE(DATE_AND_TIME_INPUT_TYPES)
     bool setupDateTimeChooserParameters(DateTimeChooserParameters&);
 #endif
@@ -430,6 +434,10 @@ private:
     bool m_hasTouchEventHandler : 1;
 #endif
     OwnPtr<InputType> m_inputType;
+    // The ImageLoader must be owned by this element because the loader code assumes
+    // that it lives as long as its owning element lives. If we move the loader into
+    // the ImageInput object we may delete the loader while this element lives on.
+    OwnPtr<HTMLImageLoader> m_imageLoader;
 #if ENABLE(DATALIST_ELEMENT)
     OwnPtr<ListAttributeTargetObserver> m_listAttributeTargetObserver;
 #endif
index 66325d4..b935df0 100644 (file)
@@ -120,42 +120,32 @@ void ImageInputType::srcAttributeChanged()
 {
     if (!element()->renderer())
         return;
-    if (!m_imageLoader)
-        m_imageLoader = adoptPtr(new HTMLImageLoader(element()));
-    m_imageLoader->updateFromElementIgnoringPreviousError();
+    element()->imageLoader()->updateFromElementIgnoringPreviousError();
 }
 
 void ImageInputType::attach()
 {
     BaseButtonInputType::attach();
 
-    if (!m_imageLoader)
-        m_imageLoader = adoptPtr(new HTMLImageLoader(element()));
-    m_imageLoader->updateFromElement();
+    HTMLImageLoader* imageLoader = element()->imageLoader();
+    imageLoader->updateFromElement();
 
     RenderImage* renderer = toRenderImage(element()->renderer());
     if (!renderer)
         return;
 
-    if (m_imageLoader->hasPendingBeforeLoadEvent())
+    if (imageLoader->hasPendingBeforeLoadEvent())
         return;
 
     RenderImageResource* imageResource = renderer->imageResource();
-    imageResource->setCachedImage(m_imageLoader->image()); 
+    imageResource->setCachedImage(imageLoader->image()); 
 
     // If we have no image at all because we have no src attribute, set
     // image height and width for the alt text instead.
-    if (!m_imageLoader->image() && !imageResource->cachedImage())
+    if (!imageLoader->image() && !imageResource->cachedImage())
         renderer->setImageSizeForAltText();
 }
 
-void ImageInputType::willMoveToNewOwnerDocument()
-{
-    BaseButtonInputType::willMoveToNewOwnerDocument();
-    if (m_imageLoader)
-        m_imageLoader->elementDidMoveToNewDocument();
-}
-
 bool ImageInputType::shouldRespectAlignAttribute()
 {
     return true;
@@ -192,8 +182,11 @@ unsigned ImageInputType::height() const
             return height;
 
         // If the image is available, use its height.
-        if (m_imageLoader && m_imageLoader->image())
-            return m_imageLoader->image()->imageSizeForRenderer(element->renderer(), 1).height();
+        if (element->hasImageLoader()) {
+            HTMLImageLoader* imageLoader = element->imageLoader();
+            if (imageLoader->image())
+                return imageLoader->image()->imageSizeForRenderer(element->renderer(), 1).height();
+        }
     }
 
     element->document()->updateLayout();
@@ -213,8 +206,11 @@ unsigned ImageInputType::width() const
             return width;
 
         // If the image is available, use its width.
-        if (m_imageLoader && m_imageLoader->image())
-            return m_imageLoader->image()->imageSizeForRenderer(element->renderer(), 1).width();
+        if (element->hasImageLoader()) {
+            HTMLImageLoader* imageLoader = element->imageLoader();
+            if (imageLoader->image())
+                return imageLoader->image()->imageSizeForRenderer(element->renderer(), 1).width();
+        }
     }
 
     element->document()->updateLayout();
index 5ec97e4..96d221b 100644 (file)
@@ -39,8 +39,6 @@
 
 namespace WebCore {
 
-class HTMLImageLoader;
-
 class ImageInputType : public BaseButtonInputType {
 public:
     static PassOwnPtr<InputType> create(HTMLInputElement*);
@@ -56,7 +54,6 @@ private:
     virtual void altAttributeChanged() OVERRIDE;
     virtual void srcAttributeChanged() OVERRIDE;
     virtual void attach() OVERRIDE;
-    virtual void willMoveToNewOwnerDocument() OVERRIDE;
     virtual bool shouldRespectAlignAttribute() OVERRIDE;
     virtual bool canBeSuccessfulSubmitButton() OVERRIDE;
     virtual bool isImageButton() const OVERRIDE;
@@ -65,7 +62,6 @@ private:
     virtual unsigned height() const OVERRIDE;
     virtual unsigned width() const OVERRIDE;
 
-    OwnPtr<HTMLImageLoader> m_imageLoader;
     IntPoint m_clickLocation; // Valid only during HTMLFormElement::prepareForSubmission().
 };
 
index d486155..13fa96d 100644 (file)
@@ -596,10 +596,6 @@ void InputType::srcAttributeChanged()
 {
 }
 
-void InputType::willMoveToNewOwnerDocument()
-{
-}
-
 bool InputType::shouldRespectAlignAttribute()
 {
     return false;
index 5878034..8e6e534 100644 (file)
@@ -245,7 +245,6 @@ public:
     virtual void stepAttributeChanged();
     virtual void altAttributeChanged();
     virtual void srcAttributeChanged();
-    virtual void willMoveToNewOwnerDocument();
     virtual bool shouldRespectAlignAttribute();
     virtual FileList* files();
     virtual void setFiles(PassRefPtr<FileList>);