Native/javascript shared ArrayBuffer crash

Having problems with building or using CEF's C/C++ APIs? This forum is here to help. Please do not post bug reports or feature requests here.

Re: Native/javascript shared ArrayBuffer crash

Postby sdiverdi » Mon Mar 22, 2021 5:49 pm

Thanks for turning that around so fast! It's good and bad news. Good news is for a single frame with no navigating, the problem is fixed, yes. Bad news is that if I navigate in the frame (the frame is my document window, and when I open a file URL, it loads the new URL in the frame), I now get a debug check fail after the buffer is successfully released. I believe this is because the BackingStore is deleted _after_ the old frame context has been cleared and the new context has been created (the check fail occurs a couple seconds after the new page loads). The precise sequence of events is this:

1. Frame loads initial URL, allocates array buffer, everything fine. Can change the resolution causing new array buffer allocations, and later GC sweeps release those buffer without incident.
2. Open file dialog selects a new file URL. Calls CefFrame::LoadURL.
3. ~V8TrackArrayBuffer is called, which calls release buffer callback successfully, stack trace:

release.jpg
release.jpg (284.44 KiB) Viewed 4727 times


4. The new URL is loaded -- I can see it render, and I can see a new array buffer allocation. It renders fine for a couple seconds.
5. A GC sweep calls ~BackingStore, which tests the weakptr in the custom deleter, which does a DCHECK on a "valid sequence", which fails. Stack trace:

fail.jpg
fail.jpg (305.42 KiB) Viewed 4727 times


So, I don't understand the semantics here: should the BackingStore linger after the context has been released, and asynchronously deleted later? It seems like probably not. But I'm not sure why the BackingStore wouldn't be deleted as part of the context being released. Also it still seems a bit weird that the V8TrackArrayBuffer and the BackingStore both might be responsible for calling the release buffer callback. But it seems like the V8TrackArrayBuffer does it successfully now and the BackingStore's sort-of-ownership of the buffer isn't necessary. Over the weekend I tested stubbing out the custom deleter for the BackingStore (see diff below) and this worked fine (though obviously further muddying the ownership semantics).

Code: Select all
--- chromium/src/v8/src/objects/backing-store.cc
+++ chromium/src/v8/src/objects/backing-store.cc
@@ -192,8 +192,10 @@ BackingStore::~BackingStore() {
     DCHECK(free_on_destruct_);
     TRACE_BS("BS:custom deleter bs=%p mem=%p (length=%zu, capacity=%zu)\n",
              this, buffer_start_, byte_length(), byte_capacity_);
-    type_specific_data_.deleter.callback(buffer_start_, byte_length_,
-                                         type_specific_data_.deleter.data);
+    if (type_specific_data_.deleter.callback) {
+      type_specific_data_.deleter.callback(buffer_start_, byte_length_,
+                                           type_specific_data_.deleter.data);
+    }
     Clear();
     return;
   }
