Changeset 145423 in webkit

Timestamp:

Mar 11, 2013, 4:18:44 PM (12 years ago)

Author:

schenney@chromium.org

Message:

HTMLInputElement can delete an ImageLoader while it's still needed
​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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/forms/image/image-error-event-modifies-type-crash-expected.txt (added)
• LayoutTests/fast/forms/image/image-error-event-modifies-type-crash.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/html/HTMLInputElement.cpp (modified) (3 diffs)
• Source/WebCore/html/HTMLInputElement.h (modified) (3 diffs)
• Source/WebCore/html/ImageInputType.cpp (modified) (5 diffs)
• Source/WebCore/html/ImageInputType.h (modified) (3 diffs)
• Source/WebCore/html/InputType.cpp (modified) (1 diff)
• Source/WebCore/html/InputType.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/html/HTMLInputElement.cpp

@@ -48 +48 @@
 #include "HTMLDataListElement.h"
 #include "HTMLFormElement.h"
+#include "HTMLImageLoader.h"
 #include "HTMLNames.h"
 #include "HTMLOptionElement.h"
@@ -146 +147 @@
     inputElement->ensureUserAgentShadowRoot();
     return inputElement.release();
+}
+
+HTMLImageLoader* HTMLInputElement::imageLoader()
+{
+    if (!m_imageLoader)
+        m_imageLoader = adoptPtr(new HTMLImageLoader(this));
+    return m_imageLoader.get();
 }
 
@@ -1514 +1522 @@
 void HTMLInputElement::didMoveToNewDocument(Document* oldDocument)
 {
-    m_inputType->willMoveToNewOwnerDocument();
+    if (hasImageLoader())
+        imageLoader()->elementDidMoveToNewDocument();
+
     bool needsSuspensionCallback = this->needsSuspensionCallback();
     if (oldDocument) {

trunk/Source/WebCore/html/HTMLInputElement.h

@@ -36 +36 @@
 class FileList;
 class HTMLDataListElement;
+class HTMLImageLoader;
 class HTMLOptionElement;
 class Icon;
@@ -294 +295 @@
     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)
@@ -431 +435 @@
 #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;

trunk/Source/WebCore/html/ImageInputType.cpp

@@ -121 +121 @@
     if (!element()->renderer())
         return;
-    if (!m_imageLoader)
-        m_imageLoader = adoptPtr(new HTMLImageLoader(element()));
-    m_imageLoader->updateFromElementIgnoringPreviousError();
+    element()->imageLoader()->updateFromElementIgnoringPreviousError();
 }
 
@@ -130 +128 @@
     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());
@@ -138 +135 @@
         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();
 }
 
@@ -193 +183 @@
 
         // 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();
+        }
     }
 
@@ -214 +207 @@
 
         // 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();
+        }
     }
 

trunk/Source/WebCore/html/ImageInputType.h

@@ -40 +40 @@
 namespace WebCore {
 
-class HTMLImageLoader;
-
 class ImageInputType : public BaseButtonInputType {
 public:
@@ -57 +55 @@
     virtual void srcAttributeChanged() OVERRIDE;
     virtual void attach() OVERRIDE;
-    virtual void willMoveToNewOwnerDocument() OVERRIDE;
     virtual bool shouldRespectAlignAttribute() OVERRIDE;
     virtual bool canBeSuccessfulSubmitButton() OVERRIDE;
@@ -66 +63 @@
     virtual unsigned width() const OVERRIDE;
 
-    OwnPtr<HTMLImageLoader> m_imageLoader;
     IntPoint m_clickLocation; // Valid only during HTMLFormElement::prepareForSubmission().
 };

trunk/Source/WebCore/html/InputType.cpp

@@ -597 +597 @@
 }
 
-void InputType::willMoveToNewOwnerDocument()
-{
-}
-
 bool InputType::shouldRespectAlignAttribute()
 {

trunk/Source/WebCore/html/InputType.h

@@ -246 +246 @@
     virtual void altAttributeChanged();
     virtual void srcAttributeChanged();
-    virtual void willMoveToNewOwnerDocument();
     virtual bool shouldRespectAlignAttribute();
     virtual FileList* files();