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

Added IPNS keys manager UI to IPFS Settings #8777

Merged
merged 2 commits into from
May 18, 2021
Merged

Conversation

spylogsster
Copy link
Contributor

@spylogsster spylogsster commented May 11, 2021

Resolves brave/brave-browser#15566

  • Added "Import" button to dialog that allows to import a key with predefined name
  • Renamed button from Submit to Generate to be consistent with another button
  • Implemented some refactoring because I reuse file uploads logic to ipfs node (Moved all network blobs constructions to ipfs_network_utils, there are some related changes in import classes. Finally refactoring will be finished in next PR in Remove redundant classes for import brave-browser#15758)
  • Added alertion about error if import was not successful

image

image

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  • Check if IPFS import works as expected and files/directories/links are imported well
  • Export keys from node on any computer via ipfs key export -o ./mykey.key mykey command and import it via brave://settings/ipfs/keys
  • Check if you import not a key file(wrong format or broken) there is alertion about error

@spylogsster spylogsster force-pushed the import-refactoring branch 5 times, most recently from 9c181d9 to 0f91343 Compare May 14, 2021 13:04
@spylogsster spylogsster force-pushed the import-key-from-files branch 2 times, most recently from 2d168b6 to 0c167d2 Compare May 14, 2021 13:22
Base automatically changed from import-refactoring to master May 14, 2021 20:03
Removed File/Text/Directory import classes.
Use base class instead, moved blob creation to network utilities
@spylogsster spylogsster force-pushed the import-key-from-files branch 3 times, most recently from ba7d74e to 1b326ee Compare May 15, 2021 05:15
<message name="IDS_SETTINGS_IPNS_KEYS_IMPORT_BUTTON_TITLE" desc="The title of the button to import ipns keys">
Import
</message>
<message name="IDS_SETTINGS_IPNS_KEYS_GENERATE_BUTTON_TITLE" desc="The title of the button to import ipns keys">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/import/generate in the desc

Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chromium_src ++

@bbondy bbondy merged commit d79bf03 into master May 18, 2021
@bbondy bbondy deleted the import-key-from-files branch May 18, 2021 16:21
@bbondy bbondy added this to the 1.26.x - Nightly milestone May 18, 2021
service->GetIpnsKeysManager()->ImportKey(
path, dialog_key_,
base::BindOnce(&BraveDefaultExtensionsHandler::OnKeyImported,
base::Unretained(this)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbondy @spylogsster I was just randomly browsing for Unretained pointers - this is an easily reproducible crash. Just start an import of a file and quickly close the settings tab (to 100% reproduce pick up any large file, I've tried with videos).

Basically the first thing I do during reviews is Ctrl-F Unretained - hope this could help to prevent future crashes

Received signal 11 SEGV_MAPERR ffffd859ca3ff9df
#0 0x7f4936df7659 base::debug::CollectStackTrace()
#1 0x7f4936cf63f3 base::debug::StackTrace::StackTrace()
#2 0x7f4936df71b0 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#3 0x7f49260363c0 (/usr/lib/x86_64-linux-gnu/libpthread-2.31.so+0x153bf)
#4 0x7f4934c79237 content::WebUIMessageHandler::IsJavascriptAllowed()
#5 0x562157e1730f content::WebUIMessageHandler::FireWebUIListener<>()
#6 0x562157e171ed BraveDefaultExtensionsHandler::OnKeyImported()
#7 0x5621577ab147 ipfs::IpnsKeysManager::OnKeyImported()
#8 0x5621577abef9 base::internal::FunctorTraits<>::Invoke<>()
#9 0x5621577abe27 base::internal::Invoker<>::RunOnce()
#10 0x7f49307157ed _ZNO4base12OnceCallbackIFvNSt4__Cr10unique_ptrINS1_12basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEENS1_14default_deleteIS8_EEEEEE3RunESB_
#11 0x7f4930715f4e network::(anonymous namespace)::SimpleURLLoaderImpl::FinishWithResult()
#12 0x7f4930713fea network::(anonymous namespace)::SimpleURLLoaderImpl::OnReceiveResponse()
#13 0x7f493075050a network::mojom::URLLoaderClientStubDispatch::Accept()
#14 0x7f4936694dab mojo::InterfaceEndpointClient::HandleValidatedMessage()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


auto iter = url_loaders_.insert(url_loaders_.begin(), std::move(url_loader));
iter->get()->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
url_loader_factory_.get(),
base::BindOnce(&IpnsKeysManager::OnKeyRemoved, base::Unretained(this),
base::BindOnce(&IpnsKeysManager::OnKeyRemoved, weak_factory_.GetWeakPtr(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here weak_factory_ is redundant, because the manager owns url loaders, and it will destroy loaders in its destructor. Loaders cancel their callbacks when destroyed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow users to export/import p2p keys
4 participants