b=293407, canvas uses wrong frame type; 293306, canvas leaks memory; 293225, default canvas height should be 150, r+sr=bzbarsky,a=shaver

content/canvas/public/nsICanvasRenderingContextInternal.h

@@ -50,7 +50,9 @@ class nsICanvasRenderingContextInternal : public nsISupports {
 public:
   NS_DEFINE_STATIC_IID_ACCESSOR(NS_ICANVASRENDERINGCONTEXTINTERNAL_IID)
 
-  NS_IMETHOD Init(nsIDOMHTMLCanvasElement* aParentCanvas) = 0;
+  // This method should NOT hold a ref to aParentCanvas; it will be called
+  // with nsnull when the element is going away.
+  NS_IMETHOD SetCanvasElement(nsIDOMHTMLCanvasElement* aParentCanvas) = 0;
 
   NS_IMETHOD SetTargetImageFrame(gfxIImageFrame* aImageFrame) = 0;
 };

content/canvas/src/nsCanvasRenderingContext2D.cpp

@@ -185,7 +185,7 @@ public:
     void SetCairoColor(nscolor c);
 
     // nsICanvasRenderingContextInternal
-    NS_IMETHOD Init (nsIDOMHTMLCanvasElement* aParentCanvas);
+    NS_IMETHOD SetCanvasElement(nsIDOMHTMLCanvasElement* aParentCanvas);
     NS_IMETHOD SetTargetImageFrame(gfxIImageFrame* aImageFrame);
 
     // nsISupports interface
@@ -216,8 +216,10 @@ protected:
     // Member vars
     PRInt32 mWidth, mHeight;
 
-    nsCOMPtr<nsIDOMHTMLCanvasElement> mDOMCanvasElement;
+    // the canvas element informs us when its going away,
-    nsCOMPtr<nsICanvasElement> mCanvasElement;
+    // so these are not nsCOMPtrs
+    nsIDOMHTMLCanvasElement* mDOMCanvasElement;
+    nsICanvasElement* mCanvasElement;
 
     // image bits
     nsCOMPtr<gfxIImageFrame> mImageFrame;
@@ -280,7 +282,8 @@ NS_NewCanvasRenderingContext2D(nsIDOMCanvasRenderingContext2D** aResult)
 }
 
 nsCanvasRenderingContext2D::nsCanvasRenderingContext2D()
