Native/javascript shared ArrayBuffer crash

[ndesktop]

Can you post a reply with all the tests/scripts you are using for this crash?
I would like to work on a fix and then fill a CEF issue and a PR, and I would appreciate any help I can get.

[sdiverdi]

Happy new year! I actually got this stuck in my craw and so spent yesterday building chromium and hacking on it. Updating to using the new BackingStore API fixes the problem. Here's what I have now, in libcef/renderer/v8_impl.cc:

Code: Select all

CefRefPtr<CefV8Value> CefV8Value::CreateArrayBuffer(
    void* buffer,
    size_t length,
    CefRefPtr<CefV8ArrayBufferReleaseCallback> release_callback) {
  CEF_V8_REQUIRE_ISOLATE_RETURN(nullptr);

  v8::Isolate* isolate = GetIsolateManager()->isolate();
  v8::HandleScope handle_scope(isolate);
  v8::Local<v8::Context> context = isolate->GetCurrentContext();
  if (context.IsEmpty()) {
    NOTREACHED() << "not currently in a V8 context";
    return nullptr;
  }

  std::unique_ptr<v8::BackingStore> backing = v8::ArrayBuffer::NewBackingStore(buffer, length,
    [](void* data, size_t, void* cb){ ((CefV8ArrayBufferReleaseCallback*)cb)->ReleaseBuffer(data); }, release_callback.get());
  v8::Local<v8::ArrayBuffer> ab = v8::ArrayBuffer::New(isolate, std::move(backing));

  CefRefPtr<CefV8ValueImpl> impl = new CefV8ValueImpl(isolate);
  impl->InitObject(ab, nullptr);
  return impl.get();
}

