Adding cache for security origin validations in 2D/WebGL canvases

Adding cache for security origin validations in 2D/WebGL canvases

There used to be a similar cache before r168864. The old cache
system did not survive the refactoring, which caused a measurable
performance degradation on several mobile platforms.  This change
reinstates it in a refactored form.

BUG=354565
TEST=jsgamebench telemetry on android

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174995

[Justin Novosad, 2014-05-26 21:44:36]

PTAL

[Stephen White, 2014-05-27 19:02:11]

Looks good with the naming nit fixed. The others are at your discretion.

https://codereview.chromium.org/297293004/diff/20001/Source/core/html/HTMLIma...
File Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/297293004/diff/20001/Source/core/html/HTMLIma...
Source/core/html/HTMLImageElement.h:96: virtual KURL sourceURL() const OVERRIDE;
I was wondering whether this could be a const KURL&, but perhaps the
CanvasImageSource impl of this function would make that difficult.

https://codereview.chromium.org/297293004/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext.cpp (right):

https://codereview.chromium.org/297293004/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext.cpp:39: bool
CanvasRenderingContext::isKnownCleanURL(const KURL& url) const
"Clean" doesn't seem to fully capture the meaning here. Could we use
isKnownOriginCleanURL? Would that make sense in this context?

https://codereview.chromium.org/297293004/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/297293004/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1705: if (!originClean) {
Not that it really matters, but I think this could could also be:

if (!originClean && !imageSource->wouldTaintOrigin(canvas()->securityOrigin()))
{
    addKnownCleanURL(imageSource->sourceURL());
    originClean = true;
}

[Justin Novosad, 2014-05-27 19:48:39]

https://codereview.chromium.org/297293004/diff/20001/Source/core/html/HTMLIma...
File Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/297293004/diff/20001/Source/core/html/HTMLIma...
Source/core/html/HTMLImageElement.h:96: virtual KURL sourceURL() const OVERRIDE;
On 2014/05/27 19:02:12, Stephen White wrote:
> I was wondering whether this could be a const KURL&, but perhaps the
> CanvasImageSource impl of this function would make that difficult.

Exactly. You can't return a temporary by address. I looked a little deeper and
it turns out that the KURL copy constructor is not *that* lightweight. there is
a static &KURL blankURL() helper that I could use here. It is not equivalent
("about://blank") but I think we should use that.

https://codereview.chromium.org/297293004/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext.cpp (right):

https://codereview.chromium.org/297293004/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext.cpp:39: bool
CanvasRenderingContext::isKnownCleanURL(const KURL& url) const
On 2014/05/27 19:02:12, Stephen White wrote:
> "Clean" doesn't seem to fully capture the meaning here. Could we use
> isKnownOriginCleanURL? Would that make sense in this context?

That name is confusing IMO. The question this method answers is whether this
canvas will remain origin clean after receiving content from the given URL. The
source can be origin clean and still taint the destination canvas.

What we want is a more compact form of: isURLKnownToNotTaintThisCanvas

[Stephen White, 2014-05-27 20:06:15]

On 2014/05/27 19:48:39, junov wrote:
>
https://codereview.chromium.org/297293004/diff/20001/Source/core/html/HTMLIma...
> File Source/core/html/HTMLImageElement.h (right):
> 
>
https://codereview.chromium.org/297293004/diff/20001/Source/core/html/HTMLIma...
> Source/core/html/HTMLImageElement.h:96: virtual KURL sourceURL() const
OVERRIDE;
> On 2014/05/27 19:02:12, Stephen White wrote:
> > I was wondering whether this could be a const KURL&, but perhaps the
> > CanvasImageSource impl of this function would make that difficult.
> 
> Exactly. You can't return a temporary by address. I looked a little deeper and
> it turns out that the KURL copy constructor is not *that* lightweight. there
is
> a static &KURL blankURL() helper that I could use here. It is not equivalent
> ("about://blank") but I think we should use that.
> 
>
https://codereview.chromium.org/297293004/diff/20001/Source/core/html/canvas/...
> File Source/core/html/canvas/CanvasRenderingContext.cpp (right):
> 
>
https://codereview.chromium.org/297293004/diff/20001/Source/core/html/canvas/...
> Source/core/html/canvas/CanvasRenderingContext.cpp:39: bool
> CanvasRenderingContext::isKnownCleanURL(const KURL& url) const
> On 2014/05/27 19:02:12, Stephen White wrote:
> > "Clean" doesn't seem to fully capture the meaning here. Could we use
> > isKnownOriginCleanURL? Would that make sense in this context?
> 
> That name is confusing IMO. The question this method answers is whether this
> canvas will remain origin clean after receiving content from the given URL.
The
> source can be origin clean and still taint the destination canvas.
> 
> What we want is a more compact form of: isURLKnownToNotTaintThisCanvas

Since the argument is already explicitly a URL, maybe it could just be:
wouldNotTaintOrigin()?

[Ken Russell (switch to Gerrit), 2014-05-27 23:18:58]

LGTM. I don't have a strong preference about the naming convention, but getting
this caching back is good.

[Justin Novosad, 2014-05-28 18:56:48]

New patch.
Re-organized the code to centralize the logic in CanvasRenderingContext. Much
cleaner this way.
I also added caching for URLs that taint the canvas (instead of just URLs that
don't taint)

[Stephen White, 2014-05-28 19:07:44]

LGTM. Nice! Thanks for the cleanup.

Do we have tests that exercise all the new codepaths? (e.g., requesting the same
would-dirty URL twice, the same would-not-dirty URL twice, etc).

[Justin Novosad, 2014-05-28 19:19:20]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2014-05-28 19:19:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/junov@chromium.org/297293004/40001

[commit-bot: I haz the power, 2014-05-28 20:33:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/10094)

[commit-bot: I haz the power, 2014-05-28 21:03:19]

Change committed as 174995