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

Bad reference counting and DCHECK in IPFS Extension API #15996

Closed
iefremov opened this issue May 20, 2021 · 2 comments · Fixed by brave/brave-core#8885
Closed

Bad reference counting and DCHECK in IPFS Extension API #15996

iefremov opened this issue May 20, 2021 · 2 comments · Fixed by brave/brave-core#8885

Comments

@iefremov
Copy link
Contributor

Extension functions in ipfs_api.h use Unretained pointers and manual AddRef() Release() that are not actually needed, because functions are refcounted on their own, and can be bound to callbacks as is.

Moreover, there is a wrong balance of AddRef() and Release() somewhere that leads to DCHECK / memleak

STR:

  1. open brave://settings/ipfs/keys
  2. Start node
  3. manually add key named "123"
  4. open dev tools and type chrome.ipfs.addIpnsKey("123", () => console.log("aaa")) in console
[1735628:1735628:0520/205438.728179:FATAL:ref_counted.h(194)] Check failed: !in_dtor_. 
#0 0x7fc2f226b659 base::debug::CollectStackTrace()
#1 0x7fc2f216a3f3 base::debug::StackTrace::StackTrace()
#2 0x7fc2f218a963 logging::LogMessage::~LogMessage()
#3 0x7fc2f218b30e logging::LogMessage::~LogMessage()
#4 0x55e73ef07ba3 base::subtle::RefCountedThreadSafeBase::AddRef()
#5 0x55e73fc7f858 extensions::api::IpfsAddIpnsKeyFunction::Run()
#6 0x55e73f28a0c7 ExtensionFunction::RunWithValidation()
#7 0x55e73f28d9dc extensions::ExtensionFunctionDispatcher::DispatchWithCallbackInternal()
#8 0x55e73f28d354 extensions::ExtensionFunctionDispatcher::Dispatch()
#9 0x55e73f2b86d9 extensions::ExtensionWebContentsObserver::OnRequest()
#10 0x55e73f2b85e7 IPC::MessageT<>::Dispatch<>()
#11 0x55e73f2b84b7 extensions::ExtensionWebContentsObserver::OnMessageReceived()
#12 0x55e740dac70e extensions::ChromeExtensionWebContentsObserver::OnMessageReceived()
@iefremov
Copy link
Contributor Author

@spylogsster pls take a look

@spylogsster spylogsster self-assigned this May 20, 2021
@spylogsster spylogsster added this to the 1.27.x - Nightly milestone May 21, 2021
@stephendonner stephendonner added the QA/In-Progress Indicates that QA is currently in progress for that particular issue label May 27, 2021
@stephendonner
Copy link

Verified PASSED using the inline testplan with build

Brave 1.27.25 Chromium: 91.0.4472.77 (Official Build) nightly (x86_64)
Revision 1cecd5c8a856bc2a5adda436e7b84d8d21b339b6-refs/branch-heads/4472@{#1246}
OS macOS Version 11.4 (Build 20F71)

Steps:

  1. opened brave://settings/ipfs/keys
  2. started my IPFS node via brave://ipfs-internals
  3. manually added a new key named "123"
  4. opened dev tools and typed chrome.ipfs.addIpnsKey("123", () => console.log("aaa")) in console

Confirmed I didn't crash; reproduced the crash very easily with an earlier nightly build, before this fix.

I now get undefined returned, with aaa printed to the console.

example example
Screen Shot 2021-05-27 at 4 30 53 PM Screen Shot 2021-05-27 at 4 30 57 PM

@stephendonner stephendonner added QA Pass-macOS and removed QA/In-Progress Indicates that QA is currently in progress for that particular issue labels May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment