293407 - Canvas frame uses wrong frame type

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We currently have:

layout/generic/nsHTMLFrame.cpp:

621 CanvasFrame::GetType() const
622 {
623   return nsLayoutAtoms::canvasFrame;
624 }

layout/generic/nsHTMLCanvasFrame.cpp:

240 nsHTMLCanvasFrame::GetType() const
241 {
242   return nsLayoutAtoms::canvasFrame;
243 }

That's not good.  Really not good.  You don't want things to think that the
nsHTMLCanvasFrame is the "canvas frame" in Gecko terms.  Please change the atom
it uses, and fix the checks in
nsCSSFrameConstructor::ContentAppended/ContentInserted accordingly.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix for frame type, memory leaks, default size

This patch fixes the layout type issue, also fixes bug 293306 (canvas leaks
lots of memory) and bug 293225 (default height of canvas should be 150).

Also deletes a bunch of obsolete files from our previous <canvas> impl.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 182997 [details] [diff] [review]
fix for frame type, memory leaks, default size

>Index: content/canvas/public/nsICanvasRenderingContextInternal.h

>+  NS_IMETHOD SetCanvasElement(nsIDOMHTMLCanvasElement* aParentCanvas) = 0;

Document that this does NOT hold a ref.

> 
>   NS_IMETHOD SetTargetImageFrame(gfxIImageFrame* aImageFrame) = 0;
>+

No need for that whitespace change.

>Index: content/canvas/src/nsCanvasRenderingContext2D.cpp
>+    // the canvas element informs us when its going away,
>+    // so these are not nsCOMPtrs
>+    nsIDOMHTMLCanvasElement* mDOMCanvasElement;
>+    nsICanvasElement* mCanvasElement;

The constructor should set these to null, I would think.

>+nsCanvasRenderingContext2D::SetCanvasElement(nsIDOMHTMLCanvasElement* aCanvasElement)

>+    if (mDOMCanvasElement) {
>+        if (NS_SUCCEEDED (CallQueryInterface(mDOMCanvasElement, &mCanvasElement))) {
>+            // don't hold a ref to this!
>+            NS_RELEASE(mCanvasElement);

NS_RELEASE sets to null.  You'd want to be calling Release() by hand here.

r+sr=bzbarsky with that, assuming apple does the 150px height thing.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 182997 [details] [diff] [review]
fix for frame type, memory leaks, default size

Fixed all 3 comments; in addition to other 2 bugs, patch also addresses
sicking's comments in 291216, regarding not creating the ImageFrame multiple
times if not necessary (as on pageload).

Comment 4

[Anne (:annevk)]

Shouldn't this bug ask for blocking 1.8b2 or 1.8b3 instead of aviary approval?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Using a pointer after Release() is really bad XPCOM style and does fail in some
instances. Please nuke mDOMCanvasElement and let SetCanvasElement take an
nsICanvasElement instead.

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 182997 [details] [diff] [review]
fix for frame type, memory leaks, default size

a=shaver for trunk (gecko means 1.8b2, not 1.1a, natch).

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in; filed bug 293544 for Release() issue.. will change the interface and
fix shortly.

Comment 8

[Scott MacGregor]

could this checkin have caused the Mac bustage for Thunderbird and Camino that
just showed up?

make[4]: *** No rule to make target `../../dist/lib/libgkconhtmlcon_s.a', needed
by `libgklayout.a'.  Stop.
make[3]: *** [libs] Error 2
make[2]: *** [tier_9] Error 2

The only other change that got checked in when the bustage showed up was windows
only it looks like:

http://tinyurl.com/abxlu

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #8)
> could this checkin have caused the Mac bustage for Thunderbird and Camino that
> just showed up?

I don't /think/ so.. I did delete a few unreferenced files, so that might have
been it (if all the tinderboxes were dep and not clobber).  It looks like
they're back to green now -- any idea if this was at fault?

Comment 10

[Scott MacGregor]

sorry for the false alarm, as you pointed it out it looks like all of the build
machines 'healed' themselves.