Rewrite clipboard write IPC handling to be easier to understand.

Rewrite clipboard write IPC handling to be easier to understand.

The original implementation sent clipboard data to be written over IPC
as a map of clipboard formats to 'parameters' for that format. The
parameters were just vectors of byte vectors. Needless to say, this
logic was complicated, fragile, and prone to bugs. In the browser
process, this resulted in the IPC validation logic being scattered
between ClipboardMessageFilter and Clipboard::DispatchObject.

The rewrite adds an IPC message for each flavor of data that the
renderer is allowed to write to the clipboard. On the browser side,
the logic is simply delegated to ScopedClipboardWriter. Since the
clipboard object map is no longer under the control of an untrusted
process, this allows the removal of a lot of validation logic.
Unfortunately, bitmap handling is still a bit complicated because it's
sent over shared memory, but all the validation logic has been moved
into ClipboardMessageFilter.

BUG=319285
Committed: https://crrev.com/a8095525d13449cd3ce2fe18d4f32f2133e69732
Cr-Commit-Position: refs/heads/master@{#304895}

[dcheng, 2014-09-18 07:03:03]

dcheng@chromium.org changed reviewers:
+ jamesr@chromium.org, jln@chromium.org

[dcheng, 2014-09-18 07:32:42]

Patchset #7 (id:120001) has been deleted

[dcheng, 2014-09-18 17:02:20]

dcheng@chromium.org changed reviewers:
+ sky@chromium.org

[dcheng, 2014-09-18 17:02:23]

I think this is ready for an initial review. This was originally prompted by
Pwn2Own mobile in Tokyo last year, and then again by Pwn2Own this past spring.
After seeing erg@'s recent Mojo clipboard patch, I was inspired to finish up the
patch I had lying around. Originally, I was hoping this could make it into
stable in time for this year's Pwn2Own mobile but I don't think that's going to
happen.

+jln for an overall review (since he was the one who pointed out how insane all
this code was)
+sky for content/browser/ and ui/ changes
+jamesr for content/renderer/ changes

Two additional notes:
- WriteObjects and ObjectMap is still kind of insane.
- The compile is busted on a lot of the trybots (but not my machine) due to the
SkBitmap.h include in scoped_clipboard_writer.h. I haven't really figured out
why; I might just make the simple fix of storing in a scoped_ptr member instead.

https://codereview.chromium.org/574273002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://codereview.chromium.org/574273002/diff/100001/content/browser/rendere...
content/browser/renderer_host/clipboard_message_filter.cc:253: new
base::SharedMemory(handle, true, PeerHandle());
Apparently, you only need to specify the process handle on Windows--a previous
comment in this code claimed that it wasn't needed for Posix. Since this
constructor is available on all platforms, can we just unconditionally call this
to avoid the ifdef and simplify the code?

https://codereview.chromium.org/574273002/diff/100001/content/browser/rendere...
content/browser/renderer_host/clipboard_message_filter.cc:274: if
(!bitmap.installPixels(bitmap.info(),
I'm going to try to find a Skia person to see if there's a better way to do
this--passing the SkImageInfo back to itself seems redundant, though harmless.

https://codereview.chromium.org/574273002/diff/100001/content/renderer/webcli...
File content/renderer/webclipboard_impl.cc (right):

https://codereview.chromium.org/574273002/diff/100001/content/renderer/webcli...
content/renderer/webclipboard_impl.cc:151: DCHECK(!image.isNull());
I'm not really sure if we need this check. It's worth noting that we were seeing
null pixelrefs even with this check so I figured we may as well change this into
a DCHECK and see what happens (we still check that the pixelref is non-null).

https://codereview.chromium.org/574273002/diff/100001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard.cc (right):

https://codereview.chromium.org/574273002/diff/100001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard.cc:123: **reinterpret_cast<const SkBitmap*
const*>(&(params[0].front())));
Yes, I am a bad person.

https://codereview.chromium.org/574273002/diff/100001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/574273002/diff/100001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard.h:283: // CBF_SMBITMAP  bitmap       A pointer to a
SkBitmap. The caller must ensure
This is really horrible, but I don't want to shave too many yaks at once. I
think I would be willing to argue that this is less horrible than before.

https://codereview.chromium.org/574273002/diff/100001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard_unittest.cc (left):

https://codereview.chromium.org/574273002/diff/100001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard_unittest.cc:424: namespace {
Should I move these deleted tests to something like ClipboardMessageFilterTest?
I don't really see too many unit tests for malformed messages; is this something
we want to encourage/discourage?

[sky, 2014-09-18 19:47:53]

https://codereview.chromium.org/574273002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://codereview.chromium.org/574273002/diff/100001/content/browser/rendere...
content/browser/renderer_host/clipboard_message_filter.cc:221:
clipboard_writer_->WriteHTML(markup, url.spec());
Do you need to deal with an invalid url?

https://codereview.chromium.org/574273002/diff/100001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/574273002/diff/100001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard.h:283: // CBF_SMBITMAP  bitmap       A pointer to a
SkBitmap. The caller must ensure
On 2014/09/18 17:02:22, dcheng (OOO) wrote:
> This is really horrible, but I don't want to shave too many yaks at once. I
> think I would be willing to argue that this is less horrible than before.

It's all awful. Are you going to make this less fragile and more type safe? If
so, what is your propsal?

https://codereview.chromium.org/574273002/diff/100001/ui/base/clipboard/scope...
File ui/base/clipboard/scoped_clipboard_writer.cc (right):

https://codereview.chromium.org/574273002/diff/100001/ui/base/clipboard/scope...
ui/base/clipboard/scoped_clipboard_writer.cc:99: for (size_t i = 0; i <
sizeof(bitmap_pointer); ++i) {
nit: no {}

[dcheng, 2014-09-18 21:06:00]

I'm still trying to resolve the dependency mess. Sigh.

https://codereview.chromium.org/574273002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://codereview.chromium.org/574273002/diff/100001/content/browser/rendere...
content/browser/renderer_host/clipboard_message_filter.cc:221:
clipboard_writer_->WriteHTML(markup, url.spec());
On 2014/09/18 19:47:53, sky wrote:
> Do you need to deal with an invalid url?

It probably doesn't hurt to filter this out--I think a hostile renderer could
use this to write malformed CF_HTML (on Windows) which would crash other
programs with lax input validation.

That being said, would you be OK if I implemented that in a separate patch? I
think it'd make the most sense to make WriteHTML() take a GURL parameter (same
for WriteBookmark) so we can validate the input in one place.

https://codereview.chromium.org/574273002/diff/100001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/574273002/diff/100001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard.h:283: // CBF_SMBITMAP  bitmap       A pointer to a
SkBitmap. The caller must ensure
On 2014/09/18 19:47:53, sky wrote:
> On 2014/09/18 17:02:22, dcheng (OOO) wrote:
> > This is really horrible, but I don't want to shave too many yaks at once. I
> > think I would be willing to argue that this is less horrible than before.
> 
> It's all awful. Are you going to make this less fragile and more type safe? If
> so, what is your propsal?

I would like to make it less fragile, but I don't have a solid plan yet. I think
the most logical thing to do would be to reuse DropData instead, but I haven't
figured out an elegant way to graft on clipboard-specific cruft yet (and there's
also drop-specific stuff in there).

https://codereview.chromium.org/574273002/diff/100001/ui/base/clipboard/scope...
File ui/base/clipboard/scoped_clipboard_writer.cc (right):

https://codereview.chromium.org/574273002/diff/100001/ui/base/clipboard/scope...
ui/base/clipboard/scoped_clipboard_writer.cc:99: for (size_t i = 0; i <
sizeof(bitmap_pointer); ++i) {
On 2014/09/18 19:47:53, sky wrote:
> nit: no {}

Done.

[dcheng, 2014-09-18 21:12:20]

dcheng@chromium.org changed reviewers:
+ wfh@chromium.org
- jln@chromium.org

[dcheng, 2014-09-18 21:12:21]

+wfh for the IPC changes instead.

[sky, 2014-09-18 23:40:31]

https://codereview.chromium.org/574273002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://codereview.chromium.org/574273002/diff/100001/content/browser/rendere...
content/browser/renderer_host/clipboard_message_filter.cc:221:
clipboard_writer_->WriteHTML(markup, url.spec());
On 2014/09/18 21:06:00, dcheng (OOO) wrote:
> On 2014/09/18 19:47:53, sky wrote:
> > Do you need to deal with an invalid url?
> 
> It probably doesn't hurt to filter this out--I think a hostile renderer could
> use this to write malformed CF_HTML (on Windows) which would crash other
> programs with lax input validation.
> 
> That being said, would you be OK if I implemented that in a separate patch? I
> think it'd make the most sense to make WriteHTML() take a GURL parameter (same
> for WriteBookmark) so we can validate the input in one place.

On tip of tree do we allow through invalid urls?

[dcheng, 2014-09-18 23:44:53]

On 2014/09/18 at 23:40:31, sky wrote:
>
https://codereview.chromium.org/574273002/diff/100001/content/browser/rendere...
> File content/browser/renderer_host/clipboard_message_filter.cc (right):
> 
>
https://codereview.chromium.org/574273002/diff/100001/content/browser/rendere...
> content/browser/renderer_host/clipboard_message_filter.cc:221:
clipboard_writer_->WriteHTML(markup, url.spec());
> On 2014/09/18 21:06:00, dcheng (OOO) wrote:
> > On 2014/09/18 19:47:53, sky wrote:
> > > Do you need to deal with an invalid url?
> > 
> > It probably doesn't hurt to filter this out--I think a hostile renderer
could
> > use this to write malformed CF_HTML (on Windows) which would crash other
> > programs with lax input validation.
> > 
> > That being said, would you be OK if I implemented that in a separate patch?
I
> > think it'd make the most sense to make WriteHTML() take a GURL parameter
(same
> > for WriteBookmark) so we can validate the input in one place.
> 
> On tip of tree do we allow through invalid urls?

Yes, we do.

[sky, 2014-09-19 02:51:49]

Ok, LGTM then

[jamesr, 2014-09-22 22:38:41]

https://codereview.chromium.org/574273002/diff/160001/content/renderer/render...
File content/renderer/renderer_clipboard_client.h (right):

https://codereview.chromium.org/574273002/diff/160001/content/renderer/render...
content/renderer/renderer_clipboard_client.h:24: class RendererClipboardClient {
this isn't a client thingy any more. if anything, it's a delegate since it's the
thing that the webclipboard_impl code is delegating to in order to actually
perform the clipboard operations.

https://codereview.chromium.org/574273002/diff/160001/ui/base/clipboard/scope...
File ui/base/clipboard/scoped_clipboard_writer.cc (right):

https://codereview.chromium.org/574273002/diff/160001/ui/base/clipboard/scope...
ui/base/clipboard/scoped_clipboard_writer.cc:98: Clipboard::ObjectMapParam
packed_pointer;
can you use bit_cast<Clipboard::ObjectMapParam>(&bitmap_) from base/macros.h ?

https://codereview.chromium.org/574273002/diff/160001/ui/base/clipboard/scope...
ui/base/clipboard/scoped_clipboard_writer.cc:99: for (size_t i = 0; i <
sizeof(bitmap_pointer); ++i)
oh god

[dcheng, 2014-09-22 23:06:25]

https://codereview.chromium.org/574273002/diff/160001/content/renderer/render...
File content/renderer/renderer_clipboard_client.h (right):

https://codereview.chromium.org/574273002/diff/160001/content/renderer/render...
content/renderer/renderer_clipboard_client.h:24: class RendererClipboardClient {
On 2014/09/22 22:38:41, jamesr wrote:
> this isn't a client thingy any more. if anything, it's a delegate since it's
the
> thing that the webclipboard_impl code is delegating to in order to actually
> perform the clipboard operations.

Done.

https://codereview.chromium.org/574273002/diff/160001/ui/base/clipboard/scope...
File ui/base/clipboard/scoped_clipboard_writer.cc (right):

https://codereview.chromium.org/574273002/diff/160001/ui/base/clipboard/scope...
ui/base/clipboard/scoped_clipboard_writer.cc:98: Clipboard::ObjectMapParam
packed_pointer;
On 2014/09/22 22:38:41, jamesr wrote:
> can you use bit_cast<Clipboard::ObjectMapParam>(&bitmap_) from base/macros.h ?

I don't think I can use bit_cast directly here, because it wouldn't know how to
memcpy into std::vector's internal buffer.

https://codereview.chromium.org/574273002/diff/160001/ui/base/clipboard/scope...
ui/base/clipboard/scoped_clipboard_writer.cc:99: for (size_t i = 0; i <
sizeof(bitmap_pointer); ++i)
On 2014/09/22 22:38:41, jamesr wrote:
> oh god

Yeah... I think that is the only appropriate response to this code. I still
think this is slightly better than the original version where we sent the
ClipboardObjectMap directly over the IPC message and then tried to do all sorts
of checks in the browser side to make sure it looked sane-ish. I've been
thinking about ideas to fix this insanity as well though in a followup patch.

[Will Harris, 2014-09-22 23:23:12]

lgtm for IPC.  I am both surprised and impressed that IPC supports maps :)

[Will Harris, 2014-09-22 23:23:33]

oh I had one draft. just a minor comment.

https://codereview.chromium.org/574273002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://codereview.chromium.org/574273002/diff/100001/content/browser/rendere...
content/browser/renderer_host/clipboard_message_filter.cc:253: new
base::SharedMemory(handle, true, PeerHandle());
On 2014/09/18 17:02:22, dcheng (OOO) wrote:
> Apparently, you only need to specify the process handle on Windows--a previous
> comment in this code claimed that it wasn't needed for Posix. Since this
> constructor is available on all platforms, can we just unconditionally call
this
> to avoid the ifdef and simplify the code?

I think calling this passing a process handle on Posix will result in a DCHECK()

[dcheng, 2014-10-06 22:22:15]

PTAL. I've added some basic unit tests. Unfortunately, I had to remove the test
that we're not trying to Map() too much data--on some platforms (Windows), I
think this fails at Map() while on Linux, it only fails once we try to read past
the end of the shared memory segment creation size.

jamesr, I think this needs a LGTM from you still for content/renderer changes as
well.

[Will Harris, 2014-10-06 22:49:24]

still lgtm for Windows clipboard interactions

https://codereview.chromium.org/574273002/diff/380001/content/browser/rendere...
File content/browser/renderer_host/clipboard_message_filter_unittest.cc (right):

https://codereview.chromium.org/574273002/diff/380001/content/browser/rendere...
content/browser/renderer_host/clipboard_message_filter_unittest.cc:47: auto m =
make_scoped_ptr(new base::SharedMemory());
feels weird to see auto

[dcheng, 2014-10-06 22:51:44]

https://codereview.chromium.org/574273002/diff/380001/content/browser/rendere...
File content/browser/renderer_host/clipboard_message_filter_unittest.cc (right):

https://codereview.chromium.org/574273002/diff/380001/content/browser/rendere...
content/browser/renderer_host/clipboard_message_filter_unittest.cc:47: auto m =
make_scoped_ptr(new base::SharedMemory());
On 2014/10/06 22:49:24, Will Harris wrote:
> feels weird to see auto

Hmm. I guess. Basically, scoped_ptr<base::SharedMemory> m = make_scoped_ptr(new
base::SharedMemory); feels really redundant to me. Since it's clear what the
type is, I think it's OK to omit it here. There are several other locations
(e.g. line 40) where I opted not to use auto, because it's not 100% clear.

[jamesr, 2014-10-06 23:00:27]

https://codereview.chromium.org/574273002/diff/380001/content/browser/rendere...
File content/browser/renderer_host/clipboard_message_filter_unittest.cc (right):

https://codereview.chromium.org/574273002/diff/380001/content/browser/rendere...
content/browser/renderer_host/clipboard_message_filter_unittest.cc:47: auto m =
make_scoped_ptr(new base::SharedMemory());
On 2014/10/06 22:51:44, dcheng wrote:
> On 2014/10/06 22:49:24, Will Harris wrote:
> > feels weird to see auto
> 
> Hmm. I guess. Basically, scoped_ptr<base::SharedMemory> m =
make_scoped_ptr(new
> base::SharedMemory); feels really redundant to me. Since it's clear what the
> type is, I think it's OK to omit it here. There are several other locations
> (e.g. line 40) where I opted not to use auto, because it's not 100% clear.

Comparing the two:

auto m = make_scoped_ptr(new base::SharedMemory());
scoped_ptr<base::SharedMemory> m(new base::SharedMemory);

the latter is 6 characters longer but feels pretty tight to me and is clearer. 
I'd skip the auto here.

[dcheng, 2014-10-06 23:01:55]

On 2014/10/06 at 23:00:27, jamesr wrote:
>
https://codereview.chromium.org/574273002/diff/380001/content/browser/rendere...
> File content/browser/renderer_host/clipboard_message_filter_unittest.cc
(right):
> 
>
https://codereview.chromium.org/574273002/diff/380001/content/browser/rendere...
> content/browser/renderer_host/clipboard_message_filter_unittest.cc:47: auto m
= make_scoped_ptr(new base::SharedMemory());
> On 2014/10/06 22:51:44, dcheng wrote:
> > On 2014/10/06 22:49:24, Will Harris wrote:
> > > feels weird to see auto
> > 
> > Hmm. I guess. Basically, scoped_ptr<base::SharedMemory> m =
make_scoped_ptr(new
> > base::SharedMemory); feels really redundant to me. Since it's clear what the
> > type is, I think it's OK to omit it here. There are several other locations
> > (e.g. line 40) where I opted not to use auto, because it's not 100% clear.
> 
> Comparing the two:
> 
> auto m = make_scoped_ptr(new base::SharedMemory());
> scoped_ptr<base::SharedMemory> m(new base::SharedMemory);
> 
> the latter is 6 characters longer but feels pretty tight to me and is clearer.
 I'd skip the auto here.

Oops, good point. I'll simplify this here.

[dcheng, 2014-10-06 23:34:42]

(Rebased as well, since a few things have moved around)

https://codereview.chromium.org/574273002/diff/380001/content/browser/rendere...
File content/browser/renderer_host/clipboard_message_filter_unittest.cc (right):

https://codereview.chromium.org/574273002/diff/380001/content/browser/rendere...
content/browser/renderer_host/clipboard_message_filter_unittest.cc:47: auto m =
make_scoped_ptr(new base::SharedMemory());
On 2014/10/06 at 23:00:27, jamesr wrote:
> On 2014/10/06 22:51:44, dcheng wrote:
> > On 2014/10/06 22:49:24, Will Harris wrote:
> > > feels weird to see auto
> > 
> > Hmm. I guess. Basically, scoped_ptr<base::SharedMemory> m =
make_scoped_ptr(new
> > base::SharedMemory); feels really redundant to me. Since it's clear what the
> > type is, I think it's OK to omit it here. There are several other locations
> > (e.g. line 40) where I opted not to use auto, because it's not 100% clear.
> 
> Comparing the two:
> 
> auto m = make_scoped_ptr(new base::SharedMemory());
> scoped_ptr<base::SharedMemory> m(new base::SharedMemory);
> 
> the latter is 6 characters longer but feels pretty tight to me and is clearer.
 I'd skip the auto here.

Done.

[jamesr, 2014-10-09 22:34:12]

content/renderer/ lgtm, one comment on another file i cannot understand

https://codereview.chromium.org/574273002/diff/420001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard.cc (right):

https://codereview.chromium.org/574273002/diff/420001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard.cc:123: **reinterpret_cast<const SkBitmap*
const*>(&(params[0].front())));
i can't follow this expression. what is the type of param[0].front()? could you
use some intermediate vars here with the types spelled out to see what's
happening?

[dcheng, 2014-10-09 22:39:17]

https://codereview.chromium.org/574273002/diff/420001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard.cc (right):

https://codereview.chromium.org/574273002/diff/420001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard.cc:123: **reinterpret_cast<const SkBitmap*
const*>(&(params[0].front())));
On 2014/10/09 22:34:12, jamesr wrote:
> i can't follow this expression. what is the type of param[0].front()? could
you
> use some intermediate vars here with the types spelled out to see what's
> happening?

Would you find this clearer if it used vector_as_array and aliased that into a
char* pointer?
The only reason I used params[0].front() is to match the existing style.

[jamesr, 2014-10-09 22:41:28]

https://codereview.chromium.org/574273002/diff/420001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard.cc (right):

https://codereview.chromium.org/574273002/diff/420001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard.cc:123: **reinterpret_cast<const SkBitmap*
const*>(&(params[0].front())));
On 2014/10/09 22:39:17, dcheng wrote:
> On 2014/10/09 22:34:12, jamesr wrote:
> > i can't follow this expression. what is the type of param[0].front()? could
> you
> > use some intermediate vars here with the types spelled out to see what's
> > happening?
> 
> Would you find this clearer if it used vector_as_array and aliased that into a
> char* pointer?
> The only reason I used params[0].front() is to match the existing style.

