Marshal the URL parameter of the clipboard bookmark format as a string.

Marshal the URL parameter of the clipboard bookmark format as a string.

This temporarily restores the original behavior. It'd be nice to use the
more specific type, but some components of Blink are generating long
data: URLs and passing them to WriteBookmark(), which causes the browser
to kill the renderer.

BUG=447094
Committed: https://crrev.com/87e6d8ab7a85f05ff4273208259a12f816a39e5d
Cr-Commit-Position: refs/heads/master@{#316981}

[dcheng, 2015-02-18 22:12:24]

dcheng@chromium.org changed reviewers:
+ jschuh@chromium.org, kbr@chromium.org

[dcheng, 2015-02-18 22:12:25]

Basically, I'm kicking the can down the road =/

[dcheng, 2015-02-18 22:48:03]

dcheng@chromium.org changed reviewers:
+ creasepattern@gmail.com

[dcheng, 2015-02-18 22:48:04]

+creis for preemptive content OWNERS review, since this is marked as blocking
stable.

[Ken Russell (switch to Gerrit), 2015-02-18 23:07:25]

When the original change to the IPCs was made to pass these potentially large
data: URLs as strings instead of GURLs, more validation was added to the
deserialization step -- I think an arbitrary limit of 10 MB was instituted.

Is that code still in place? Could you please point to it if so?

[dcheng, 2015-02-18 23:53:59]

On 2015/02/18 at 23:07:25, kbr wrote:
> When the original change to the IPCs was made to pass these potentially large
data: URLs as strings instead of GURLs, more validation was added to the
deserialization step -- I think an arbitrary limit of 10 MB was instituted.
> 
> Is that code still in place? Could you please point to it if so?

I don't know where this limit is enforced, but:
1) We've never enforced any meaningful limits on the clipboard data
serialization before this, because it was serialized as a map of formats to
vectors of bytes (!!!).
2) IPC::ParamTraits<std::string> doesn't seem to enforce any limits either.

[Charlie Reis, 2015-02-19 00:47:47]

creis@chromium.org changed reviewers:
+ creis@chromium.org

[Charlie Reis, 2015-02-19 00:47:48]

Can we put a TODO somewhere saying to change this back to GURL when Canvas stops
using data URLs for images?  We should point to a bug number for making that
happen.

Otherwise, I'll grant my owner LGTM for this as a stopgap to avoid the renderer
kills.  Please get kbr's approval as well.

[Ken Russell (switch to Gerrit), 2015-02-19 00:56:55]

On 2015/02/18 23:53:59, dcheng wrote:
> On 2015/02/18 at 23:07:25, kbr wrote:
> > When the original change to the IPCs was made to pass these potentially
large
> data: URLs as strings instead of GURLs, more validation was added to the
> deserialization step -- I think an arbitrary limit of 10 MB was instituted.
> > 
> > Is that code still in place? Could you please point to it if so?
> 
> I don't know where this limit is enforced, but:
> 1) We've never enforced any meaningful limits on the clipboard data
> serialization before this, because it was serialized as a map of formats to
> vectors of bytes (!!!).
> 2) IPC::ParamTraits<std::string> doesn't seem to enforce any limits either.

The enforcement I was thinking of was added in
https://codereview.chromium.org/518693002/ -- specifically the check against
kMaxLengthOfDataURLString. I see now that this is a different code path.

If the code before your change didn't enforce any size limits on the data, then
this also LGTM as a stopgap measure. Please do follow creis' request and file a
bug about fixing this with a different data transfer mechanism in the future.

[dcheng, 2015-02-19 01:28:30]

On 2015/02/19 at 00:56:55, kbr wrote:
> On 2015/02/18 23:53:59, dcheng wrote:
> > On 2015/02/18 at 23:07:25, kbr wrote:
> > > When the original change to the IPCs was made to pass these potentially
large
> > data: URLs as strings instead of GURLs, more validation was added to the
> > deserialization step -- I think an arbitrary limit of 10 MB was instituted.
> > > 
> > > Is that code still in place? Could you please point to it if so?
> > 
> > I don't know where this limit is enforced, but:
> > 1) We've never enforced any meaningful limits on the clipboard data
> > serialization before this, because it was serialized as a map of formats to
> > vectors of bytes (!!!).
> > 2) IPC::ParamTraits<std::string> doesn't seem to enforce any limits either.
> 
> The enforcement I was thinking of was added in
https://codereview.chromium.org/518693002/ -- specifically the check against
kMaxLengthOfDataURLString. I see now that this is a different code path.
> 
> If the code before your change didn't enforce any size limits on the data,
then this also LGTM as a stopgap measure. Please do follow creis' request and
file a bug about fixing this with a different data transfer mechanism in the
future.

Ah. Clipboard code has never gone through this code path.
Yay for consistency...

[dcheng, 2015-02-19 01:35:20]

New patchsets have been uploaded after l-g-t-m from
creis@chromium.org,kbr@chromium.org

[dcheng, 2015-02-19 01:36:04]

On 2015/02/19 at 00:47:48, creis wrote:
> Can we put a TODO somewhere saying to change this back to GURL when Canvas
stops using data URLs for images?  We should point to a bug number for making
that happen.
> 
> Otherwise, I'll grant my owner LGTM for this as a stopgap to avoid the
renderer kills.  Please get kbr's approval as well.

Done, I filed a new bug so it doesn't get conflated with the patch that needs to
be merged into stable.

[dcheng, 2015-02-19 01:36:07]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2015-02-19 01:37:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/934373002/20001

[commit-bot: I haz the power, 2015-02-19 02:55:02]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-19 02:55:43]

Patchset 2 (id:??) landed as
https://crrev.com/87e6d8ab7a85f05ff4273208259a12f816a39e5d
Cr-Commit-Position: refs/heads/master@{#316981}