Closed
Bug 438695
Opened 16 years ago
Closed 16 years ago
Canvas's SetFont crashes when no frame is present [@ std::vector<unsigned short, std::allocator<unsigned short> >::_M_fill_insert]
Categories
(Core :: Graphics: Canvas2D, defect)
Core
Graphics: Canvas2D
Tracking
()
VERIFIED
FIXED
mozilla1.9.1a1
People
(Reporter: zpao, Assigned: ebutler)
Details
(Keywords: crash)
Crash Data
Attachments
(3 files, 2 obsolete files)
1.71 KB,
text/html
|
Details | |
16.89 KB,
patch
|
Details | Diff | Splinter Review | |
5.19 KB,
patch
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
OS: Mac OS X → All
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → ebutler
Status: ASSIGNED → NEW
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•16 years ago
|
||
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.
Summary: Use of canvas crashes Minefield → Canvas's SetFont crashes when no frame is present
Assignee | ||
Comment 2•16 years ago
|
||
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.
Attachment #325000 -
Flags: review?(dbaron)
(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 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.
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.)
Assignee | ||
Comment 6•16 years ago
|
||
fixed all the suggestions.
Attachment #325000 -
Attachment is obsolete: true
Attachment #325275 -
Flags: review?(dbaron)
Attachment #325000 -
Flags: review?(dbaron)
Assignee | ||
Comment 7•16 years ago
|
||
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.
Attachment #325275 -
Attachment is obsolete: true
Attachment #325275 -
Flags: review?(dbaron)
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.
Oh, and the test files had windows line endings on them, which I fixed.
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.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1
Comment 11•16 years ago
|
||
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
Severity: normal → critical
Status: RESOLVED → VERIFIED
Keywords: crash
Target Milestone: mozilla1.9.1 → mozilla1.9.1a1
Updated•16 years ago
|
Summary: Canvas's SetFont crashes when no frame is present → Canvas's SetFont crashes when no frame is present [@ std::vector<unsigned short, std::allocator<unsigned short> >::_M_fill_insert]
Assignee | ||
Comment 12•16 years ago
|
||
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•16 years ago
|
||
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?
Updated•13 years ago
|
Crash Signature: [@ std::vector<unsigned short, std::allocator<unsigned short> >::_M_fill_insert]
You need to log in
before you can comment on or make changes to this bug.
Description
•