-    : mDirty(PR_TRUE), mCairo(nsnull), mSurface(nsnull), mSurfaceData(nsnull)
+    : mDOMCanvasElement(nsnull), mCanvasElement(nsnull),
+      mDirty(PR_TRUE), mCairo(nsnull), mSurface(nsnull), mSurfaceData(nsnull)
 {
     mColorStyles[STYLE_STROKE] = NS_RGB(0,0,0);
     mColorStyles[STYLE_FILL] = NS_RGB(255,255,255);
@@ -300,7 +303,7 @@ nsCanvasRenderingContext2D::~nsCanvasRenderingContext2D()
 nsIFrame*
 nsCanvasRenderingContext2D::GetCanvasLayoutFrame()
 {
-    if (!mDOMCanvasElement)
+    if (!mCanvasElement)
         return nsnull;
 
     nsIFrame *fr = nsnull;
@@ -606,12 +609,20 @@ nsCanvasRenderingContext2D::UpdateImageFrame()
 //
 
 NS_IMETHODIMP
-nsCanvasRenderingContext2D::Init(nsIDOMHTMLCanvasElement* aCanvasElement)
+nsCanvasRenderingContext2D::SetCanvasElement(nsIDOMHTMLCanvasElement* aCanvasElement)
 {
+    // don't hold a ref to this!
     mDOMCanvasElement = aCanvasElement;
-    mCanvasElement = do_QueryInterface(mDOMCanvasElement);
+    if (mDOMCanvasElement) {
+        if (NS_SUCCEEDED (CallQueryInterface(mDOMCanvasElement, &mCanvasElement))) {
+            // don't hold a ref to this!
+            mCanvasElement->Release();
+        }
+    } else {
+        mCanvasElement = nsnull;
+    }
 
-    // set up our css parser
+    // set up our css parser, if necessary
     if (!mCSSParser) {
         mCSSParser = do_CreateInstance("@mozilla.org/content/css-parser;1");
     }

content/html/content/src/nsHTMLCanvasElement.cpp

@@ -54,7 +54,7 @@
 #include "nsICanvasRenderingContextInternal.h"
 
 #define DEFAULT_CANVAS_WIDTH 300
-#define DEFAULT_CANVAS_HEIGHT 200
+#define DEFAULT_CANVAS_HEIGHT 150
 
 class nsHTMLCanvasElement : public nsGenericHTMLElement,
                             public nsIDOMHTMLCanvasElement,
@@ -100,7 +100,7 @@ public:
                            PRBool aNotify);
 protected:
   nsIntSize GetWidthHeight();
-  nsresult UpdateImageContainer();
+  nsresult UpdateImageContainer(PRBool forceCreate);
   nsresult UpdateContext();
 
   nsString mCurrentContextId;
@@ -123,6 +123,11 @@ nsHTMLCanvasElement::nsHTMLCanvasElement(nsINodeInfo *aNodeInfo)
 
 nsHTMLCanvasElement::~nsHTMLCanvasElement()
 {
+  if (mCurrentContext) {
+    nsCOMPtr<nsICanvasRenderingContextInternal> internalctx(do_QueryInterface(mCurrentContext));
+    internalctx->SetCanvasElement(nsnull);
+    mCurrentContext = nsnull;
+  }
 }
 
 NS_IMPL_ADDREF_INHERITED(nsHTMLCanvasElement, nsGenericElement)
@@ -176,7 +181,7 @@ nsHTMLCanvasElement::SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
   if (NS_SUCCEEDED(rv) && mCurrentContext &&
       (aName == nsHTMLAtoms::width || aName == nsHTMLAtoms::height))
   {
-    rv = UpdateImageContainer();
+    rv = UpdateImageContainer(PR_FALSE);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
@@ -283,10 +288,10 @@ nsHTMLCanvasElement::GetContext(const nsAString& aContextId,
       return NS_ERROR_FAILURE;
     }
 
-    rv = internalctx->Init(this);
+    rv = internalctx->SetCanvasElement(this);
     NS_ENSURE_SUCCESS(rv, rv);
 
-    rv = UpdateImageContainer();
+    rv = UpdateImageContainer(PR_TRUE);
     NS_ENSURE_SUCCESS(rv, rv);
 
     mCurrentContextId.Assign(aContextId);
@@ -300,10 +305,15 @@ nsHTMLCanvasElement::GetContext(const nsAString& aContextId,
 }
 
 nsresult
-nsHTMLCanvasElement::UpdateImageContainer()
+nsHTMLCanvasElement::UpdateImageContainer(PRBool forceCreate)
 {
   nsresult rv = NS_OK;
 
+  // don't create if we don't already have one,
+  // and no frame or context has asked for one.
+  if (!forceCreate && !mImageFrame)
+    return NS_OK;
+
   nsIntSize sz = GetWidthHeight();
   PRInt32 w = 0, h = 0;
 
@@ -354,7 +364,7 @@ nsHTMLCanvasElement::GetCanvasImageContainer (imgIContainer **aImageContainer)
   nsresult rv;
 
   if (!mImageContainer) {
-    rv = UpdateImageContainer();
+    rv = UpdateImageContainer(PR_TRUE);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 

layout/base/nsCSSFrameConstructor.cpp

@@ -8488,7 +8488,7 @@ nsCSSFrameConstructor::ContentAppended(nsIContent*     aContainer,
   nsIAtom* frameType = parentFrame->GetType();
   if (frameType == nsLayoutAtoms::objectFrame ||
       frameType == nsLayoutAtoms::tableColFrame ||
-      frameType == nsLayoutAtoms::canvasFrame) {
+      frameType == nsLayoutAtoms::HTMLCanvasFrame) {
     // This handles APPLET, EMBED, OBJECT, COL, and CANVAS
     return NS_OK;
   }
@@ -9076,7 +9076,7 @@ nsCSSFrameConstructor::ContentInserted(nsIContent*            aContainer,
     nsIAtom* frameType = parentFrame->GetType();
     if (frameType == nsLayoutAtoms::objectFrame ||
         frameType == nsLayoutAtoms::tableColFrame ||
-        frameType == nsLayoutAtoms::canvasFrame) {
+        frameType == nsLayoutAtoms::HTMLCanvasFrame) {
       // This handles APPLET, EMBED, OBJECT, COL, and CANVAS
       return NS_OK;
     }

layout/base/nsLayoutAtomList.h

@@ -100,6 +100,7 @@ LAYOUT_ATOM(bulletFrame, "BulletFrame")
 LAYOUT_ATOM(columnSetFrame, "ColumnSetFrame")
 LAYOUT_ATOM(fieldSetFrame, "FieldSetFrame")
 LAYOUT_ATOM(gfxButtonControlFrame, "gfxButtonControlFrame")
+LAYOUT_ATOM(HTMLCanvasFrame, "HTMLCanvasFrame")
 LAYOUT_ATOM(subDocumentFrame, "subDocumentFrame")
 LAYOUT_ATOM(imageBoxFrame, "ImageBoxFrame")
 LAYOUT_ATOM(imageFrame, "ImageFrame")

layout/generic/nsFrame.cpp

@@ -6225,6 +6225,7 @@ void DR_State::InitFrameTypeTable()
   AddFrameTypeInfo(nsLayoutAtoms::brFrame,               "br",        "br");
   AddFrameTypeInfo(nsLayoutAtoms::bulletFrame,           "bullet",    "bullet");
   AddFrameTypeInfo(nsLayoutAtoms::gfxButtonControlFrame, "button",    "gfxButtonControl");
+  AddFrameTypeInfo(nsLayoutAtoms::HTMLCanvasFrame,       "HTMLCanvas","HTMLCanvas");
   AddFrameTypeInfo(nsLayoutAtoms::subDocumentFrame,      "subdoc",    "subDocument");
   AddFrameTypeInfo(nsLayoutAtoms::imageFrame,            "img",       "image");
   AddFrameTypeInfo(nsLayoutAtoms::inlineFrame,           "inline",    "inline");

layout/generic/nsHTMLCanvasFrame.cpp

@@ -239,7 +239,7 @@ nsHTMLCanvasFrame::GetContentForEvent(nsPresContext* aPresContext,
 nsIAtom*
 nsHTMLCanvasFrame::GetType() const
 {
-  return nsLayoutAtoms::canvasFrame;
+  return nsLayoutAtoms::HTMLCanvasFrame;
 }
 
 // get the offset into the content area of the image where aImg starts if it is a continuation.