--- chromium/src/cef/libcef/renderer/v8_impl.cc
+++ chromium/src/cef/libcef/renderer/v8_impl.cc
@@ -1416,15 +1416,15 @@ CefRefPtr<CefV8Value> CefV8Value::CreateArrayBuffer(
   V8TrackArrayBuffer* tracker =
       new V8TrackArrayBuffer(isolate, buffer, release_callback);
 
-  auto deleter = [](void* data, size_t length, void* deleter_data) {
+/*  auto deleter = [](void* data, size_t length, void* deleter_data) {
     auto* tracker = reinterpret_cast<V8TrackArrayBuffer*>(deleter_data);
     if (tracker) {
       tracker->ReleaseBuffer();
     }
-  };
+  };*/
 
   std::unique_ptr<v8::BackingStore> backing =
-      v8::ArrayBuffer::NewBackingStore(buffer, length, deleter, tracker);
+      v8::ArrayBuffer::NewBackingStore(buffer, length, nullptr, nullptr);
   v8::Local<v8::ArrayBuffer> ab =
       v8::ArrayBuffer::New(isolate, std::move(backing));

sdiverdi
Mentor
 
Posts: 51
Joined: Fri Dec 25, 2020 7:41 pm

Re: Native/javascript shared ArrayBuffer crash

Postby magreenblatt » Mon Mar 22, 2021 6:34 pm

Looks like the GC sweep is happening on a different thread, hence the "valid sequence" DCHECK.

So, I don't understand the semantics here: should the BackingStore linger after the context has been released, and asynchronously deleted later? It seems like probably not. But I'm not sure why the BackingStore wouldn't be deleted as part of the context being released. Also it still seems a bit weird that the V8TrackArrayBuffer and the BackingStore both might be responsible for calling the release buffer callback.

This is unclear to me as well. Maybe we should have the |deleter| function own the buffer instead of V8TrackArrayBuffer.
magreenblatt
Site Admin
 
Posts: 12382
Joined: Fri May 29, 2009 6:57 pm

Re: Native/javascript shared ArrayBuffer crash

Postby magreenblatt » Mon Mar 22, 2021 7:16 pm

magreenblatt wrote:Maybe we should have the |deleter| function own the buffer instead of V8TrackArrayBuffer.

I tried that, but there seems to be no way to know when the |deleter| function will be called (e.g. V8Test.ArrayBuffer is failing because |destructorCalled| and |releaseBufferCalled| are false). Here's the patch, if you'd like to try it.
Code: Select all
diff --git libcef/renderer/v8_impl.cc libcef/renderer/v8_impl.cc
index fd879fa79..30c998250 100644
--- libcef/renderer/v8_impl.cc
+++ libcef/renderer/v8_impl.cc
@@ -318,18 +318,14 @@ class V8TrackArrayBuffer : public CefTrackNode {
  public:
   explicit V8TrackArrayBuffer(
       v8::Isolate* isolate,
-      void* buffer,
       CefRefPtr<CefV8ArrayBufferReleaseCallback> release_callback)
-      : isolate_(isolate),
-        buffer_(buffer),
-        release_callback_(release_callback) {
+      : isolate_(isolate), release_callback_(release_callback) {
     DCHECK(isolate_);
     isolate_->AdjustAmountOfExternalAllocatedMemory(
         static_cast<int>(sizeof(V8TrackArrayBuffer)));
   }
 
   ~V8TrackArrayBuffer() {
-    ReleaseBuffer();
     isolate_->AdjustAmountOfExternalAllocatedMemory(
         -static_cast<int>(sizeof(V8TrackArrayBuffer)));
   }
@@ -338,15 +334,6 @@ class V8TrackArrayBuffer : public CefTrackNode {
     return release_callback_;
   }
 
-  void ReleaseBuffer() {
-    if (buffer_ && release_callback_) {
-      release_callback_->ReleaseBuffer(buffer_);
-    }
-    Detach();
-  }
-
-  void Detach() { buffer_ = nullptr; }
-
   // Attach this track object to the specified V8 object.
   void AttachTo(v8::Local<v8::Context> context,
                 v8::Local<v8::ArrayBuffer> arrayBuffer) {
@@ -367,7 +354,6 @@ class V8TrackArrayBuffer : public CefTrackNode {
 
  private:
   v8::Isolate* isolate_;
-  void* buffer_;
   CefRefPtr<CefV8ArrayBufferReleaseCallback> release_callback_;
 };
 
@@ -1414,17 +1400,22 @@ CefRefPtr<CefV8Value> CefV8Value::CreateArrayBuffer(
   // Create a tracker object that will cause the user data reference to be
   // released when the V8 object is destroyed.
   V8TrackArrayBuffer* tracker =
-      new V8TrackArrayBuffer(isolate, buffer, release_callback);
+      new V8TrackArrayBuffer(isolate, release_callback);
+
+  if (release_callback)
+    release_callback->AddRef();
 
   auto deleter = [](void* data, size_t length, void* deleter_data) {
-    auto* tracker = reinterpret_cast<V8TrackArrayBuffer*>(deleter_data);
-    if (tracker) {
-      tracker->ReleaseBuffer();
+    auto* release_callback =
+        reinterpret_cast<CefV8ArrayBufferReleaseCallback*>(deleter_data);
+    if (release_callback) {
+      release_callback->ReleaseBuffer(data);
+      release_callback->Release();
     }
   };
 
-  std::unique_ptr<v8::BackingStore> backing =
-      v8::ArrayBuffer::NewBackingStore(buffer, length, deleter, tracker);
+  std::unique_ptr<v8::BackingStore> backing = v8::ArrayBuffer::NewBackingStore(
+      buffer, length, deleter, release_callback.get());
   v8::Local<v8::ArrayBuffer> ab =
       v8::ArrayBuffer::New(isolate, std::move(backing));
 
@@ -2302,8 +2293,6 @@ bool CefV8ValueImpl::NeuterArrayBuffer() {
     return false;
   }
   arr->Detach();
-  V8TrackArrayBuffer* tracker = V8TrackArrayBuffer::Unwrap(context, obj);
-  tracker->Detach();
 
   return true;
 }
magreenblatt
Site Admin
 
Posts: 12382
Joined: Fri May 29, 2009 6:57 pm

Re: Native/javascript shared ArrayBuffer crash

Postby magreenblatt » Mon Mar 22, 2021 8:16 pm

Please also try this patch as an alternative to the above patch. This one gives buffer ownership strictly to V8TrackArrayBuffer and ignores the |deleter| function call. I think this will be the best fix unless your testing shows issues with it.
Code: Select all
diff --git libcef/renderer/v8_impl.cc libcef/renderer/v8_impl.cc
index fd879fa79..ecd4407b0 100644
--- libcef/renderer/v8_impl.cc
+++ libcef/renderer/v8_impl.cc
@@ -342,11 +342,9 @@ class V8TrackArrayBuffer : public CefTrackNode {
     if (buffer_ && release_callback_) {
       release_callback_->ReleaseBuffer(buffer_);
     }
-    Detach();
+    buffer_ = nullptr;
   }

-  void Detach() { buffer_ = nullptr; }
-
   // Attach this track object to the specified V8 object.
   void AttachTo(v8::Local<v8::Context> context,
                 v8::Local<v8::ArrayBuffer> arrayBuffer) {
@@ -1416,12 +1414,7 @@ CefRefPtr<CefV8Value> CefV8Value::CreateArrayBuffer(
   V8TrackArrayBuffer* tracker =
       new V8TrackArrayBuffer(isolate, buffer, release_callback);

-  auto deleter = [](void* data, size_t length, void* deleter_data) {
-    auto* tracker = reinterpret_cast<V8TrackArrayBuffer*>(deleter_data);
-    if (tracker) {
-      tracker->ReleaseBuffer();
-    }
-  };
+  auto deleter = [](void* data, size_t length, void* deleter_data) {};

   std::unique_ptr<v8::BackingStore> backing =
       v8::ArrayBuffer::NewBackingStore(buffer, length, deleter, tracker);
@@ -2303,7 +2296,7 @@ bool CefV8ValueImpl::NeuterArrayBuffer() {
   }
   arr->Detach();
   V8TrackArrayBuffer* tracker = V8TrackArrayBuffer::Unwrap(context, obj);
-  tracker->Detach();
+  tracker->ReleaseBuffer();

   return true;
 }
magreenblatt
Site Admin
 
Posts: 12382
Joined: Fri May 29, 2009 6:57 pm

Re: Native/javascript shared ArrayBuffer crash

Postby sdiverdi » Mon Mar 22, 2021 11:49 pm

Quick note, I got my wires crossed with some of the earlier tests. When I disable the BackingStore custom deleter (as in the patch at the end of my prev message), then when I load a new URL in the frame, after the array buffer is created and I call my javascript function to paint the array buffer into a canvas, I get this check fail in GlobalBackingStoreRegistry::Register:

registry.jpg
registry.jpg (365.92 KiB) Viewed 4706 times


I guess this may make sense because when they array buffer is released and a new one of the same size is immediately allocated, it receives the same chunk of memory, which if the BackingStore hasn't been cleaned up yet, will appear to be re-registering the already registered buffer.

I'll try out the new patches you shared, and also see if I can get the hacked up cefsimple to show this behavior too to make it easier to repro / test. Thanks!
sdiverdi
Mentor
 
Posts: 51
Joined: Fri Dec 25, 2020 7:41 pm

Re: Native/javascript shared ArrayBuffer crash

Postby sdiverdi » Tue Mar 23, 2021 1:37 am

Woof, man, too many things I'm not sure what's going on here.

Your first patch, that gives ownership exclusively to the backing store, works! I can understand how it doesn't pass the tests, because yeah I see the release buffer callback called sometime later, but it doesn't seem to conflict with the new context doing its thing with a new array buffer, so that's great. Maybe instead there's a way to update the test to reflect the variability of when the backing store gets cleaned up?

However, I don't really understand _why_ it works. The check fail I just noted, inside GlobalBackingStoreRegistry::Register, is the problem I originally wrote about at the beginning of this thread, rearing its head again:

Code: Select all
DOMArrayBuffer* V8ArrayBuffer::ToImpl(v8::Local<v8::Object> object) {
  DCHECK(object->IsArrayBuffer());
  v8::Local<v8::ArrayBuffer> v8buffer = object.As<v8::ArrayBuffer>();
  // TODO(ahaas): The use of IsExternal is wrong here. Instead we should call
  // ToScriptWrappable(object)->ToImpl<ArrayBuffer>() and check for nullptr.
  // We can then also avoid the call to Externalize below.
  if (v8buffer->IsExternal()) {
    const WrapperTypeInfo* wrapper_type = ToWrapperTypeInfo(object);
    CHECK(wrapper_type);
    CHECK_EQ(wrapper_type->gin_embedder, gin::kEmbedderBlink);
    return ToScriptWrappable(object)->ToImpl<DOMArrayBuffer>();
  }

  // Transfer the ownership of the allocated memory to an ArrayBuffer without
  // copying.
  auto backing_store = v8buffer->GetBackingStore();
  v8buffer->Externalize(backing_store);
  ArrayBufferContents contents(std::move(backing_store));
  DOMArrayBuffer* buffer = DOMArrayBuffer::Create(contents);
  v8::Local<v8::Object> associatedWrapper = buffer->AssociateWithWrapper(v8::Isolate::GetCurrent(), buffer->GetWrapperTypeInfo(), object);
  DCHECK(associatedWrapper == object);

  return buffer;
}


Before, this code failed in the first block after IsExternal==true. Now, I think the first time through IsExternal==false (because even though we provide a BackingStore with a custom deleter, that function doesn't seem to set the external bit anywhere. So the IsExternal==false path of the above function calls GetBackingStore and then Externalize, making subsequent calls to this function (it's called every frame) get IsExternal==true (and since blink called Externalize, the embedder check passes??).

The weird thing is GetBackingStore:

Code: Select all
std::shared_ptr<v8::BackingStore> v8::ArrayBuffer::GetBackingStore() {
  i::Handle<i::JSArrayBuffer> self = Utils::OpenHandle(this);
  std::shared_ptr<i::BackingStore> backing_store = self->GetBackingStore();
  if (!backing_store) {
    backing_store =
        i::BackingStore::EmptyBackingStore(i::SharedFlag::kNotShared);
  }
  i::GlobalBackingStoreRegistry::Register(backing_store);
  std::shared_ptr<i::BackingStoreBase> bs_base = backing_store;
  return std::static_pointer_cast<v8::BackingStore>(bs_base);
}


What I don't understand here is why _this_ function calls GlobalBackingStoreRegistry::Register, instead of e.g. NewBackingStore. From what I understand of the BackingStore registry which is meant to assure a 1:1 mapping between BackingStores and buffer pointers, not registering it at initialization seems strange (maybe BackingStores that aren't accessed aren't meant to get registered??). Anyway so the first time this is called, a pointer is registered.

The _next_ thing I don't understand is in my patch that nulled out the custom deleter, my second call to Register would check fail because the pointer was already in the registry. That made sense because I would allocate an array buffer, then release it (via ~V8TrackArrayBuffer) and immediately create a new one of the same size, thus being allocated at the same buffer pointer, and the BackingStore wasn't cleaned up yet, so Register would see what appeared to be a second BackingStore with the same buffer pointer. I think I get that.

What I don't get is how _this_ patch, that ditches the ~V8TrackArrayBuffer and just has the BackingStore own it, gets around that problem, since it seems that Register is still being called in the new context and that the BackingStore isn't cleaned up yet.

Ohhhh, wait, maybe that makes sense, since ~V8TrackArrayBuffer hasn't freed the buffer, then the new array buffer must have a different pointer. Actually yeah I think that does work! And it's good this way because it means a buffer pointer won't be re-used until it's been freed by the BackingStore, which avoids this double-registry problem. So that's great!

So...think the test can be updated to work with this patch? Maybe wait until the GC sweep cleans up the BackingStore?
sdiverdi
Mentor
 
Posts: 51
Joined: Fri Dec 25, 2020 7:41 pm

Re: Native/javascript shared ArrayBuffer crash

Postby sdiverdi » Tue Mar 23, 2021 2:18 am

Looks like V8 provides two ways to wait for GC on an isolate, one for tests:

Code: Select all
  /**
   * Request garbage collection in this Isolate. It is only valid to call this
   * function if --expose_gc was specified.
   *
   * This should only be used for testing purposes and not to enforce a garbage
   * collection schedule. It has strong negative impact on the garbage
   * collection performance. Use IdleNotificationDeadline() or
   * LowMemoryNotification() instead to influence the garbage collection
   * schedule.
   */
  void RequestGarbageCollectionForTesting(GarbageCollectionType type);


though it's not synchronous, so not sure how you'd know when it's done. Or:

Code: Select all
  /**
   * Optional notification that the embedder is idle.
   * V8 uses the notification to perform garbage collection.
   * This call can be used repeatedly if the embedder remains idle.
   * Returns true if the embedder should stop calling IdleNotificationDeadline
   * until real work has been done.  This indicates that V8 has done
   * as much cleanup as it will be able to do.
   *
   * The deadline_in_seconds argument specifies the deadline V8 has to finish
   * garbage collection work. deadline_in_seconds is compared with
   * MonotonicallyIncreasingTime() and should be based on the same timebase as
   * that function. There is no guarantee that the actual work will be done
   * within the time limit.
   */
  bool IdleNotificationDeadline(double deadline_in_seconds);


which it seems you can put in a loop like:

Code: Select all
while (!isolate->IdleNotificationDeadline(1)) {}


Not sure how you might expose this via CEF though.
sdiverdi
Mentor
 
Posts: 51
Joined: Fri Dec 25, 2020 7:41 pm

Re: Native/javascript shared ArrayBuffer crash

Postby magreenblatt » Tue Mar 23, 2021 9:12 am

Did you get a chance to try the 2nd patch that gives buffer ownership strictly to V8TrackArrayBuffer?
magreenblatt
Site Admin
 
Posts: 12382
Joined: Fri May 29, 2009 6:57 pm

Re: Native/javascript shared ArrayBuffer crash

Postby sdiverdi » Tue Mar 23, 2021 1:08 pm

Was just able to try it now. It fails the check in GlobalBackingStoreRegistry::Register same as I found earlier when stubbing out the BackingStore custom deleter. This makes sense with the analysis I did last night -- V8TrackArrayBuffer immediately releases the buffer and a new buffer is allocated at the same memory location, which is then registered in the backing store registry and found to collide with the buffer pointer of the old BackingStore that hasn't been deleted yet.
sdiverdi
Mentor
 
Posts: 51
Joined: Fri Dec 25, 2020 7:41 pm

Re: Native/javascript shared ArrayBuffer crash

Postby magreenblatt » Tue Mar 23, 2021 2:05 pm

OK, so having the deleter function own the buffer seems the way to go. Thanks for doing the testing.
magreenblatt
Site Admin
 
Posts: 12382
Joined: Fri May 29, 2009 6:57 pm

Previous

Return to Support Forum

Who is online

Users browsing this forum: Google [Bot] and 36 guests