I'd find it clearer if the intermediates of this expression were spelled out. 
For instance, I can't tell easily what the type of params[0].front() is, so it's
hard to understand what the address of it means and how it's being cast to a
const SkBitmap* const* or why that's being dereferenced twice.

[dcheng, 2014-10-13 05:52:22]

https://codereview.chromium.org/574273002/diff/420001/ui/base/clipboard/clipb...
File ui/base/clipboard/clipboard.cc (right):

https://codereview.chromium.org/574273002/diff/420001/ui/base/clipboard/clipb...
ui/base/clipboard/clipboard.cc:123: **reinterpret_cast<const SkBitmap*
const*>(&(params[0].front())));
On 2014/10/09 at 22:41:28, jamesr wrote:
> On 2014/10/09 22:39:17, dcheng wrote:
> > On 2014/10/09 22:34:12, jamesr wrote:
> > > i can't follow this expression. what is the type of param[0].front()?
could
> > you
> > > use some intermediate vars here with the types spelled out to see what's
> > > happening?
> > 
> > Would you find this clearer if it used vector_as_array and aliased that into
a
> > char* pointer?
> > The only reason I used params[0].front() is to match the existing style.
> 
> I'd find it clearer if the intermediates of this expression were spelled out. 
For instance, I can't tell easily what the type of params[0].front() is, so it's
hard to understand what the address of it means and how it's being cast to a
const SkBitmap* const* or why that's being dereferenced twice.

