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

fix: transparently package bundles as zip archives #25030

Merged
merged 14 commits into from Oct 28, 2020

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Aug 19, 2020

Description of Change

Closes #24755.

Refactors and cleans up our input element handing for files and directories. Moves FileSelectHelper to its own fileset and hooks up functionality to transparently package bundles on macOS into zip archives.

Screen Shot 2020-08-18 at 8 50 13 PM

Tested with https://gist.github.com/6ed9ee05659956e33c15343e49e26ed3. Also tested that the multiple and webkitdirectory attributes still work as expected.

cc @nornagon @zcbenz @MarshallOfSound

Checklist

Release Notes

Notes: Fixed an issue where packages could not be selected with on macOS.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Aug 19, 2020
shell/browser/file_select_helper_mac.mm Show resolved Hide resolved
shell/browser/file_select_helper_mac.mm Show resolved Hide resolved
shell/browser/file_select_helper.cc Outdated Show resolved Hide resolved
shell/browser/file_select_helper.cc Show resolved Hide resolved
shell/browser/file_select_helper.cc Show resolved Hide resolved
shell/browser/file_select_helper.cc Show resolved Hide resolved
shell/browser/file_select_helper.cc Show resolved Hide resolved
shell/browser/file_select_helper.cc Show resolved Hide resolved
@codebytere codebytere force-pushed the sometimes-we-do-want-pkgs branch 2 times, most recently from b010e73 to 5122b49 Compare August 19, 2020 20:48
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

It looks like this is cribbed from //chrome/browser/file_select_helper_mac.mm. That component looks fairly isolated—would it be possible to reuse that class directly?

@codebytere
Copy link
Member Author

@nornagon see #25030 (comment)

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Aug 20, 2020
@codebytere
Copy link
Member Author

Note that i ended up putting back file_select_helper_mac owing to the fact that it includes chrome/browser/file_select_helper.h instead of shell/browser/file_select_helper.h and we would thus have had to patch. As we want to keep patch count down and this is a small surface area unlikely to change, I think it's probably a better choice to just copy it.

@codebytere codebytere force-pushed the sometimes-we-do-want-pkgs branch 2 times, most recently from 9c240e7 to f595f0a Compare August 20, 2020 19:53
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

LGTM

@deepak1556
Copy link
Member

deepak1556 commented Aug 20, 2020

