chromiumembedded / cef

Chromium Embedded Framework (CEF). A simple framework for embedding Chromium-based browsers in other applications.
https://bitbucket.org/chromiumembedded/cef/
Other
3.31k stars 463 forks source link

Linux: Crash in BitmapContentLayerUpdater destructor with off-screen rendering and CefDoMessageLoopWork #1446

Closed magreenblatt closed 9 years ago

magreenblatt commented 9 years ago

Original report by me.


Original issue 1446 created by magreenblatt on 2014-11-20T22:49:15.000Z:

Ubuntu 12.10 64-bit
CEF version 3.2171.1902
JCEF revision 112

What steps will reproduce the problem?

  1. Embed CEF in another framework (like JCEF) with off-screen rendering and CefDoMessageLoopWork.
  2. Show the off-screen browser.

What is the expected output? What do you see instead?
The browser should render the first frame successfully. Instead, get the following crash:

Program received signal SIGABRT, Aborted.
[Switching to Thread 0x7fffa2646700 (LWP 12447)]
0x00007ffff7410f77 in __GI_raise (sig=sig@ entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
56 ../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
comment 0. 0x00007ffff7410f77 in __GI_raise (sig=sig@ entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
comment 1. 0x00007ffff74145e8 in __GI_abort () at abort.c:90
comment 2. 0x00007ffff744e4fb in __libc_message (do_abort=do_abort@ entry=2,
fmt=fmt@ entry=0x7ffff7562240 "*** Error in `%s': %s: 0x%s ***\n") at ../sysdeps/unix/sysv/linux/libc_fatal.c:199
comment 3. 0x00007ffff745a996 in malloc_printerr (ptr=0x1d13ad9f6000, str=0x7ffff755e205 "free(): invalid pointer", action=3)
at malloc.c:4923
comment 4. _int_free (av=, p=0x1d13ad9f5ff0, have_lock=0) at malloc.c:3779
comment 5. 0x00007fffd8b3e7d5 in SkMallocPixelRef::~SkMallocPixelRef() ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
comment 6. 0x00007fffd8b004cf in SkBitmap::~SkBitmap() ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
comment 7. 0x00007fffd9f7cc33 in cc::BitmapContentLayerUpdater::~BitmapContentLayerUpdater() ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
comment 8. 0x00007fffd9f2ec4a in cc::ContentLayer::~ContentLayer() ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
comment 9. 0x00007fffda5c3de2 in ui::Layer::SwitchToLayer(scoped_refptr) ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
comment 10. 0x00007fffda5c43e7 in ui::Layer::SetShowDelegatedContent(cc::DelegatedFrameProvider*, gfx::Size) ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
comment 11. 0x00007fffdaa368dd in content::DelegatedFrameHost::SwapDelegatedFrame(unsigned int, scoped_ptr<cc::DelegatedFrameData, base::DefaultDeleter >, float, std::vector<ui::LatencyInfo, std::allocator > const&) ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
comment 12. 0x00007fffd8744320 in CefRenderWidgetHostViewOSR::OnSwapCompositorFrame(unsigned int, scoped_ptr<cc::CompositorFrame, base::DefaultDeleter >) () from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
comment 13. 0x00007fffda96c391 in content::RenderWidgetHostImpl::OnSwapCompositorFrame(IPC::Message const&) ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
comment 14. 0x00007fffda96b4ba in content::RenderWidgetHostImpl::OnMessageReceived(IPC::Message const&) ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
comment 15. 0x00007fffda9659e1 in content::RenderViewHostImpl::OnMessageReceived(IPC::Message const&) ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
comment 16. 0x00007fffda95996e in content::RenderProcessHostImpl::OnMessageReceived(IPC::Message const&) ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
comment 17. 0x00007fffda11efec in IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message const&) ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
comment 18. 0x00007fffd87f6a72 in base::debug::TaskAnnotator::RunTask(char const*, char const*, base::PendingTask const&) ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
comment 19. 0x00007fffd88218e2 in base::MessageLoop::RunTask(base::PendingTask const&) ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
comment 20. 0x00007fffd8821e4c in base::MessageLoop::DoWork() ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
---Type to continue, or q to quit---
comment 21. 0x00007fffd87e5f0a in base::(anonymous namespace)::WorkSourceDispatch(_GSource*, int (*)(void*), void*) ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
comment 22. 0x00007fffe20833b6 in g_main_dispatch (context=0x7fff9401aa20) at /build/buildd/glib2.0-2.38.1/./glib/gmain.c:3065
comment 23. g_main_context_dispatch (context=context@ entry=0x7fff9401aa20) at /build/buildd/glib2.0-2.38.1/./glib/gmain.c:3641
comment 24. 0x00007fffe2083708 in g_main_context_iterate (context=context@ entry=0x7fff9401aa20, block=block@ entry=0,
dispatch=dispatch@ entry=1, self=) at /build/buildd/glib2.0-2.38.1/./glib/gmain.c:3712
comment 25. 0x00007fffe20837ac in g_main_context_iteration (context=0x7fff9401aa20, may_block=0)
at /build/buildd/glib2.0-2.38.1/./glib/gmain.c:3773
comment 26. 0x00007fffd87e5d6b in base::MessagePumpGlib::Run(base::MessagePump::Delegate*) ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
comment 27. 0x00007fffd8835884 in base::RunLoop::Run() ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so
comment 28. 0x00007fffd8719cba in CefBrowserMessageLoop::DoMessageLoopIteration() ()
from /home/marshall/code/jcef/src/binary_distrib/linux64/bin/lib/linux64/libcef.so

Please use labels and text to provide additional information.
This forum thread describes a similar crash: http://www.magpcss.org/ceforum/viewtopic.php?f=6&t=12225

magreenblatt commented 9 years ago

Original comment by Michael S (Bitbucket: mschuresko).


I have the exact same crash. My stack trace is nearly identical (possibly exactly identical)

It occurs (for me) only if WasResized was called after the first call to CefDoMessageLoopWork.

The actual crash occurs on a call to CefDoMessageLoopWork -- but not necessarily any particlar call to CefDoMessageLoopWork after the offending call to WasResized.

If I go into the Skia graphics library of Chromium (in third_party) and change SkMallocPixelRef::~SkMallocPixelRef to no longer call fReleaseProc then I no longer get the crash.

I presume, however, that the change I suggest causes me to leak memory, and is not an acceptable fix. I am looking around at where CEF might accidentally grab a pointer to the pixels wrapped by the SkMallocPixelRef and free them before SkMallocPixelRef does.

I am pretty sure that, in the case of my setup with my crash, fReleaseProc is just a wrapper around "free"

magreenblatt commented 9 years ago

Original comment by Michael S (Bitbucket: mschuresko).


Hey, I've figured out a bit more about this problem.

Apparently a chunk of memory that was allocated with tc_malloc is later freed with plain old vanilla system free.

If one goes into base/memory_linux.cc into UncheckedMalloc and replaces

*result = tc_malloc_skip_new_handler_weak(size);

with

*result = malloc(size);

the crash "goes away"

Again, I don't recommend this as a fix, but as proof that the "free" in "sk_free" in /chromium/src/skia/ext/SkMemory_new_handler.cpp

is matched to a "tc_malloc" is "sk_malloc"

Now I need to figure out why this happens for me, and what the best way to modify CEF to make it stop would be.

magreenblatt commented 9 years ago

Original comment by Michael S (Bitbucket: mschuresko).


By the way, is anyone reading this?

If I find a good clean fix, and it happens to be in Chromium's extensions to skia (chromium/src/skia/ext/SkWhatever) do I submit the fix to Chromium? Or do I also send a patch to CEF so the CEF build system can patch it in?

magreenblatt commented 9 years ago

@r_transpose_p :

Apparently a chunk of memory that was allocated with tc_malloc is later freed with plain old vanilla system free.

Can you debug how fStorage is being allocated and how fReleaseProc is being assigned to understand why it's mixing the system allocator and tcmalloc? Guessing, it might be an ordering issue (perhaps one is being set before CEF is initialized) or a threading issue (perhaps one is being called on a thread where tcmalloc hooks are not installed).

If I find a good clean fix, and it happens to be in Chromium's extensions to skia (chromium/src/skia/ext/SkWhatever) do I submit the fix to Chromium? Or do I also send a patch to CEF so the CEF build system can patch it in?

One or both depending on what you find as the root cause.

magreenblatt commented 9 years ago

Original comment by Michael S (Bitbucket: mschuresko).


So, I think these are both getting set to the functions sk_malloc_nothrow and sk_free in chromium/src/skia/ext/SkMemory_new_handler.cpp

Inside of that, sk_free is just a wrapper around free

but sk_malloc_nothrow

can be malloc or tc_malloc_skip_new_handler_weak (via base::UncheckedMalloc).

tc_malloc_skip_new_handler_weak is aliased to malloc unless tc_malloc_skip_new_handler is present, in which case it is that.

So, if both malloc and free are aliased to their tc_malloc variants, or if tc_malloc_skip_new_handler isn't loaded and doesn't override the alias of tc_malloc_skip_new_handler, or if the #if surrounding tc_malloc_skip_new_handler_weak isn't true, everything should be fine. It is reasonable to hypothesize that standalone chrome might be aliasing malloc and free whenever tc_malloc is present, thus obviating this mismatch problem.

More details follow :

When the memory gets allocated, it goes through

SkMallocPixelRef::NewAllocate

which passes

sk_free_releaseproc

which is a static local method that is just a wrapper around sk_free, which is defined, in

/chromium/src/skia/ext/SkMemory_new_handler.cpp

which is just a wrapper around free.

The allocation for SkMallocPixelRef::NewAllocate

allocates memory with

void* addr = sk_malloc_flags (size, 0);

which calls

sk_malloc_nothrow(size);

which calls either

base::UncheckedMalloc

or

malloc

depending on

#if  defined(LIBC_GLIBC) || defined(USE_TCMALLOC) || \
     (defined(OS_MACOSX) && !defined(OS_IOS)) || defined(OS_ANDROID)

For me, it is taking the base::UncheckedMalloc path, which winds up calling,well, here is my annotated UncheckedMalloc

bool UncheckedMalloc(size_t size, void** result) {
#if defined(MEMORY_TOOL_REPLACES_ALLOCATOR) || \
    (!defined(LIBC_GLIBC) && !defined(USE_TCMALLOC))
  THIS DOES NOT GET BUILT
  *result = malloc(size);
#elif defined(LIBC_GLIBC) && !defined(USE_TCMALLOC)
  THIS ALSO DOES NOT GET BUILT
  *result = __libc_malloc(size);
#elif defined(USE_TCMALLOC)
  // This gets built
  fprintf (stderr, "tc_malloc_skip_new_handler_weak is %p\n",
       &tc_malloc_skip_new_handler_weak);
  *result = tc_malloc_skip_new_handler_weak(size);
  // If I replace the line above with
  // *result = malloc(size);
  // I stop crashing.
#endif
  return *result != NULL;
}

tc_malloc_skip_new_handler

is initially set to

#if defined(USE_TCMALLOC)
// Used by UncheckedMalloc. If tcmalloc is linked to the executable
// this will be replaced by a strong symbol that actually implement
// the semantics and don't call new handler in case the allocation fails.
extern "C" {

__attribute__((weak, visibility("default")))
void* tc_malloc_skip_new_handler_weak(size_t size);

void* tc_malloc_skip_new_handler_weak(size_t size) {
  fprintf (stderr, "IN TC_MALLOC_SKIP_NEW, allocating %zu, malloc is %p\n",
       size, &malloc);
  return malloc(size);
}

}
#endif

and overridden with

#if defined(OS_LINUX)
extern "C" void* PERFTOOLS_DLL_DECL tc_malloc_skip_new_handler(size_t size) {
  void* result = do_malloc(size);
  MallocHook::InvokeNewHook(result, size);
  return result;
}
#endif

#endif  // TCMALLOC_USING_DEBUGALLOCATION

#if defined(OS_LINUX)
// Alias the weak symbol in chromium to our implementation.
extern "C" __attribute__((visibility("default"), alias("tc_malloc_skip_new_handler")))
void* tc_malloc_skip_new_handler_weak(size_t size);
#endif
magreenblatt commented 9 years ago

Original comment by Michael S (Bitbucket: mschuresko).


My proposal is to make a matching chain of calls ending in sk_free that exactly mirrors the path through base::UncheckedMalloc that sk_malloc_flags takes.

The problem with this is that sk_malloc_flags can either call sk_malloc_nothrow (which sometimes calls tc_malloc) or sk_malloc_throw (which always calls malloc)

So we'd probably have to change sk_malloc_throw as well.

magreenblatt commented 9 years ago

Original comment by Michael S (Bitbucket: mschuresko).


Hey, I have a "works for me on linux" version of that thing I outlined above. I'll clean it up and send it to you (I mean, I'll ask my boss first or something, but whatever) tomorrow.

