fix: transparently package bundles as zip archives by codebytere · Pull Request #25030 · electron/electron

codebytere · 2020-08-19T03:51:37Z

Description of Change

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.

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

cc @nornagon @zcbenz @MarshallOfSound

Checklist

PR description included and stakeholders cc'd
npm test passes
PR title follows semantic commit guidelines
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.
This is NOT A BREAKING CHANGE. Breaking changes may not be merged to master until 11-x-y is branched.

Release Notes

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

shell/browser/file_select_helper_mac.mm

shell/browser/file_select_helper.cc

nornagon

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 · 2020-08-19T22:39:08Z

@nornagon see #25030 (comment)

codebytere · 2020-08-20T19:29:18Z

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.

deepak1556

LGTM

deepak1556 · 2020-08-20T21:59:56Z

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

nornagon

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

shell/browser/web_dialog_helper.cc

deepak1556

LGTM

codebytere · 2020-10-28T00:05:09Z

PDF failure tests are unrelated.

release-clerk · 2020-10-28T00:05:32Z

Release Notes Persisted

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

trop · 2020-10-28T00:05:47Z

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

trop · 2020-10-28T00:06:03Z

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

trop · 2020-11-18T04:20:00Z

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

codebytere requested review from MarshallOfSound, nornagon and zcbenz August 19, 2020 03:51

electron-cation bot added the new-pr 🌱 PR opened recently label Aug 19, 2020

codebytere force-pushed the sometimes-we-do-want-pkgs branch from 9319ca1 to 49421ca Compare August 19, 2020 03:53

deepak1556 reviewed Aug 19, 2020

View reviewed changes

codebytere force-pushed the sometimes-we-do-want-pkgs branch 2 times, most recently from b010e73 to 5122b49 Compare August 19, 2020 20:48

nornagon reviewed Aug 19, 2020

View reviewed changes

electron-cation bot removed the new-pr 🌱 PR opened recently label Aug 20, 2020

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

codebytere requested review from deepak1556 and nornagon August 20, 2020 19:55

deepak1556 approved these changes Aug 20, 2020

View reviewed changes

nornagon suggested changes Aug 24, 2020

View reviewed changes

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 and others added 11 commits October 22, 2020 08:38

refactor: move FileSelectHelper to own file

30047c1

fix: zip packages on macOS

b7421f9

Thread safety, it's good actually

6040135

misc cleanup

0af70f3

Remove some unused includes

e682a8a

Better ref management

11edddd

Use upstream file_select_helper_mac

2af4f2a

Small fixes

f10c7c4

Put back file_select_helper_mac

09ddaed

observe RenderWidgetHost

c62c8b4

fix ref-counting and add some guards

438b9fe

codebytere force-pushed the sometimes-we-do-want-pkgs branch 2 times, most recently from ba27b8e to be54c0f Compare October 27, 2020 17:55

deepak1556 reviewed Oct 27, 2020

View reviewed changes

shell/browser/web_dialog_helper.cc Show resolved Hide resolved

codebytere force-pushed the sometimes-we-do-want-pkgs branch from be54c0f to e7ca638 Compare October 27, 2020 18:51

Remove unnecessary GetWeakPtr

467c771

codebytere force-pushed the sometimes-we-do-want-pkgs branch from e7ca638 to 467c771 Compare October 27, 2020 19:00

deepak1556 approved these changes Oct 27, 2020

View reviewed changes

codebytere added target/10-x-y labels Oct 27, 2020

codebytere merged commit 284c1b9 into master Oct 28, 2020

codebytere deleted the sometimes-we-do-want-pkgs branch October 28, 2020 00:05

trop bot added needs-manual-bp/10-x-y and removed target/10-x-y labels Oct 28, 2020

trop bot mentioned this pull request Oct 28, 2020

fix: transparently package bundles as zip archives #26217

Merged

trop bot added in-flight/11-x-y and removed target/11-x-y labels Oct 28, 2020

codebytere mentioned this pull request Nov 18, 2020

fix: transparently package bundles as zip archives #26555

Merged

trop bot added in-flight/10-x-y and removed needs-manual-bp/10-x-y labels Nov 18, 2020

codebytere mentioned this pull request Aug 23, 2021

refactor: reuse upstream //shell_dialogs #30663

Merged

3 tasks

jtbandes mentioned this pull request Aug 14, 2025

File picker allows selecting directories if their name includes a file extension #48081

Closed

3 tasks

Conversation

codebytere commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Checklist

Release Notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nornagon left a comment

Choose a reason for hiding this comment

Uh oh!

codebytere commented Aug 19, 2020

Uh oh!

codebytere commented Aug 20, 2020

Uh oh!

deepak1556 left a comment

Choose a reason for hiding this comment

Uh oh!

deepak1556 commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nornagon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deepak1556 left a comment

Choose a reason for hiding this comment

Uh oh!

codebytere commented Oct 28, 2020

Uh oh!

release-clerk bot commented Oct 28, 2020

Uh oh!

trop bot commented Oct 28, 2020

Uh oh!

trop bot commented Oct 28, 2020

Uh oh!

trop bot commented Nov 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codebytere commented Aug 19, 2020 •

edited

Loading

deepak1556 commented Aug 20, 2020 •

edited

Loading