Summary: | Canvas: Remember verified clean origins for drawImage() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | benjamin, commit-queue, sam | ||||||
Priority: | P2 | Keywords: | Performance | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Andreas Kling
2010-06-22 16:02:27 PDT
Created attachment 59427 [details]
Proposed patch
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. 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.
(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. (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. Comment on attachment 59456 [details] Proposed patch v2 Clearing flags on attachment: 59456 Committed r61877: <http://trac.webkit.org/changeset/61877> All reviewed patches have been landed. Closing bug. |