while testing found the following DCHECK that can also be seen on chrome canary (can't check for dcheck crash), but content_shell is sufficient

  • Launch the test app
  • Open a .app file that is relative large, for my example tried Docker.app or you can select multiple *.app that would end up a large zip cache
  • It will take a while to populate the zip
  • Before above step finishes, close the app or navigate away
  • DCHECK!!!
FATAL:file_chooser_impl.cc(21)] Check failed: was_file_select_listener_function_called_. Must call either FileSelectListener::FileSelected() or FileSelectListener::FileSelectionCanceled()
0   Electron Framework                  0x000000011c1529f9 base::debug::CollectStackTrace(void**, unsigned long) + 9
1   Electron Framework                  0x000000011c04d073 base::debug::StackTrace::StackTrace() + 19
2   Electron Framework                  0x000000011c06bb15 logging::LogMessage::~LogMessage() + 261
3   Electron Framework                  0x000000011c06c81e logging::LogMessage::~LogMessage() + 14
4   Electron Framework                  0x000000011b186441 content::FileChooserImpl::FileSelectListenerImpl::~FileSelectListenerImpl() + 97
5   Electron Framework                  0x000000011b1864ce content::FileChooserImpl::FileSelectListenerImpl::~FileSelectListenerImpl() + 14
6   Electron Framework                  0x000000011b187197 content::FileChooserImpl::~FileChooserImpl() + 103
7   Electron Framework                  0x000000011b187f03 mojo::StrongBinding<blink::mojom::FileChooser>::Close() + 83
8   Electron Framework                  0x000000011b187c3c mojo::StrongBinding<blink::mojom::FileChooser>::OnConnectionError(unsigned int, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 76
9   Electron Framework                  0x000000011c5f3155 mojo::InterfaceEndpointClient::NotifyError(base::Optional<mojo::DisconnectReason> const&) + 357
10  Electron Framework                  0x000000011c5fc3df mojo::internal::MultiplexRouter::ProcessNotifyErrorTask(mojo::internal::MultiplexRouter::Task*, mojo::internal::MultiplexRouter::ClientCallBehavior, base::SequencedTaskRunner*) + 447
11  Electron Framework                  0x000000011c5f936e mojo::internal::MultiplexRouter::ProcessTasks(mojo::internal::MultiplexRouter::ClientCallBehavior, base::SequencedTaskRunner*) + 878
12  Electron Framework                  0x000000011c5f7942 mojo::internal::MultiplexRouter::OnPipeConnectionError(bool) + 914
13  Electron Framework                  0x000000011c5ea31e mojo::Connector::HandleError(bool, bool) + 494
14  Electron Framework                  0x000000011c5eb501 mojo::Connector::OnHandleReadyInternal(unsigned int) + 113
15  Electron Framework                  0x00000001168b9820 mojo::SimpleWatcher::DiscardReadyState(base::RepeatingCallback<void (unsigned int)> const&, unsigned int, mojo::HandleSignalsState const&) + 80
16  Electron Framework                  0x000000011c61d5da mojo::SimpleWatcher::OnHandleReady(int, unsigned int, mojo::HandleSignalsState const&) + 410
17  Electron Framework                  0x000000011c61dacb base::internal::Invoker<base::internal::BindState<void (mojo::SimpleWatcher::*)(int, unsigned int, mojo::HandleSignalsState const&), base::WeakPtr<mojo::SimpleWatcher>, int, unsigned int, mojo::HandleSignalsState>, void ()>::RunOnce(base::internal::BindStateBase*) + 171
18  Electron Framework                  0x000000011c0e2a3b base::TaskAnnotator::RunTask(char const*, base::PendingTask*) + 411
19  Electron Framework                  0x000000011c101a27 base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWorkImpl(base::sequence_manager::LazyNow*) + 519
20  Electron Framework                  0x000000011c101698 base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork() + 168
21  Electron Framework                  0x000000011c173191 base::MessagePumpCFRunLoopBase::RunWork() + 65
22  Electron Framework                  0x000000011c168222 base::mac::CallWithEHFrame(void () block_pointer) + 10
23  Electron Framework                  0x000000011c172b5f base::MessagePumpCFRunLoopBase::RunWorkSource(void*) + 63
24  CoreFoundation                      0x00007fff3251ed52 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
25  CoreFoundation                      0x00007fff3251ecf1 __CFRunLoopDoSource0 + 103
26  CoreFoundation                      0x00007fff3251eb0b __CFRunLoopDoSources0 + 209
27  CoreFoundation                      0x00007fff3251d83a __CFRunLoopRun + 927
28  CoreFoundation                      0x00007fff3251ce3e CFRunLoopRunSpecific + 462
29  HIToolbox                           0x00007fff31149abd RunCurrentEventLoopInMode + 292
30  HIToolbox                           0x00007fff311497d5 ReceiveNextEventCommon + 584
31  HIToolbox                           0x00007fff31149579 _BlockUntilNextEventMatchingListInModeWithFilter + 64
32  AppKit                              0x00007fff2f78f039 _DPSNextEvent + 883
33  AppKit                              0x00007fff2f78d880 -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1352
34  AppKit                              0x00007fff2f77f58e -[NSApplication run] + 658
35  Electron Framework                  0x000000011c17414c base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*) + 348
36  Electron Framework                  0x000000011c172662 base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*) + 130
37  Electron Framework                  0x000000011c10225d base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run(bool, base::TimeDelta) + 349
38  Electron Framework                  0x000000011c0b5bac base::RunLoop::Run() + 956
39  Electron Framework                  0x000000011aa247eb content::BrowserMainLoop::RunMainMessageLoopParts() + 203
40  Electron Framework                  0x000000011aa26bb2 content::BrowserMainRunnerImpl::Run() + 82
41  Electron Framework                  0x000000011aa2130b content::BrowserMain(content::MainFunctionParams const&) + 267
42  Electron Framework                  0x000000011a8371ce content::ContentMainRunnerImpl::RunServiceManager(content::MainFunctionParams&, bool) + 1406
43  Electron Framework                  0x000000011a836c23 content::ContentMainRunnerImpl::Run(bool) + 467
44  Electron Framework                  0x000000011e886a5e service_manager::Main(service_manager::MainParams const&) + 3310
45  Electron Framework                  0x000000011876df68 content::ContentMain(content::ContentMainParams const&) + 120
46  Electron Framework                  0x0000000116863386 ElectronMain + 134
47  Electron                            0x000000010a4f5551 main + 289
48  libdyld.dylib                       0x00007fff6c589cc9 start + 1
Task trace:
0   Electron Framework                  0x000000011c61d936 mojo::SimpleWatcher::Context::Notify(unsigned int, MojoHandleSignalsState, unsigned int) + 518

Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

if we can't reuse //chrome/browser/file_select_helper.cc, can we at least reuse //ui/shell_dialogs?

shell/browser/file_select_helper.cc Outdated Show resolved Hide resolved
shell/browser/file_select_helper.cc Outdated Show resolved Hide resolved
shell/browser/file_select_helper.cc Outdated Show resolved Hide resolved
shell/browser/file_select_helper.cc Show resolved Hide resolved
@codebytere codebytere force-pushed the sometimes-we-do-want-pkgs branch 2 times, most recently from ba27b8e to be54c0f Compare October 27, 2020 17:55
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

LGTM

@codebytere
Copy link
Member Author

PDF failure tests are unrelated.

@codebytere codebytere merged commit 284c1b9 into master Oct 28, 2020
@release-clerk
Copy link

release-clerk bot commented Oct 28, 2020

Release Notes Persisted

Fixed an issue where packages could not be selected with on macOS.

@codebytere codebytere deleted the sometimes-we-do-want-pkgs branch October 28, 2020 00:05
@trop
Copy link
Contributor

trop bot commented Oct 28, 2020

I was unable to backport this PR to "10-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Oct 28, 2020

I have automatically backported this PR to "11-x-y", please check out #26217

@trop
Copy link
Contributor

trop bot commented Nov 18, 2020

@codebytere has manually backported this PR to "10-x-y", please check out #26555

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.

Packages cannot be selected in <input file="type"> file selector on macOS
3 participants