438695 - Canvas's SetFont crashes when no frame is present [@ std::vector<unsigned short, std::allocator<unsigned short> >::_M_fill_insert]

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Basic Canvas animation

Running test case that works in Firefox 3 crashes nightly build.

STR:
1. Open Minefield
2. Open attachment (and other examples from Rob)
3. Watch crash.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a1pre) Gecko/2008061102 Minefield/3.1a1pre

Comment 1

[Eric Butler [:ebutler]]

I have found the cause of the problem in nsCanvasRenderingContext2D::SetFont. The function erroneously assumes that the frame of the canvas is non-null. Cases need to be added for when the frame cannot be found.

Comment 2

[Eric Butler [:ebutler]]

Attachment: version 1.0

Fixes the crashing bug as well as correcting the implementation to more closely follow the spec.

There was an additional problem that I am not sure I'm handling correctly. In drawText, the function needs to resolve the "direction" property of the canvas, which it was formerly getting from nsIFrame::GetStyleVisibility. Now it uses that and nsIContent::GetParent to climb the tree until it finds a frame. It seems to work correctly, but I'm not sure that it's the correct and clean way to do that.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #2)
> There was an additional problem that I am not sure I'm handling correctly. In
> drawText, the function needs to resolve the "direction" property of the canvas,
> which it was formerly getting from nsIFrame::GetStyleVisibility. Now it uses
> that and nsIContent::GetParent to climb the tree until it finds a frame. It
> seems to work correctly, but I'm not sure that it's the correct and clean way
> to do that.

http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2008-June/015075.html

You probably want to use nsInspectorCSSUtils::GetStyleContextForContent here as well instead of what you're currently doing for handling the case of in-document but no frame.  The question is what to do for the not-in-document case.


For what it's worth, for some previous discussion related to this bug, see:
http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2008-June/015056.html
http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2008-June/015067.html

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 325000 [details] [diff] [review]
version 1.0

>+static nsresult
>+CreateFontStyleRule(const nsAString& font,
>+                    nsICSSParser* cssParser,
>+                    nsINode* node,
>+                    nsCOMPtr<nsICSSStyleRule>* ptr)

nsCOMPtr parameters are potentially confusing.  You should either:
 * use an "nsICSSStyleRule **aResult" out parameter, or
 * drop the nsresult, and return already_AddRefed<nsICSSStyleRule>.

I'd also note that local convention is to name arguments beginning with a, followed by a capital letter.

It also seems like this could be a member function; then it wouldn't need the css parser or node parameters.

>+    rv = cssParser->ParseStyleAttribute(
>+            EmptyString(),
>+            docURL,
>+            baseURL,
>+            principal,
>+            getter_AddRefs(rule));
>+    if (NS_FAILED(rv))
>+        return rv;
>+    if (!rule)
>+        return NS_ERROR_FAILURE;

This will never return null with a success result, so you don't need the second check.

>+NS_IMETHODIMP
>+nsCanvasRenderingContext2D::SetFont(const nsAString& font)
>+{
>+    nsresult rv;
>+
>+    /*
>+    If font is defined with relative units (e.g. ems) and the parent
>+    style context changes in between calls, setting the font to the
>+    same value as previous could result in a different computed value,
>+    so we cannot have the optimization where we check if the new font
>+    string is equal to the old one.
>+    */

Mozilla style for comments tends to use a * at the beginning of each line (or to use // comments).

>+    nsIDocument* document = content->GetOwnerDoc();
>+
>+    if (!document) {
>+        NS_WARNING("Element is missing document or principal");
>+        return NS_ERROR_FAILURE;
>+    }

GetOwnerDoc is always non-null, so you don't need a runtime check.  You can have an assertion if you really want it, but the text should be correct.

>-    nsStyleSet *styleSet = presShell->StyleSet();
>+    nsStyleSet* styleSet = presShell->StyleSet();
>+    if (!styleSet)
>+        return NS_ERROR_FAILURE;

You shouldn't need to null-check this either.  (Getters without a "Get" at the start are our new way of indicating this that don't need null-checking, but we haven't switched everything over to that convention yet.)

>+    // have to get a parent style context
>+    nsRefPtr<nsStyleContext> parentContext;
>+
>+    if (content->IsInDoc()) {
>+        // try to find the closest context
>+        parentContext = nsInspectorCSSUtils::GetStyleContextForContent(
>+               content,
>+               nsnull,
>+               presShell);
>+    } 
>+    
>+    if (!parentContext) {

this should probably be an else; if GetStyleContextForContent fails we should give up.

>+        // otherwise create a default style context (10px sans-serif)

"default style context" doesn't make sense to me; maybe just say:

// otherwise inherit from the default (10px sans-serif)

>@@ -1810,21 +1890,44 @@

Could you add -p (or showfunc=1 in the [diff] section of ~/.hgrc) to the diff arguments so function names are shown here?

>+    nsIDocument* document = contentPtr->GetOwnerDoc();
>+
>+    if (!document) {
>+        NS_WARNING("Element is missing document or principal");
>+        return NS_ERROR_FAILURE;
>+    }

Same comment here as above.

>+<canvas id="c" width="512" height="256" style="direction: rtl;"/>

In HTML (as opposed to XHTML), you can't use /> to close an element that can have children; you need a proper end-tag.  What you're doing now is making the script the child of the canvas, which is a bit odd.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

You might also want to test being inside a display:none div in addition to having the canvas itself be display:none.  (In the latter case, we have a style context in the undisplayed map, although GetStyleContextForContent won't actually use that; in the former case, we have to create more style cotnexts.)

Comment 6

[Eric Butler [:ebutler]]

Attachment: version 1.1

fixed all the suggestions.

Comment 7

[Eric Butler [:ebutler]]

Attachment: version 1.2

fixed a typo in one of the reftests
added in a diff to change the uuid of nsIDOMCanvasRenderingContext2D since it was forgotten in bug 436904.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: review comments

These are my review comments on attachment 325349 [details] [diff] [review] in the form of a patch.  In particular:
 * aCssParser -> aCSSParser to follow convention elsewhere (and in the class name)
 * fixed indentation lining up in calls to ParseProperty
 * used nsCOMPtr<T>::forget rather than extra AddRef/Release
 * made comment about needing parent clearer
 * corrected comment about "closest context"
 * renamed parentContext to canvasStyle in drawText, since it's not being used as a parent context.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, and the test files had windows line endings on them, which I fixed.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/1f58eff34a77
http://hg.mozilla.org/mozilla-central/index.cgi/rev/72f8cdcfa34a

Marking fixed.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008071203 Minefield/3.1a1pre ID:2008071203

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a1pre) Gecko/2008071202 Minefield/3.1a1pre ID:2008071202

Comment 12

[Eric Butler [:ebutler]]

Er... why did you change the title of the bug? The crash was not occurring at that function. STL wasn't even being used. This bug was already resolved; are you referring to some other problem?

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Eric, I ran the given testcase with a build before your patch was checked-in. Firefox crashed with following crash id: bp-36765857-5052-11dd-bd90-001a4bd43ed6

A current build doesn't crash. Due to no top stack frame was listed within the summary I believed that this is the correct stack. Sorry, if this isn't so. Do you have a real crash id so we can add the correct top frame? 

Paul, could you also please recheck with an older build?