Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(393)

Issue 740003003: Revert of Rewrite clipboard write IPC handling to be easier to understand. (Closed)

Created:
6 years, 1 month ago by sky
Modified:
6 years, 1 month ago
CC:
chromium-reviews, darin-cc_chromium.org, Elliot Glaysher, jam, jln (very slow on Chromium), mkwst+moarreviews-renderer_chromium.org, piman
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Revert of Rewrite clipboard write IPC handling to be easier to understand. (patchset #29 id:860001 of https://codereview.chromium.org/574273002/) Reason for revert: Reverting as broke components_unittests on a couple of bots: https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/34215 https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/34215/steps/components_unittests/logs/PasteBookmarkFromURL 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> Original issue's description: > 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} TBR=avi@chromium.org,jamesr@chromium.org,wfh@chromium.org,dcheng@chromium.org NOTREECHECKS=true NOTRY=true BUG=319285 Committed: https://crrev.com/5f8381bf99d4438b160d7628230f3755cb838432 Cr-Commit-Position: refs/heads/master@{#304912}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+895 lines, -698 lines) Patch
M chrome/chrome_common.gypi View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/common/net/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M components/bookmarks/browser/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M content/browser/renderer_host/clipboard_message_filter.h View 4 chunks +6 lines, -29 lines 0 comments Download
M content/browser/renderer_host/clipboard_message_filter.cc View 6 chunks +94 lines, -106 lines 0 comments Download
D content/browser/renderer_host/clipboard_message_filter_unittest.cc View 1 chunk +0 lines, -132 lines 0 comments Download
M content/common/clipboard_messages.h View 3 chunks +9 lines, -31 lines 0 comments Download
M content/content_renderer.gypi View 2 chunks +4 lines, -2 lines 0 comments Download
M content/content_tests.gypi View 1 chunk +0 lines, -1 line 0 comments Download
A content/renderer/clipboard_client.h View 1 chunk +81 lines, -0 lines 0 comments Download
M content/renderer/renderer_blink_platform_impl.h View 2 chunks +2 lines, -2 lines 0 comments Download
M content/renderer/renderer_blink_platform_impl.cc View 2 chunks +3 lines, -3 lines 0 comments Download
A content/renderer/renderer_clipboard_client.h View 1 chunk +43 lines, -0 lines 0 comments Download
A content/renderer/renderer_clipboard_client.cc View 1 chunk +180 lines, -0 lines 0 comments Download
D content/renderer/renderer_clipboard_delegate.h View 1 chunk +0 lines, -66 lines 0 comments Download
D content/renderer/renderer_clipboard_delegate.cc View 1 chunk +0 lines, -165 lines 0 comments Download
A content/renderer/scoped_clipboard_writer_glue.h View 1 chunk +30 lines, -0 lines 0 comments Download
A content/renderer/scoped_clipboard_writer_glue.cc View 1 chunk +36 lines, -0 lines 0 comments Download
M content/renderer/webclipboard_impl.h View 2 chunks +4 lines, -3 lines 0 comments Download
M content/renderer/webclipboard_impl.cc View 11 chunks +59 lines, -48 lines 0 comments Download
M ui/base/clipboard/clipboard.h View 5 chunks +60 lines, -39 lines 0 comments Download
M ui/base/clipboard/clipboard.cc View 4 chunks +99 lines, -7 lines 0 comments Download
M ui/base/clipboard/clipboard_test_template.h View 3 chunks +178 lines, -26 lines 0 comments Download
M ui/base/clipboard/clipboard_util_win.cc View 1 chunk +0 lines, -1 line 0 comments Download
M ui/base/clipboard/scoped_clipboard_writer.h View 3 chunks +2 lines, -6 lines 0 comments Download
M ui/base/clipboard/scoped_clipboard_writer.cc View 2 chunks +0 lines, -18 lines 0 comments Download
M ui/base/test/test_clipboard.h View 1 chunk +1 line, -4 lines 0 comments Download
M ui/base/test/test_clipboard.cc View 1 chunk +2 lines, -4 lines 0 comments Download
M ui/base/test/test_clipboard_unittest.cc View 1 chunk +2 lines, -1 line 0 comments Download
M ui/base/ui_base.gyp View 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 4 (0 generated)
sky
Created Revert of Rewrite clipboard write IPC handling to be easier to understand.
6 years, 1 month ago (2014-11-19 22:43:43 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/740003003/1
6 years, 1 month ago (2014-11-19 22:44:50 UTC) #2
commit-bot: I haz the power
Committed patchset #1 (id:1)
6 years, 1 month ago (2014-11-19 22:46:31 UTC) #3
commit-bot: I haz the power
6 years, 1 month ago (2014-11-19 22:48:00 UTC) #4
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/5f8381bf99d4438b160d7628230f3755cb838432
Cr-Commit-Position: refs/heads/master@{#304912}

Powered by Google App Engine
This is Rietveld 408576698