Done. I've added some intermediate types in both
ScopedClipboardWriter::WriteImage and in here to make it a little more obvious
what's going on.

[dcheng, 2014-10-13 05:52:36]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-10-13 05:53:42]

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

[commit-bot: I haz the power, 2014-10-13 06:00:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-13 06:00:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dcheng, 2014-11-19 16:44:16]

dcheng@chromium.org changed reviewers:
+ avi@chromium.org

[dcheng, 2014-11-19 16:44:18]

+avi, mind doing an OWNERS review of content/browser/renderer_host?

+sky, let me know if the test changes in ui/base/test look reasonable to you--it
seemed easier to me to have the test helper just return the created clipboard
directly.

[Avi (use Gerrit), 2014-11-19 17:58:01]

content/browser/renderer_host LGTM

[sky, 2014-11-19 18:55:30]

LGTM

https://codereview.chromium.org/574273002/diff/840001/ui/base/test/test_clipb...
File ui/base/test/test_clipboard.h (right):

https://codereview.chromium.org/574273002/diff/840001/ui/base/test/test_clipb...
ui/base/test/test_clipboard.h:19: static Clipboard* CreateForCurrentThread();
Document ownership.

[dcheng, 2014-11-19 20:38:52]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2014-11-19 20:39:03]

https://codereview.chromium.org/574273002/diff/840001/ui/base/test/test_clipb...
File ui/base/test/test_clipboard.h (right):