However, this loses the smart pointer on release_callback so it probably needs to be stored in the Tracker (which I've removed here).

I've attached the files I'm using to test, for what they're worth. It's not a unit test, it's based on cefsimple and creates a browser window with a local html file, and clicking the button in the page is what triggers the ArrayBuffer and subsequent crash.

[ndesktop]

Thank you. From Monday I hope to start working on a fix to be merged in CEF.

[sdiverdi]

Awesome! Let me know if I can help.

[ndesktop]

That would be the v1 of the patch. Modifications are done against 87.0.4280.88, however I suppose it can be applied with minimum effort on master or other new branches.
Generated with git diff > ArrayBuffer_NewBackingStore_v1.diff (renamed in .txt, extension .diff is not allowed).

For convenience, I am attaching as both code and attachment.

Code: Select all

diff --git a/libcef/renderer/v8_impl.cc b/libcef/renderer/v8_impl.cc
index a0127b25..34019636 100644
--- a/libcef/renderer/v8_impl.cc
+++ b/libcef/renderer/v8_impl.cc
@@ -1410,7 +1410,20 @@ CefRefPtr<CefV8Value> CefV8Value::CreateArrayBuffer(
   // released when the V8 object is destroyed.
   V8TrackArrayBuffer* tracker =
       new V8TrackArrayBuffer(isolate, buffer, release_callback);
-  v8::Local<v8::ArrayBuffer> ab = v8::ArrayBuffer::New(isolate, buffer, length);
+
+  auto deleter = [](void* data, size_t length, void* deleter_data) {
+    CefV8ArrayBufferReleaseCallback* callback =
+      reinterpret_cast<CefV8ArrayBufferReleaseCallback*>(
+        deleter_data);
+    if(callback != nullptr)
+      callback->ReleaseBuffer(data);
+  };
+
+  std::unique_ptr<v8::BackingStore> backing =
+    v8::ArrayBuffer::NewBackingStore(buffer, length,
+      deleter, reinterpret_cast<void*>(release_callback.get()));
+  v8::Local<v8::ArrayBuffer> ab =
+    v8::ArrayBuffer::New(isolate, std::move(backing));
 
   // Attach the tracker object.
   tracker->AttachTo(context, ab);
diff --git a/tests/ceftests/v8_unittest.cc b/tests/ceftests/v8_unittest.cc
index b46c33b5..79ae890d 100644
--- a/tests/ceftests/v8_unittest.cc
+++ b/tests/ceftests/v8_unittest.cc
@@ -59,6 +59,7 @@ enum V8TestMode {
   V8TEST_ARRAY_VALUE,
   V8TEST_ARRAY_BUFFER,
   V8TEST_ARRAY_BUFFER_VALUE,
+  V8TEST_ARRAY_BUFFER_VALUE_NEW_BACKING_STORE,
   V8TEST_OBJECT_CREATE,
   V8TEST_OBJECT_USERDATA,
   V8TEST_OBJECT_ACCESSOR,
@@ -141,6 +142,9 @@ class V8RendererTest : public ClientAppRenderer::Delegate,
       case V8TEST_ARRAY_BUFFER_VALUE:
         RunArrayBufferValueTest();
         break;
+      case V8TEST_ARRAY_BUFFER_VALUE_NEW_BACKING_STORE:
+        RunArrayBufferValueNewBackingStoreTest();
+        break;
       case V8TEST_OBJECT_CREATE:
         RunObjectCreateTest();
         break;
@@ -634,6 +638,71 @@ class V8RendererTest : public ClientAppRenderer::Delegate,
     DestroyTest();
   }
 
+  void RunArrayBufferValueNewBackingStoreTest() {
+    class TestArrayBufferReleaseCallback
+        : public CefV8ArrayBufferReleaseCallback {
+     public:
+      TestArrayBufferReleaseCallback() {}
+
+      ~TestArrayBufferReleaseCallback() {}
+
+      void ReleaseBuffer(void* buffer) override {
+        delete[] (unsigned char*)(buffer);
+     }
+
+      IMPLEMENT_REFCOUNTING(TestArrayBufferReleaseCallback);
+    };
+
+    CefRefPtr<CefV8Context> context = GetContext();
+
+    // Enter the V8 context.
+    CefRefPtr<CefV8Value> value;
+
+    CefRefPtr<TestArrayBufferReleaseCallback> owner =
+        new TestArrayBufferReleaseCallback();
+    EXPECT_TRUE(context->Enter());
+    int bufsize = 601;
+    unsigned char* buffer = new unsigned char[bufsize];
+    for(int i = 0; i < bufsize; ++i) {
+      buffer[i] = 'Y';
+    }
+    value =
+        CefV8Value::CreateArrayBuffer(buffer, bufsize, owner);
+   EXPECT_TRUE(value->IsArrayBuffer());
+
+    CefRefPtr<CefV8Value> object = context->GetGlobal();
+    EXPECT_TRUE(object.get());
+    object->SetValue("arrbuf", value, V8_PROPERTY_ATTRIBUTE_NONE);
+
+    std::string test = "\
+      (function() {\
+        var arrbuf = window.arrbuf;\
+        for(i = 0; i < arrbuf.byteLength; i++)\
+          arrbuf[i] = 0;\
+        for(i = 0; i < 4; i++)\
+          arrbuf[70 + i * 11] = 11 * (i + 1);\
+        var sum = 0;\
+        for(i = 0; i < arrbuf.byteLength; i++)\
+          sum += arrbuf[i];\
+        return sum;\
+   })();";
+    CefRefPtr<CefV8Value> retval;
+    CefRefPtr<CefV8Exception> exception;
+    EXPECT_TRUE(context->Eval(test, CefString(), 0, retval, exception));
+    if (exception.get())
+      ADD_FAILURE() << exception->GetMessage().c_str();
+
+    EXPECT_TRUE(retval->IsInt());
+    EXPECT_TRUE(retval->GetIntValue() == 11 * 10);
+
+    EXPECT_TRUE(value->GetArrayBufferReleaseCallback().get() != nullptr);
+    EXPECT_TRUE(value->NeuterArrayBuffer());
+
+    // Exit the V8 context.
+    EXPECT_TRUE(context->Exit());
+    DestroyTest();
+  }
+
   void RunObjectCreateTest() {
     CefRefPtr<CefV8Context> context = GetContext();
 
@@ -3040,6 +3109,7 @@ V8_TEST(ArrayCreate, V8TEST_ARRAY_CREATE)
 V8_TEST(ArrayValue, V8TEST_ARRAY_VALUE)
 V8_TEST(ArrayBuffer, V8TEST_ARRAY_BUFFER)
 V8_TEST(ArrayBufferValue, V8TEST_ARRAY_BUFFER_VALUE)
+V8_TEST(ArrayBufferValueNewBackingStore, V8TEST_ARRAY_BUFFER_VALUE_NEW_BACKING_STORE)
 V8_TEST(ObjectCreate, V8TEST_OBJECT_CREATE)
 V8_TEST(ObjectUserData, V8TEST_OBJECT_USERDATA)
 V8_TEST(ObjectAccessor, V8TEST_OBJECT_ACCESSOR)


[ndesktop]

Filed also the CEF issue.

[ndesktop]

Fixed in PR 361.
Sorry it took longer than I expected.

[sdiverdi]

Awesome thank you! I've rolled my own framework from sources with your patch and have been happily using it for a few weeks now.

[ndesktop]

Well, the merge has not yet been done, Marshall is right when he commented that I am not deleting the V8TrackArrayBuffer but the buffer directly, which will leak the track array object.
I will update the PR soon to delete the V8TrackArrayBuffer - which will invoke on deletion callback->ReleaseBuffer(buffer) - instead of directly deleting the contained buffer.

I don't know if I can make it today, but until the end of the week I will submit the updated PR, run the test, and if all is green and Marshall approves it it will make it into the sources soon.

[sdiverdi]

Actually I'm running into some trouble now, when I load new URLs in my browser. It looks like the release callback is stored both in the BackingStore and in the V8TrackArrayBuffer (see attached image), and may be called in both classes' destructors. I'm unclear on precisely when each happens but it seems this could cause double deletion. Also, V8TrackArrayBuffer's Detach method doesn't clear the release callback ref ptr, which means it could keep that object around longer than anticipated (I'm using my renderer as the release callback object but will change that now to avoid this problem). I mention Detach because I'm trying to explicitly clean up my memory in Renderer's OnContextReleased with NeuterArrayBuffer, so that the subsequent OnContextCreated call can create a new ArrayBuffer without trouble. I'm having trouble nailing down exactly what's failing now, but it is related to loading new URLs and the ArrayBuffer creation and deletion.