Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standalone embedder seems to leak libraries loaded via IsolateMirror.loadUri #37030

Closed
mkustermann opened this issue May 21, 2019 · 0 comments
Closed
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.

Comments

@mkustermann
Copy link
Member

The implementation of IsolateMirror.loadUri calls out to the embedder to load the library, see runtime/lib/mirrors.cc:

DEFINE_NATIVE_ENTRY(IsolateMirror_loadUri, 0, 1) {
  ...

  // Request the embedder to load the library.
  isolate->BlockClassFinalization();
  Object& result = Object::Handle(
      zone, isolate->CallTagHandler(
                Dart_kImportTag,
                Library::Handle(zone, isolate->object_store()->root_library()),
                canonical_uri));
  isolate->UnblockClassFinalization();
  ...
  if (!ClassFinalizer::ProcessPendingClasses()) {
    Exceptions::PropagateError(Error::Handle(thread->sticky_error()));
  }

Looking at the code in runtime/bin/loader.cc:

    if (dfe.CanUseDartFrontend() && dfe.UseDartFrontend() &&
        (tag == Dart_kImportTag)) {
      // E.g., IsolateMirror.loadUri.
      char* error = NULL;
      int exit_code = 0;
      uint8_t* kernel_buffer = NULL;
      intptr_t kernel_buffer_size = -1;
      dfe.CompileAndReadScript(url_string, &kernel_buffer, &kernel_buffer_size,
                               &error, &exit_code, NULL);
      if (exit_code == 0) {
        return Dart_LoadLibraryFromKernel(kernel_buffer, kernel_buffer_size);
        ...

It seems this kernel buffer is never freed and thereby leaked (process-wide, even after the isolate is shutdown).

/cc @rmacnak-google

@mkustermann mkustermann added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label May 21, 2019
dart-bot pushed a commit that referenced this issue May 22, 2019
… buffers

Until now we often leaked kernel buffers (e.g. hot reload buffers) because various
objects were referencing ExternalTypedData objects pointing into the middle of
c-allocated memory. This made it impossible for the GC to determine when the last
reference is gone.

This CL ensures that the actual buffers are *always* made available via
ExternalTypedData and any inner pointers into it are created via TypedDataViews.

The embedder guarantees to the free kernel buffers it has provided to:
    - Dart_CreateIsolateFromKernel
    - Dart_LoadScriptFromKernel
    - Dart_LoadLibraryFromKernel
    - Dart_SetDartLibrarySourcesKernel
on isolate shutdown.

All other kernel buffers will get a finalizer attached, which ensures the
kernel buffers get freed by the GC once they are no longer referenced:
    - Kernel blobs for expression evaluation
    - Kernel blobs for Hot-Reload
    - Kernel blobs for cc tests

Fixes #33973
Fixes #36857
Issue #37030

Change-Id: I1cc410c94c0f4b229413e793728a261afcb10aaf
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/103130
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue May 31, 2019
…r kernel buffers"

This reverts commit ab6aeaa.

Revert "[vm/compiler] Speed up the compiler part which deals with kernel reading up in DEBUG mode"

This reverts commit b316210.

Reason for revert: regression of snapshot sizes (DNO-599).

Original change's description:
> [vm/kernel] Use GC-tracked ExternalTypedData/TypedDataView for kernel buffers
>
> Until now we often leaked kernel buffers (e.g. hot reload buffers) because various
> objects were referencing ExternalTypedData objects pointing into the middle of
> c-allocated memory. This made it impossible for the GC to determine when the last
> reference is gone.
>
> This CL ensures that the actual buffers are *always* made available via
> ExternalTypedData and any inner pointers into it are created via TypedDataViews.
>
> The embedder guarantees to the free kernel buffers it has provided to:
>     - Dart_CreateIsolateFromKernel
>     - Dart_LoadScriptFromKernel
>     - Dart_LoadLibraryFromKernel
>     - Dart_SetDartLibrarySourcesKernel
> on isolate shutdown.
>
> All other kernel buffers will get a finalizer attached, which ensures the
> kernel buffers get freed by the GC once they are no longer referenced:
>     - Kernel blobs for expression evaluation
>     - Kernel blobs for Hot-Reload
>     - Kernel blobs for cc tests
>
> Fixes #33973
> Fixes #36857
> Issue #37030
>
> Change-Id: I1cc410c94c0f4b229413e793728a261afcb10aaf
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/103130
> Reviewed-by: Ryan Macnak <rmacnak@google.com>
> Commit-Queue: Martin Kustermann <kustermann@google.com>

TBR=kustermann@google.com,rmacnak@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Change-Id: I49715d2400f4a5c8806b7d6a2912b7258f671a0a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/104343
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>
Auto-Submit: Alexander Markov <alexmarkov@google.com>
@rmacnak-google rmacnak-google self-assigned this Feb 2, 2023
copybara-service bot pushed a commit that referenced this issue Feb 2, 2023
TEST=asan
Bug: #37030
Bug: #51210
Change-Id: Ia62a4d7d1805c6ac4a311ef9952fff248e7b4693
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/280284
Reviewed-by: Siva Annamalai <asiva@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.
Projects
None yet
Development

No branches or pull requests

3 participants