Bug 106385 - CanvasRenderingContext2D::setFont argument may reference destroyed object
Summary: CanvasRenderingContext2D::setFont argument may reference destroyed object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Novosad
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-08 14:50 PST by Justin Novosad
Modified: 2013-01-08 18:31 PST (History)
4 users (show)

See Also:


Attachments
Patch (2.27 KB, patch)
2013-01-08 14:53 PST, Justin Novosad
no flags Details | Formatted Diff | Diff
Patch for landing (2.32 KB, patch)
2013-01-08 15:44 PST, Justin Novosad
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Novosad 2013-01-08 14:50:04 PST
CanvasRenderingContext2D::setFont argument may reference destroyed object
Comment 1 Justin Novosad 2013-01-08 14:53:33 PST
Created attachment 181773 [details]
Patch
Comment 2 Abhishek Arya 2013-01-08 15:04:06 PST
Comment on attachment 181773 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181773&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2091
> +    String newFontSafeCopy(newFont); // In case newFont is a ref to a string touched by realizeSaves

The comment could be improved like "Create a string copy since newFont can be deleted inside realizeSaves."
Comment 3 Darin Adler 2013-01-08 15:19:01 PST
Comment on attachment 181773 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181773&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2093
> +    modifiableState().m_unparsedFont = newFontSafeCopy;

If this was a RefPtr then we’d want to do a release here to avoid reference count churn.

Since it’s a String, we *could* avoid the churn by doing a swap here instead of assignment. But I’m thinking that’s too ugly for the tiny performance win.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2379
>      if (!state().m_realizedFont) {
> -        // Create temporary string object to hold ref count in case
> -        // state().m_unparsedFont in unreffed by call to realizeSaves in
> -        // setFont.
> -        String unparsedFont(state().m_unparsedFont);
> -        setFont(unparsedFont);
> +        setFont(state().m_unparsedFont);
>      }

Should take out the braces here too.
Comment 4 Justin Novosad 2013-01-08 15:42:52 PST
(In reply to comment #3)

> Should take out the braces here too.

I wonder how come this passed the style check...
Comment 5 Justin Novosad 2013-01-08 15:44:53 PST
Created attachment 181792 [details]
Patch for landing
Comment 6 WebKit Review Bot 2013-01-08 18:31:24 PST
Comment on attachment 181792 [details]
Patch for landing

Clearing flags on attachment: 181792

Committed r139144: <http://trac.webkit.org/changeset/139144>
Comment 7 WebKit Review Bot 2013-01-08 18:31:27 PST
All reviewed patches have been landed.  Closing bug.