Add saveImageFromDataURL() and use it in saveImageAt().

Add saveImageFromDataURL() and use it in saveImageAt().

The new method is invoked as a response of saveImageAt() and takes over a data
url string as a parameter. Then we use the string of data url instead of real
url to avoid size-limits for url on IPC subsystems. Ultimately, this new code
path imposes a higher limit for data url used for saving canvases and large
images to disk.

This change is used by the following chromium side CL.
  https://crrev.com/518693002/

BUG=69227, 401888
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182100

[zino, 2014-09-02 06:49:31]

jinho.bang@samsung.com changed reviewers:
+ junov@chromium.org, kbr@chromium.org

[zino, 2014-09-02 07:09:24]

Please take a look.

This CL is related to https://crrev.com/518693002/.
Thank you.

[Justin Novosad, 2014-09-03 16:55:29]

https://codereview.chromium.org/518043003/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/518043003/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:3306:
m_client->saveImageFromDataURL(toElement(*node).imageSourceURL());
The name "saveImageFromDataURL" is misleading. The imageSourceURL will not be a
data URL is the node is an image element. That should be fixed in the companion
chromium-side change.

[zino, 2014-09-03 17:56:08]

On 2014/09/03 16:55:29, junov wrote:
>
https://codereview.chromium.org/518043003/diff/20001/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/518043003/diff/20001/Source/web/WebViewImpl.c...
> Source/web/WebViewImpl.cpp:3306:
> m_client->saveImageFromDataURL(toElement(*node).imageSourceURL());
> The name "saveImageFromDataURL" is misleading. The imageSourceURL will not be
a
> data URL is the node is an image element. That should be fixed in the
companion
> chromium-side change.

Thank you for review.

The method is used in chromium side only if an image element has a data url.

[Ken Russell (switch to Gerrit), 2014-09-04 00:23:10]

https://codereview.chromium.org/518043003/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/518043003/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:3306:
m_client->saveImageFromDataURL(toElement(*node).imageSourceURL());
On 2014/09/03 16:55:29, junov wrote:
> The name "saveImageFromDataURL" is misleading. The imageSourceURL will not be
a
> data URL is the node is an image element. That should be fixed in the
companion
> chromium-side change.

After considering the various alternatives I think this code should invoke the
old code path for images, and use this new API only for data: URLs. That has the
least possible security impact. It should still allow some of the old code to be
deleted; just not the NavigationPolicyDownloadTo enum.

https://codereview.chromium.org/518043003/diff/20001/public/web/WebViewClient.h
File public/web/WebViewClient.h (right):

https://codereview.chromium.org/518043003/diff/20001/public/web/WebViewClient...
public/web/WebViewClient.h:125: // A data url from <canvas> or <img> is passed
to the method's argument.
For security reasons I think this new API should only be used for canvases and
their data: URLs. As Justin points out, URLs coming from images won't actually
be data: URLs.

[zino, 2014-09-04 01:50:01]

On 2014/09/04 00:23:10, Ken Russell wrote:
> After considering the various alternatives I think this code should invoke the
> old code path for images, and use this new API only for data: URLs. That has
the
> least possible security impact. It should still allow some of the old code to
be
> deleted; just not the NavigationPolicyDownloadTo enum.
> 
>
https://codereview.chromium.org/518043003/diff/20001/public/web/WebViewClient.h
> File public/web/WebViewClient.h (right):
> 
>
https://codereview.chromium.org/518043003/diff/20001/public/web/WebViewClient...
> public/web/WebViewClient.h:125: // A data url from <canvas> or <img> is passed
> to the method's argument.
> For security reasons I think this new API should only be used for canvases and
> their data: URLs. As Justin points out, URLs coming from images won't actually
> be data: URLs.

Thank you for review.

As my CL, I think this method is invoked in the following cases.
(But it needs to check whether incoming data is url or not.)
 - <img src="data:image/png,base64blahblah..."> (the data url > 2MiB)
 - <canvas>

It shouldn't be invoked in the following cases.
 - <img src="http://google.com/blahblah.png">
 - <img src="blahblah.png">

What kind of security problem does it have?
If the <img src="data:..."> has a problem, then <canvas> is okay?

[Ken Russell (switch to Gerrit), 2014-09-04 02:25:48]

On 2014/09/04 01:50:01, zino wrote:
> On 2014/09/04 00:23:10, Ken Russell wrote:
> > After considering the various alternatives I think this code should invoke
the
> > old code path for images, and use this new API only for data: URLs. That has
> the
> > least possible security impact. It should still allow some of the old code
to
> be
> > deleted; just not the NavigationPolicyDownloadTo enum.
> > 
> >
>
https://codereview.chromium.org/518043003/diff/20001/public/web/WebViewClient.h
> > File public/web/WebViewClient.h (right):
> > 
> >
>
https://codereview.chromium.org/518043003/diff/20001/public/web/WebViewClient...
> > public/web/WebViewClient.h:125: // A data url from <canvas> or <img> is
passed
> > to the method's argument.
> > For security reasons I think this new API should only be used for canvases
and
> > their data: URLs. As Justin points out, URLs coming from images won't
actually
> > be data: URLs.
> 
> Thank you for review.
> 
> As my CL, I think this method is invoked in the following cases.
> (But it needs to check whether incoming data is url or not.)
>  - <img src="data:image/png,base64blahblah..."> (the data url > 2MiB)
>  - <canvas>
> 
> It shouldn't be invoked in the following cases.
>  - <img src="http://google.com/blahblah.png">
>  - <img src="blahblah.png">
> 
> What kind of security problem does it have?
> If the <img src="data:..."> has a problem, then <canvas> is okay?

