Bug 41019 – Canvas: Remember verified clean origins for drawImage()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Andreas Kling]

This canvas game:
http://www.watersheep.org/~markh/html_canvas/game.html

..is doing a ton of drawImage() calls, and 25% of that time is spent checking that the image URL's don't taint the canvas.

Simply caching the KURL's of verified origins helps performance a lot.

[Andreas Kling]

Created attachment 59427 [details]
Proposed patch

[Darin Adler]

Comment on attachment 59427 [details]
Proposed patch

Why is this optimization important? It seems dangerous to cache security decisions because the conditions the security decision was based on could change in theory, so we should only do it if we absolutely need to. How did you determine the optimization was needed? Did this come up in some real world benchmark?

Is the common code path for checkOrigin the KURL one or the String one? I ask because we could easily store strings in the hash instead of URLs and then we could do the fast check before even turning a string into a URL. It's free to turn an URL back into a string with string(), and strings hash even better than URLs do, so I think you should really consider using strings instead of URLs as the hash keys.

> +        ListHashSet<KURL> m_cleanOrigins;

You want HashSet, not ListHashSet. ListHashSet is a more expensive and complex variant of HashSet that guarantees you can get the elements of the set back in the order you added them.

review- because this should be HashSet not ListHashSet.

[Andreas Kling]

Created attachment 59456 [details]
Proposed patch v2

The origin of the canvas's underlying document will not change, so caching verified URL's should be perfectly safe.

The KURL path is the common one, but it indeed makes more sense to cache the Strings.

I've updated the patch to use HashSet<String>, thanks for the feedback.

[Sam Weinig]

(In reply to comment #3)
> Created an attachment (id=59456) [details]
> Proposed patch v2
> 
> The origin of the canvas's underlying document will not change, so caching verified URL's should be perfectly safe.

Are you sure?  What if document.domain is set in between calls to canvas methods.  That could change the tainting the rules I believe.

[Andreas Kling]

(In reply to comment #4)
> Are you sure?  What if document.domain is set in between calls to canvas methods.  That could change the tainting the rules I believe.

From SecurityOrigin::canRequest(KURL):

// We call isSameSchemeHostPort here instead of canAccess because we want
// to ignore document.domain effects.
if (isSameSchemeHostPort(targetOrigin.get()))
    return true;

This is on the path currently taken by canvas's origin check.

[WebKit Commit Bot]

Comment on attachment 59456 [details]
Proposed patch v2

Clearing flags on attachment: 59456

Committed r61877: <http://trac.webkit.org/changeset/61877>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.