FileSelectHelper should allow specifying a default filename for Open |
||||||||
Issue descriptionhttps://cs.chromium.org/chromium/src/chrome/browser/file_select_helper.cc?dr=C&q=file_select_helper.cc&sq=package:chromium&l=453 It happens when the open file button is clicked and a file is selected, and then click the open file button again. DCHECK failed at: [15003:15003:1012/140109:FATAL:file_select_helper.cc(454)] Check failed: params->default_file_name.empty() || params->mode == FileChooserParams::Save. The default_file_name parameter should only be specified for Save file choosers #0 0x7fa423f6aaae base::debug::StackTrace::StackTrace() #1 0x7fa423fd855f logging::LogMessage::~LogMessage() #2 0x7fa429244e61 FileSelectHelper::RunFileChooser() #3 0x7fa429244b9c FileSelectHelper::RunFileChooser() #4 0x7fa42828f3f1 Browser::RunFileChooser() #5 0x7fa41e840b50 content::WebContentsImpl::RunFileChooser() #6 0x7fa41df83981 content::RenderFrameHostImpl::OnRunFileChooser() #7 0x7fa41dfa1989 _ZN4base20DispatchToMethodImplIPN7content19RenderFrameHostImplEMS2_FvRKNS1_17FileChooserParamsEERKSt5tupleIJS4_EEJLm0EEEEvRKT_T0_OT1_NS_13IndexSequenceIJXspT2_EEEE #8 0x7fa41dfa18e8 _ZN4base16DispatchToMethodIPN7content19RenderFrameHostImplEMS2_FvRKNS1_17FileChooserParamsEERKSt5tupleIJS4_EEEEvRKT_T0_OT1_ #9 0x7fa41dfa186f _ZN3IPC16DispatchToMethodIN7content19RenderFrameHostImplEMS2_FvRKNS1_17FileChooserParamsEEvSt5tupleIJS3_EEEEvPT_T0_PT1_RKT2_ #10 0x7fa41df929bb _ZN3IPC8MessageTI32FrameHostMsg_RunFileChooser_MetaSt5tupleIJN7content17FileChooserParamsEEEvE8DispatchINS3_19RenderFrameHostImplES8_vMS8_FvRKS4_EEEbPKNS_7MessageEPT_PT0_PT1_T2_ #11 0x7fa41df7f75d content::RenderFrameHostImpl::OnMessageReceived() #12 0x7fa41e4c7001 content::RenderProcessHostImpl::OnMessageReceived() #13 0x7fa42132ed68 IPC::ChannelProxy::Context::OnDispatchMessage() #14 0x7fa421335607 _ZN4base8internal13FunctorTraitsIMN3IPC12ChannelProxy7ContextEFvRKNS2_7MessageEEvE6InvokeIRK13scoped_refptrIS4_EJS7_EEEvS9_OT_DpOT0_ #15 0x7fa4213354f6 _ZN4base8internal12InvokeHelperILb0EvE8MakeItSoIRKMN3IPC12ChannelProxy7ContextEFvRKNS4_7MessageEEJRK13scoped_refptrIS6_ES9_EEEvOT_DpOT0_ #16 0x7fa421335483 _ZN4base8internal7InvokerINS0_9BindStateIMN3IPC12ChannelProxy7ContextEFvRKNS3_7MessageEEJ13scoped_refptrIS5_ES6_EEEFvvEE7RunImplIRKSA_RKSt5tupleIJSC_S6_EEJLm0ELm1EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE #17 0x7fa42133539c _ZN4base8internal7InvokerINS0_9BindStateIMN3IPC12ChannelProxy7ContextEFvRKNS3_7MessageEEJ13scoped_refptrIS5_ES6_EEEFvvEE3RunEPNS0_13BindStateBaseE #18 0x7fa423f3b0ab base::internal::RunMixin<>::Run() #19 0x7fa423f70391 base::debug::TaskAnnotator::RunTask() #20 0x7fa4240006a4 base::MessageLoop::RunTask() #21 0x7fa424000934 base::MessageLoop::DeferOrRunPendingTask() #22 0x7fa424000c1e base::MessageLoop::DoWork() #23 0x7fa4240190b6 base::MessagePumpGlib::Run() #24 0x7fa424000216 base::MessageLoop::RunHandler() #25 0x7fa4240a59f4 base::RunLoop::Run() #26 0x7fa42648300f ChromeBrowserMainParts::MainMessageLoopRun() #27 0x7fa41dc55769 content::BrowserMainLoop::RunMainMessageLoopParts() #28 0x7fa41dc607e5 content::BrowserMainRunnerImpl::Run() #29 0x7fa41dc4f458 content::BrowserMain() #30 0x7fa41f3bbb26 content::RunNamedProcessTypeMain() #31 0x7fa41f3bdfb2 content::ContentMainRunnerImpl::Run() #32 0x7fa41f3babc2 content::ContentMain() #33 0x7fa424dc80ab ChromeMain #34 0x7fa424dc8042 main #35 0x7fa411dc3f45 __libc_start_main #36 0x7fa424dc7f45 <unknown>
,
Oct 12 2016
,
Oct 12 2016
It seems that the renderer sets default_file_name to the first element of selectedFiles list after the event. When that event happens again browser will hit this DCHECK. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/ChromeClientImpl.cpp?cl=GROK&l=715 Maybe we should remove the DCHECK or change the behavior of the renderer?
,
Oct 13 2016
Right. Removing the DCHECK by itself won't address the issue since the DCHECK is asserting assumptions made by the rest of the code. The default_file_name is considered untrusted. Hence it is sanitized and checked against safe browsing. We can add a separate code path that sanitizes, but skips the safe browsing check if the default_file_name is being used to preselect a file for uploading. The SB check is only necessary if writing a new file to disk.
,
Oct 19 2016
BTW, the renderer code in Blink/Webkit has been that way for years. I'm surprised it took us ~1 year to hit this DCHECK. Maybe we need more testing here. See also bug 632454 .
,
Oct 24 2016
I think we should keep the DCHECK and just remove the code in Blink that sets the initialValue: https://codereview.chromium.org/2446743002 It's only used for picking files/dirs for upload and the browser already remembers the last folder.
,
Oct 24 2016
"Maybe we need more testing here." Yep. And that said, suggestions on how to add tests to these would be welcome: https://codereview.chromium.org/2446743002 https://codereview.chromium.org/2450543002
,
Oct 25 2016
,
Oct 25 2016
Moving ownership as per #6 and #7.
,
Oct 25 2016
jsbell: Re: testing. See TestSelectFileDialogFactory in ppapi_filechooser_browsertest.cc (https://cs.chromium.org/chromium/src/chrome/test/ppapi/ppapi_filechooser_browsertest.cc?rcl=0&l=32). Were you referring to something like that in #7?
,
Oct 26 2016
,
Oct 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/14785d90f691b2b20d07612a62ba35a630bbb9ac commit 14785d90f691b2b20d07612a62ba35a630bbb9ac Author: jsbell <jsbell@chromium.org> Date: Wed Oct 26 21:37:26 2016 Remove WebFileChooserParams::initialValue On the Chromium side, FileSelectHelper::RunFileChooser asserts that default filenames should only be specified for "Save" choosers. Blink doesn't use those ("download" is a browser thing) so just remove the field which would be ignored anyway. (Remembering the last used directory is done through other means in the browser.) BUG= 632454 , 655298 Review-Url: https://codereview.chromium.org/2446743002 Cr-Commit-Position: refs/heads/master@{#427814} [modify] https://crrev.com/14785d90f691b2b20d07612a62ba35a630bbb9ac/content/browser/fileapi/file_system_browsertest.cc [add] https://crrev.com/14785d90f691b2b20d07612a62ba35a630bbb9ac/content/browser/fileapi/fileapi_browsertest.cc [modify] https://crrev.com/14785d90f691b2b20d07612a62ba35a630bbb9ac/content/renderer/pepper/pepper_file_chooser_host.cc [modify] https://crrev.com/14785d90f691b2b20d07612a62ba35a630bbb9ac/content/renderer/render_frame_impl.cc [modify] https://crrev.com/14785d90f691b2b20d07612a62ba35a630bbb9ac/content/renderer/render_frame_impl.h [modify] https://crrev.com/14785d90f691b2b20d07612a62ba35a630bbb9ac/content/test/BUILD.gn [modify] https://crrev.com/14785d90f691b2b20d07612a62ba35a630bbb9ac/content/test/content_browser_test_utils_internal.cc [modify] https://crrev.com/14785d90f691b2b20d07612a62ba35a630bbb9ac/content/test/content_browser_test_utils_internal.h [modify] https://crrev.com/14785d90f691b2b20d07612a62ba35a630bbb9ac/third_party/WebKit/Source/web/ChromeClientImpl.cpp [modify] https://crrev.com/14785d90f691b2b20d07612a62ba35a630bbb9ac/third_party/WebKit/public/web/WebFileChooserParams.h
,
Oct 26 2016
Fixed, i.e. we no longer pass a value through from Blink that hits the DCHECK. The DCHECK is still valid. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 Deleted