https://codereview.chromium.org/574273002/diff/840001/ui/base/test/test_clipb...
ui/base/test/test_clipboard.h:19: static Clipboard* CreateForCurrentThread();
On 2014/11/19 18:55:30, sky wrote:
> Document ownership.

Done.

[commit-bot: I haz the power, 2014-11-19 20:40:03]

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

[commit-bot: I haz the power, 2014-11-19 21:49:37]

Committed patchset #29 (id:860001)

[commit-bot: I haz the power, 2014-11-19 21:50:02]

Patchset 29 (id:??) landed as
https://crrev.com/a8095525d13449cd3ce2fe18d4f32f2133e69732
Cr-Commit-Position: refs/heads/master@{#304895}

[sky, 2014-11-19 22:43:43]

A revert of this CL (patchset #29 id:860001) has been created in
https://codereview.chromium.org/740003003/ by sky@chromium.org.

The reason for reverting is: Reverting as broke components_unittests on a couple
of bots:

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...
0 0x7fe36932398e base::debug::StackTrace::StackTrace()
#1 0x7fe3693234c3 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7fe35cb16cb0 \u003Cunknown>
#3 0x7fe35917c425 gsignal
#4 0x7fe35917fb8b abort
#5 0x7fe35977e5ad __gnu_debug::_Error_formatter::_M_error()
#6 0x7fe36ce9025f std::__debug::vector\u003C>::front()
#7 0x7fe36ce8e892 ui::Clipboard::DispatchObject()
#8 0x7fe36ce98183 ui::ClipboardAura::WriteObjects()
#9 0x7fe36cea124f ui::ScopedClipboardWriter::~ScopedClipboardWriter()
#10 0x0000007b6216 bookmarks::(anonymous
namespace)::BookmarkUtilsTest_PasteBookmarkFromURL_Test::TestBody()
#11 0x000000ff3af3
testing::internal::HandleSehExceptionsInMethodIfSupported\u003C>()
#12 0x000000fe873e
testing::internal::HandleExceptionsInMethodIfSupported\u003C>()
#13 0x000000fdd5b5 testing::Test::Run()
#14 0x000000fddcab testing::TestInfo::Run()
#15 0x000000fde29a testing::TestCase::Run()
#16 0x000000fe3793 testing::internal::UnitTestImpl::RunAllTests()
#17 0x000000ff0db3
testing::internal::HandleSehExceptionsInMethodIfSupported\u003C>()
#18 0x000000fe9f4e
testing::internal::HandleExceptionsInMethodIfSupported\u003C>()
#19 0x000000fe3431 testing::UnitTest::Run()
#20 0x000000f7d511 RUN_ALL_TESTS()
#21 0x000000f7c4e7 base::TestSuite::Run()
#22 0x000000762b22 base::internal::RunnableAdapter\u003C>::Run()
#23 0x000000c5833f base::internal::InvokeHelper\u003C>::MakeItSo()
#24 0x000000c582ea base::internal::Invoker\u003C>::Run()
#25 0x0000007f099e base::Callback\u003C>::Run()
#26 0x000000f70299 base::(anonymous namespace)::LaunchUnitTestsInternal()
#27 0x000000f6ffd7 base::LaunchUnitTests()
#28 0x000000c58035 main
#29 0x7fe35916776d __libc_start_main
#30 0x000000533a55 \u003Cunknown>
  r8: 00007fe355d8f8c0  r9: 00007fff2537ee98 r10: 0000000000000008 r11:
0000000000000202
 r12: 00007fff2537f120 r13: 0000000000000000 r14: 0000000000000001 r15:
0000000000000000
  di: 0000000000003046  si: 0000000000003046  bp: 0000000000000001  bx:
00007fff2537f0e8
  dx: 0000000000000006  ax: 0000000000000000  cx: ffffffffffffffff  sp:
00007fff2537eee8
  ip: 00007fe35917c425 efl: 0000000000000202 cgf: 0000000000000033 erf:
0000000000000000
 trp: 0000000000000000 msk: 0000000000000000 cr2: 0000000000000000

BookmarkUtilsTest.PasteBookmarkFromURL (run #2):
[ RUN      ] BookmarkUtilsTest.PasteBookmarkFromURL
/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/debug/vector:336:
    error: attempt to access an element in an empty container.

Objects involved in the operation:
sequence "this" @ 0x0x22748bd981e0 {
}
Received signal 6
#0 0x7f00fb73798e base::debug::StackTrace::StackTrace()
#1 0x7f00fb7374c3 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f00eef2acb0 \u003Cunknown>
#3 0x7f00eb590425 gsignal
#4 0x7f00eb593b8b abort
#5 0x7f00ebb925ad __gnu_debug::_Error_formatter::_M_error()
#6 0x7f00ff2a425f std::__debug::vector\u003C>::front()
#7 0x7f00ff2a2892 ui::Clipboard::DispatchObject()
#8 0x7f00ff2ac183 ui::ClipboardAura::WriteObjects()
#9 0x7f00ff2b524f ui::ScopedClipboardWriter::~ScopedClipboardWriter()
#10 0x0000007b6216 bookmarks::(anonymous
namespace)::BookmarkUtilsTest_PasteBookmarkFromURL_Test::TestBody()
#11 0x000000ff3af3
testing::internal::HandleSehExceptionsInMethodIfSupported\u003C>()
#12 0x000000fe873e
testing::internal::HandleExceptionsInMethodIfSupported\u003C>()
#13 0x000000fdd5b5 testing::Test::Run()
#14 0x000000fddcab testing::TestInfo::Run()
#15 0x000000fde29a testing::TestCase::Run()
#16 0x000000fe3793 testing::internal::UnitTestImpl::RunAllTests()
#17 0x000000ff0db3
testing::internal::HandleSehExceptionsInMethodIfSupported\u003C>()
#18 0x000000fe9f4e
testing::internal::HandleExceptionsInMethodIfSupported\u003C>()
#19 0x000000fe3431 testing::UnitTest::Run()
#20 0x000000f7d511 RUN_ALL_TESTS()
#21 0x000000f7c4e7 base::TestSuite::Run()
#22 0x000000762b22 base::internal::RunnableAdapter\u003C>::Run()
#23 0x000000c5833f base::internal::InvokeHelper\u003C>::MakeItSo()
#24 0x000000c582ea base::internal::Invoker\u003C>::Run()
#25 0x0000007f099e base::Callback\u003C>::Run()
#26 0x000000f70299 base::(anonymous namespace)::LaunchUnitTestsInternal()
#27 0x000000f6ffd7 base::LaunchUnitTests()
#28 0x000000c58035 main
#29 0x7f00eb57b76d __libc_start_main
#30 0x000000533a55 \u003Cunknown>
  r8: 00007f00e81a38c0  r9: 00007fff75ebfcd8 r10: 0000000000000008 r11:
0000000000000202
 r12: 00007fff75ebff60 r13: 0000000000000000 r14: 0000000000000001 r15:
0000000000000000
  di: 00000000000031b5  si: 00000000000031b5  bp: 0000000000000001  bx:
00007fff75ebff28
  dx: 0000000000000006  ax: 0000000000000000  cx: ffffffffffffffff  sp:
00007fff75ebfd28
  ip: 00007f00eb590425 efl: 0000000000000202 cgf: 0000000000000033 erf:
0000000000000000
 trp: 0000000000000000 msk: 0000000000000000 cr2: 0000000000000000

BookmarkUtilsTest.PasteBookmarkFromURL (run #3):
[ RUN      ] BookmarkUtilsTest.PasteBookmarkFromURL
/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/debug/vector:336:
    error: attempt to access an element in an empty container.

Objects involved in the operation:
sequence "this" @ 0x0x3c55f5e901e0 {
}
Received signal 6
#0 0x7fed3784398e base::debug::StackTrace::StackTrace()
#1 0x7fed378434c3 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7fed2b036cb0 \u003Cunknown>
#3 0x7fed2769c425 gsignal
#4 0x7fed2769fb8b abort
#5 0x7fed27c9e5ad __gnu_debug::_Error_formatter::_M_error()
#6 0x7fed3b3b025f std::__debug::vector\u003C>::front()
#7 0x7fed3b3ae892 ui::Clipboard::DispatchObject()
#8 0x7fed3b3b8183 ui::ClipboardAura::WriteObjects()
#9 0x7fed3b3c124f ui::ScopedClipboardWriter::~ScopedClipboardWriter()
#10 0x0000007b6216 bookmarks::(anonymous
namespace)::BookmarkUtilsTest_PasteBookmarkFromURL_Test::TestBody()
#11 0x000000ff3af3
testing::internal::HandleSehExceptionsInMethodIfSupported\u003C>()
#12 0x000000fe873e
testing::internal::HandleExceptionsInMethodIfSupported\u003C>()
#13 0x000000fdd5b5 testing::Test::Run()
#14 0x000000fddcab testing::TestInfo::Run()
#15 0x000000fde29a testing::TestCase::Run()
#16 0x000000fe3793 testing::internal::UnitTestImpl::RunAllTests()
#17 0x000000ff0db3
testing::internal::HandleSehExceptionsInMethodIfSupported\u003C>()
#18 0x000000fe9f4e
testing::internal::HandleExceptionsInMethodIfSupported\u003C>()
#19 0x000000fe3431 testing::UnitTest::Run()
#20 0x000000f7d511 RUN_ALL_TESTS()
#21 0x000000f7c4e7 base::TestSuite::Run()
#22 0x000000762b22 base::internal::RunnableAdapter\u003C>::Run()
#23 0x000000c5833f base::internal::InvokeHelper\u003C>::MakeItSo()
#24 0x000000c582ea base::internal::Invoker\u003C>::Run()
#25 0x0000007f099e base::Callback\u003C>::Run()
#26 0x000000f70299 base::(anonymous namespace)::LaunchUnitTestsInternal()
#27 0x000000f6ffd7 base::LaunchUnitTests()
#28 0x000000c58035 main
#29 0x7fed2768776d __libc_start_main
#30 0x000000533a55 \u003Cunknown>
  r8: 00007fed242af8c0  r9: 00007fff448a7b98 r10: 0000000000000008 r11:
0000000000000202
 r12: 00007fff448a7e20 r13: 0000000000000000 r14: 0000000000000001 r15:
0000000000000000
  di: 00000000000031b6  si: 00000000000031b6  bp: 0000000000000001  bx:
00007fff448a7de8
  dx: 0000000000000006  ax: 0000000000000000  cx: ffffffffffffffff  sp:
00007fff448a7be8
  ip: 00007fed2769c425 efl: 0000000000000202 cgf: 0000000000000033 erf:
0000000000000000
 trp: 0000000000000000 msk: 0000000000000000 cr2: 0000000000000000

BookmarkUtilsTest.PasteBookmarkFromURL (run #4):
[ RUN      ] BookmarkUtilsTest.PasteBookmarkFromURL
/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/debug/vector:336:
    error: attempt to access an element in an empty container.

Objects involved in the operation:
sequence "this" @ 0x0x116d121c1e0 {
}
Received signal 6
#0 0x7f155f68d98e base::debug::StackTrace::StackTrace()
#1 0x7f155f68d4c3 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f1552e80cb0 \u003Cunknown>.