magreenblatt commented 9 years ago

Original comment by Michael S (Bitbucket: mschuresko).


aw crud. It looks like my fixes are all intermingled with whatever patches CEF applies to chromium. I do not understand how that (CEF patching chromium) works at all.

magreenblatt commented 9 years ago

Original comment by Michael S (Bitbucket: mschuresko).


Marshall, what is the best way to send you a patch?

magreenblatt commented 9 years ago

Original comment by Michael S (Bitbucket: mschuresko).


#!patch

From [6ea2b9117e417d46263830b8e0bb90ddfb333662 (bb)](https://bitbucket.org/chromiumembedded/cef/commits/6ea2b9117e417d46263830b8e0bb90ddfb333662) Mon Sep 17 00:00:00 2001
From: Michael D Schuresko <mschuresko@oblong.com>
Date: Tue, 21 Apr 2015 20:01:53 -0700
Subject: [PATCH] changes to make malloc/free match

---
 base/process/memory.h                         |    2 ++
 base/process/memory_linux.cc                  |   32 +++++++++++++++++++++++++
 base/process/memory_mac.mm                    |   20 ++++++++++++++++
 base/process/memory_win.cc                    |    4 ++++
 skia/ext/SkMemory_new_handler.cpp             |   32 +++++++++++++++++++++----
 third_party/tcmalloc/chromium/src/tcmalloc.cc |    7 ++++++
 6 files changed, 93 insertions(+), 4 deletions(-)

diff --git a/base/process/memory.h b/base/process/memory.h
index [100d9c7 (bb)](https://bitbucket.org/chromiumembedded/cef/commits/100d9c7)..cb914a1 [100644 (bb)](https://bitbucket.org/chromiumembedded/cef/commits/100644)
--- a/base/process/memory.h
+++ b/base/process/memory.h
@@ -77,6 +77,8 @@ BASE_EXPORT WARN_UNUSED_RESULT bool UncheckedCalloc(size_t num_items,
                                                     size_t size,
                                                     void** result);

+BASE_EXPORT void UncheckedFree(void** free_me);
+
 }  // namespace base

 #endif  // BASE_PROCESS_MEMORY_H_
diff --git a/base/process/memory_linux.cc b/base/process/memory_linux.cc
index [6dbe8b7 (bb)](https://bitbucket.org/chromiumembedded/cef/commits/6dbe8b7)..d7e2748 [100644 (bb)](https://bitbucket.org/chromiumembedded/cef/commits/100644)
--- a/base/process/memory_linux.cc
+++ b/base/process/memory_linux.cc
@@ -22,12 +22,30 @@ __attribute__((weak, visibility("default")))
 void* tc_malloc_skip_new_handler_weak(size_t size);

 void* tc_malloc_skip_new_handler_weak(size_t size) {
+  fprintf (stderr, "IN TC_MALLOC_SKIP_NEW, allocating %zu, malloc is %p\n",
+      size, &malloc);
   return malloc(size);
 }

 }
 #endif

+#if defined(USE_TCMALLOC)
+// Used by UncheckedFree. If tcmalloc is linked to the executable
+// this will be replaced by a strong symbol that actually implement
+// the semantics and don't call new handler in case the allocation fails.
+extern "C" {
+
+__attribute__((weak, visibility("default")))
+void tc_free_weak(void *free_me);
+
+void tc_free_weak(void *free_me) {
+  free(free_me);
+}
+
+}
+#endif
+
 namespace base {

 size_t g_oom_size = 0U;
@@ -201,8 +219,10 @@ bool AdjustOOMScore(ProcessId process, int score) {
 bool UncheckedMalloc(size_t size, void** result) {
 #if defined(MEMORY_TOOL_REPLACES_ALLOCATOR) || \
     (!defined(LIBC_GLIBC) && !defined(USE_TCMALLOC))
+
   *result = malloc(size);
 #elif defined(LIBC_GLIBC) && !defined(USE_TCMALLOC)
+
   *result = __libc_malloc(size);
 #elif defined(USE_TCMALLOC)
   *result = tc_malloc_skip_new_handler_weak(size);
@@ -210,4 +230,16 @@ bool UncheckedMalloc(size_t size, void** result) {
   return *result != NULL;
 }

+void UncheckedFree(void** free_me) {
+#if defined(MEMORY_TOOL_REPLACES_ALLOCATOR) || \
+    (!defined(LIBC_GLIBC) && !defined(USE_TCMALLOC))
+  free(*free_me);
+#elif defined(LIBC_GLIBC) && !defined(USE_TCMALLOC)
+  __libc_free(*free_me);
+#elif defined(USE_TCMALLOC)
+  tc_free_weak(*free_me);
+#endif
+  *free_me = NULL;
+}
+
 }  // namespace base
diff --git a/base/process/memory_mac.mm b/base/process/memory_mac.mm
index [e59e63f (bb)](https://bitbucket.org/chromiumembedded/cef/commits/e59e63f)..6ef3a3a [100644 (bb)](https://bitbucket.org/chromiumembedded/cef/commits/100644)
--- a/base/process/memory_mac.mm
+++ b/base/process/memory_mac.mm
@@ -516,6 +516,24 @@ id oom_killer_allocWithZone(id self, SEL _cmd, NSZone* zone)

 }  // namespace

+void UncheckedFree(void** free_me) {
+#if defined(ADDRESS_SANITIZER)
+  free(*free_me);
+#else
+  if (g_old_free) {
+#if defined(HANDLE_MEMORY_CORRUPTION_MANUALLY)
+    ScopedClearErrno clear_errno;
+    ThreadLocalBooleanAutoReset flag(g_unchecked_alloc.Pointer(), true);
+#endif  // defined(HANDLE_MEMORY_CORRUPTION_MANUALLY)
+    g_old_free(malloc_default_zone(), *free_me);
+  } else {
+    free(*free_me);
+  }
+#endif  // defined(ADDRESS_SANITIZER)
+
+  *free_me = NULL;
+}
+
 bool UncheckedMalloc(size_t size, void** result) {
 #if defined(ADDRESS_SANITIZER)
   *result = malloc(size);
@@ -552,6 +570,8 @@ bool UncheckedCalloc(size_t num_items, size_t size, void** result) {
   return *result != NULL;
 }

+
+
 void* UncheckedMalloc(size_t size) {
   void* address;
   return UncheckedMalloc(size, &address) ? address : NULL;
diff --git a/base/process/memory_win.cc b/base/process/memory_win.cc
index [668214c (bb)](https://bitbucket.org/chromiumembedded/cef/commits/668214c)..25ae18e [100644 (bb)](https://bitbucket.org/chromiumembedded/cef/commits/100644)
--- a/base/process/memory_win.cc
+++ b/base/process/memory_win.cc
@@ -90,7 +90,11 @@ HMODULE GetModuleFromAddress(void* address) {
 // functions.
 bool UncheckedMalloc(size_t size, void** result) {
   *result = malloc(size);
+
   return *result != NULL;
 }

+void UncheckedFree(void** free_me) {
+  free(*free_me);
+}
 }  // namespace base
diff --git a/skia/ext/SkMemory_new_handler.cpp b/skia/ext/SkMemory_new_handler.cpp
index [015521f (bb)](https://bitbucket.org/chromiumembedded/cef/commits/015521f)..f571c0d [100644 (bb)](https://bitbucket.org/chromiumembedded/cef/commits/100644)
--- a/skia/ext/SkMemory_new_handler.cpp
+++ b/skia/ext/SkMemory_new_handler.cpp
@@ -36,19 +36,28 @@ void sk_out_of_memory(void) {
     abort();
 }

+///// TODO : mschuresko : is there a version of realloc
+///// that follows teh behavior of base::UncheckedMalloc
 void* sk_realloc_throw(void* addr, size_t size) {
     return throw_on_failure(size, realloc(addr, size));
 }

 void sk_free(void* p) {
+#if  defined(LIBC_GLIBC) || defined(USE_TCMALLOC) || \
+     (defined(OS_MACOSX) && !defined(OS_IOS)) || defined(OS_ANDROID)
+
+    if (p) {
+        base::UncheckedFree(&p);
+    }
+
+#else
     if (p) {
         free(p);
     }
+#endif
 }

-void* sk_malloc_throw(size_t size) {
-    return throw_on_failure(size, malloc(size));
-}
+

 static void* sk_malloc_nothrow(size_t size) {
     // TODO(b.kelemen): we should always use UncheckedMalloc but currently it
@@ -58,10 +67,14 @@ static void* sk_malloc_nothrow(size_t size) {
     void* result;
     // It's the responsibility of the caller to check the return value.
     ignore_result(base::UncheckedMalloc(size, &result));
+
+
     return result;
 #else
     // This is not really thread safe.  It only won't collide with itself, but we're totally
     // unprotected from races with other code that calls set_new_handler.
+
+
     SkAutoMutexAcquire lock(gSkNewHandlerMutex);
     std::new_handler old_handler = std::set_new_handler(NULL);
     void* p = malloc(size);
@@ -70,6 +83,15 @@ static void* sk_malloc_nothrow(size_t size) {
 #endif
 }

+/// mschuresko@oblong.com made behavior mimic sk_malloc_nothrow wrt.
+/// "malloc vs. tc_malloc"
+void* sk_malloc_throw(size_t size) {
+    return throw_on_failure(size, sk_malloc_nothrow(size));
+    /// mschuresko and here we differ from sk_malloc_nothrow by
+    /// potentially throwing.
+}
+
+
 void* sk_malloc_flags(size_t size, unsigned flags) {
     if (flags & SK_MALLOC_THROW) {
         return sk_malloc_throw(size);
@@ -77,8 +99,10 @@ void* sk_malloc_flags(size_t size, unsigned flags) {
     return sk_malloc_nothrow(size);
 }

+/// mschuresko@oblong.com made behavior mimic sk_calloc_nothrow wrt.
+/// "malloc vs. tc_malloc"
 void* sk_calloc_throw(size_t size) {
-    return throw_on_failure(size, calloc(size, 1));
+    return throw_on_failure(size, sk_calloc(size));
 }

 void* sk_calloc(size_t size) {
diff --git a/third_party/tcmalloc/chromium/src/tcmalloc.cc b/third_party/tcmalloc/chromium/src/tcmalloc.cc
index [c8a705f (bb)](https://bitbucket.org/chromiumembedded/cef/commits/c8a705f)..0c93571 [100644 (bb)](https://bitbucket.org/chromiumembedded/cef/commits/100644)
--- a/third_party/tcmalloc/chromium/src/tcmalloc.cc
+++ b/third_party/tcmalloc/chromium/src/tcmalloc.cc
@@ -1572,6 +1572,13 @@ extern "C" PERFTOOLS_DLL_DECL void tc_free(void* ptr) __THROW {
   do_free(ptr);
 }

+#if defined(OS_LINUX)
+// Alias the weak symbol in chromium to our implementation.
+extern "C" __attribute__((visibility("default"), alias("tc_free")))
+void* tc_free_weak(void *ptr);
+#endif
+
+
 extern "C" PERFTOOLS_DLL_DECL void* tc_calloc(size_t n,
                                               size_t elem_size) __THROW {
   void* result = do_calloc(n, elem_size);
-- 
1.7.9.5
magreenblatt commented 9 years ago

Original comment by Michael S (Bitbucket: mschuresko).


Oops, one thing I didn't consider : It is fairly obvious that freeing a chunk of memory allocated with tcmalloc with system free creates a problem : but is it even safe to use tcmalloc and system malloc in the same program at all? If both are using brk/sbrk, it would seem not.

However, I don't get crashes when I use both together. Are they designed such that "does not crash" is expected behavior, or am I just observing a lucky coincidence?

magreenblatt commented 9 years ago

Original comment by Robert Chudoba (Bitbucket: Robert C).


After applying the patch i get this crash now when I open pages with video elements:

#!
#0  0x00007f1eac8422f0 in tcmalloc::Abort() () from ./libcef.so
(gdb)
(gdb) bt
#0  0x00007f1eac8422f0 in tcmalloc::Abort() () from ./libcef.so
#1  0x00007f1eac852653 in tcmalloc::Log(tcmalloc::LogMode, char const*, int, tcmalloc::LogItem, tcmalloc::LogItem, tcmalloc::LogItem, tcmalloc::LogItem) () from ./libcef.so
#2  0x00007f1eac85e095 in (anonymous namespace)::do_free_with_callback(void*, void (*)(void*)) () from ./libcef.so
#3  0x00007f1eac8aa42c in base::UncheckedFree(void**) () from ./libcef.so
#4  0x00007f1eacd29623 in sk_free(void*) () from ./libcef.so
#5  0x00007f1eacbc5787 in SkPathRef::~SkPathRef() () from ./libcef.so
#6  0x00007f1eacba4bb2 in SkDraw::drawRect(SkRect const&, SkPaint const&) const () from ./libcef.so
#7  0x00007f1eacba5fd6 in SkDraw::drawBitmap(SkBitmap const&, SkMatrix const&, SkPaint const&) const () from ./libcef.so
#8  0x00007f1eacb786fa in SkBitmapDevice::drawBitmapRect(SkDraw const&, SkBitmap const&, SkRect const*, SkRect const&, SkPaint const&, SkCanvas::DrawBitmapRectFlags) () from ./libcef.so
#9  0x00007f1eacb99124 in SkCanvas::internalDrawBitmapRect(SkBitmap const&, SkRect const*, SkRect const&, SkPaint const*, SkCanvas::DrawBitmapRectFlags) () from ./libcef.so
#10 0x00007f1eadfc42ff in cc::SoftwareRenderer::DrawTileQuad(cc::DirectRenderer::DrawingFrame const*, cc::TileDrawQuad const*) () from ./libcef.so
#11 0x00007f1eadfc2f12 in cc::SoftwareRenderer::DoDrawQuad(cc::DirectRenderer::DrawingFrame*, cc::DrawQuad const*) () from ./libcef.so
#12 0x00007f1eadfa5670 in cc::DirectRenderer::DrawRenderPass(cc::DirectRenderer::DrawingFrame*, cc::RenderPass const*) () from ./libcef.so
#13 0x00007f1eadfa4fa2 in cc::DirectRenderer::DrawFrame(cc::ScopedPtrVector<cc::RenderPass>*, float, gfx::Rect const&, gfx::Rect const&, bool) ()
   from ./libcef.so
#14 0x00007f1eae02318c in cc::LayerTreeHostImpl::DrawLayers(cc::LayerTreeHostImpl::FrameData*, base::TimeTicks) () from ./libcef.so
#15 0x00007f1eae03540c in cc::SingleThreadProxy::DoComposite(base::TimeTicks, cc::LayerTreeHostImpl::FrameData*) () from ./libcef.so
#16 0x00007f1eae035187 in cc::SingleThreadProxy::CompositeImmediately(base::TimeTicks) () from ./libcef.so
#17 0x00007f1eae5f53be in ui::Compositor::Draw() () from ./libcef.so
#18 0x00007f1eac86fe22 in base::debug::TaskAnnotator::RunTask(char const*, char const*, base::PendingTask const&) () from ./libcef.so
#19 0x00007f1eac89a8e2 in base::MessageLoop::RunTask(base::PendingTask const&) () from ./libcef.so
#20 0x00007f1eac89ae4c in base::MessageLoop::DoWork() () from ./libcef.so
#21 0x00007f1eac85f329 in base::MessagePumpGlib::Run(base::MessagePump::Delegate*) () from ./libcef.so
#22 0x00007f1eac8ad514 in base::RunLoop::Run() () from ./libcef.so
#23 0x00007f1eac793bda in CefBrowserMessageLoop::DoMessageLoopIteration() () from ./libcef.so
#24 0x0000000040bff0aa in ?? ()
#25 0x0000000000000000 in ?? ()
magreenblatt commented 9 years ago

Original comment by Michael S (Bitbucket: mschuresko).


Thanks Robert. I don't think I had tested pages with video elements.

It is likely that you are now using tc_free to free an element allocated with system malloc.

I am currently working on other things, so I doubt I'll get around to looking at that anytime soon : on our end we are trying to compile chromium and CEF in such a way as to force tc_malloc not to be used.

An interesting idea for a quick check would be to see if all of your malloc/free/tcmalloc/tcfree problems go away if you run your application with LD_PRELOAD="/usr/lib/libtcmalloc.so" as suggested on http://goog-perftools.sourceforge.net/doc/tcmalloc.html

magreenblatt commented 9 years ago

Original comment by Robert Chudoba (Bitbucket: Robert C).


Thanks for the help Michael. Anyway, I "fixed" that problem by using the 2357 branch.

magreenblatt commented 9 years ago

Original comment by Michael S (Bitbucket: mschuresko).


Robert, when you say you fixed a problem by using the 2357 branch, do you mean the video element tcmalloc problem ,or the original skia free SkBitMap destructor problem?

If you mean the video element tcmalloc problem, does that mean you are using the above patch with 2357? (I am not sure whether that is even possible with the patch as presented, but hey)

On our end we have had this problem go away, and come back again due to (??? we're trying to figure that last part out).

magreenblatt commented 9 years ago

Original comment by Michael S (Bitbucket: mschuresko).


We found a fix on our end.

It turns out to be an issue with library load order, and whether libc.so overwrote the "malloc" and "free" symbols. CEF and Chromium only work if "free" means "tc_free" and "malloc" means "tc_malloc"

magreenblatt commented 9 years ago

Original comment by Tom Jakubowski (Bitbucket: Tom Jakubowski).


To expand on Michael S's comment, we found that on Linux if libcef.so appeared before libc.so in the list of our executable binary's shared libraries (as listed by, say, ldd), all was well and we could not reproduce the crash. When libcef.so appeared after libc.so, we observed the crash in Skia under various circumstances.

In particular, we have a shared library which links to libcef.so, and our executable's build process linked directly to our shared library only, not to libcef. That pushed libcef.so after libc.so in the load order. Moving libcef.so ahead of libc.so in the shared library load order (either with LD_PRELOAD=/path/to/libcef.so at runtime, or by adjusting our build process) resolved the crash issue for us.

magreenblatt commented 9 years ago

Issue #1545 was marked as a duplicate of this issue.

magreenblatt commented 9 years ago

See above comments for possible resolutions.

magreenblatt commented 9 years ago