It isn't clear to me that the new code path is only used for images with data:
URLs as their source. How is that decided? Have you tested that? Can more tests
be added to ensure this?

I don't know the ResourceRequest code or what it triggers in response, but my
thought was that if it implements some kind of content or security policies on
behalf of the user, that sidestepping it and sending a
ViewHostMsg_SaveImageFromDataURL for images with http: sources unconditionally
is not correct.

japhet@ and abarth@ should know that code better. CC'ing them for comment.

[zino, 2014-09-05 03:53:29]

Dear @kbr,

I've just uploaded a new patch set (which includes unit test).
and please refer to https://codereview.chromium.org/518693002/#msg19

Could you please review again?

[Justin Novosad, 2014-09-05 17:22:47]

https://codereview.chromium.org/518043003/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/518043003/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:3294:
m_page->deprecatedLocalMainFrame()->editor().copyImage(result);
Don't we have the same bug here too?

https://codereview.chromium.org/518043003/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:3308: return;
If the user attempts to save an image with an http URL, we just return? Isn't
that a regression?  I think what kbr was suggestion is that in the case we
should do things the old way.

[zino, 2014-09-05 18:00:26]

Thank you for review.

https://codereview.chromium.org/518043003/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/518043003/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:3294:
m_page->deprecatedLocalMainFrame()->editor().copyImage(result);
On 2014/09/05 17:22:46, junov wrote:

Internally, the API copies a wrapper object for bitmap image and its url to
shared memory and then renderer sends a pointer of the shared memory to browser.
So, it is okay because GURL marshalling doesn't happen.

https://codereview.chromium.org/518043003/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:3308: return;
On 2014/09/05 17:22:46, junov wrote:

If the user attempts to save an image with an http URL, we don't use this code
path. In that case, browser process can immediately start to download the image
without this API. because the process already knows the URL.

<canvas> : use saveImageAt()
<img> with a large data url : use saveImageAt()
<img> with http, https, and so on : use WebContents::saveFrame()

[Justin Novosad, 2014-09-05 18:39:03]

Okay, I am satisfied. But I will let kbr have the last word.

[Justin Novosad, 2014-09-10 18:09:19]

Review ping. kbr?

[Ken Russell (switch to Gerrit), 2014-09-11 00:43:07]

I'm sorry for the delay re-reviewing this. Thank you for explaining how both
this and the Chromium-side code worked. This revised CL and the new test LGTM.
Thank you for fixing this.

https://codereview.chromium.org/518043003/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/518043003/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:3308: return;
On 2014/09/05 18:00:26, zino wrote:
> On 2014/09/05 17:22:46, junov wrote:
> 
> If the user attempts to save an image with an http URL, we don't use this code
> path. In that case, browser process can immediately start to download the
image
> without this API. because the process already knows the URL.
> 
> <canvas> : use saveImageAt()
> <img> with a large data url : use saveImageAt()
> <img> with http, https, and so on : use WebContents::saveFrame()

Thanks for clarifying this.

[zino, 2014-09-13 14:43:54]

jinho.bang@samsung.com changed reviewers:
+ jochen@chromium.org

[zino, 2014-09-13 14:46:29]

Dear @jochen,

Could you please review this?
This CL needs your approval. (Especially, public/web/*)

[zino, 2014-09-16 14:56:42]

On 2014/09/13 14:46:29, zino wrote:
> Dear @jochen,
> 
> Could you please review this?
> This CL needs your approval. (Especially, public/web/*)

Ping @jochen.

[Ken Russell (switch to Gerrit), 2014-09-16 15:08:12]

kbr@chromium.org changed reviewers:
+ abarth@chromium.org, dglazkov@chromium.org, jamesr@chromium.org,
tkent@chromium.org

[Ken Russell (switch to Gerrit), 2014-09-16 15:08:13]

abarth or dglazkov or jamesr or tkent: public/ OWNERS review please.

[jochen (gone - plz use gerrit), 2014-09-16 15:53:32]

https://codereview.chromium.org/518043003/diff/40001/public/web/WebViewClient.h
File public/web/WebViewClient.h (right):

https://codereview.chromium.org/518043003/diff/40001/public/web/WebViewClient...
public/web/WebViewClient.h:126: virtual void saveImageFromDataURL(const
WebString&) { }
why not pass a WebURL ?

[Ken Russell (switch to Gerrit), 2014-09-16 15:58:08]

On 2014/09/16 15:53:32, jochen wrote:
>
https://codereview.chromium.org/518043003/diff/40001/public/web/WebViewClient.h
> File public/web/WebViewClient.h (right):
> 
>
https://codereview.chromium.org/518043003/diff/40001/public/web/WebViewClient...
> public/web/WebViewClient.h:126: virtual void saveImageFromDataURL(const
> WebString&) { }
> why not pass a WebURL ?

That ties in to the whole point of this CL -- the IPC subsystem imposes
arbitrary limits on GURLs. This new code path imposes a higher limit for data:
URLs used for saving canvases and large images to disk. The incoming data is
verified well in the browser-side code (https://crrev.com/518693002/).

[jochen (gone - plz use gerrit), 2014-09-16 16:11:16]

ok, can you please state this in the CL description

lgtm

[zino, 2014-09-16 17:15:11]

On 2014/09/16 16:11:16, jochen wrote:
> ok, can you please state this in the CL description
> 
> lgtm

Thank you for rubber stamp.
I've just updated the CL description by referring to the KenRussell's summary.

[zino, 2014-09-16 17:19:29]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-09-16 17:20:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/518043003/40001

[commit-bot: I haz the power, 2014-09-16 21:25:05]

Committed patchset #2 (id:40001) as 182100