GetDialogOriginRelationship breaks in the presence of non-http(s) pages |
||||||||
Issue description
Chrome Version: 69.0.3452.0 (Official Build) dev (64-bit) (cohort: Dev)
OS: Windows, ChromeOS
What steps will reproduce the problem?
(1) Open a new tab. Navigate it to about:blank.
(2) In inspector: paste and execute "document.body.appendChild(document.createElement('iframe')); window[0].onbeforeunload = () => { return 'abc'; };"
(3) Navigate the tab to https://example.com.
What is the expected result?
No crash.
What happens instead?
Crash!
It's because GetDialogOriginRelationship attempts to walk up the frame tree to look for an HTTP or HTTPS origin; in this case, it fails, and promptly dereferences a null pointer.
,
Jun 20 2018
Same thing for me... Web browsing is impossible. Always crash.
,
Jun 20 2018
,
Jun 20 2018
I am able to reproduce the crash on the reported version: 69.0.3452.0 on Windows-7(crash id: e83669fcaeda746f). However the same seems to be resolved on the latest Windows dev 69.0.3465.0 and issue is no longer seen. Probably fixed already by avi@ in https://chromium-review.googlesource.com/c/chromium/src/+/1089659. dcheng@: Could you please confirm whether this is resolved at your end as well on the latest Dev. Thank you! Stack trace of e83669fcaeda746f: Thread 0 (id: 0x28b4) CRASHED [EXCEPTION_ACCESS_VIOLATION_READ @ 0x00000000 ] MAGIC SIGNATURE THREAD Stack Quality100%Show frame trust levels 0x000007febe784f02 (chrome.dll -javascript_dialog_tab_helper.cc:110 ) `anonymous namespace'::GetDialogOriginRelationship 0x000007febe785096 (chrome.dll -javascript_dialog_tab_helper.cc:316 ) JavaScriptDialogTabHelper::RunBeforeUnloadDialog(content::WebContents *,content::RenderFrameHost *,bool,base::OnceCallback<void >) 0x000007febd885eb1 (chrome.dll -web_contents_impl.cc:4978 ) content::WebContentsImpl::RunBeforeUnloadConfirm(content::RenderFrameHost *,bool,IPC::Message *) 0x000007febd6a40c8 (chrome.dll -render_frame_host_impl.cc:2166 ) content::RenderFrameHostImpl::OnRunBeforeUnloadConfirm(bool,IPC::Message *) 0x000007febd6a3f23 (chrome.dll -ipc_message_templates.h:229 ) IPC::MessageT<FrameHostMsg_RunBeforeUnloadConfirm_Meta,std::tuple<bool>,std::tuple<bool,std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > > >::DispatchDelayReply<content::RenderFrameHostImpl,void,void (content::RenderFrameHostImpl::*)(bool, IPC::Message *)> 0x000007febcfff25c (chrome.dll -render_frame_host_impl.cc:982 ) content::RenderFrameHostImpl::OnMessageReceived(IPC::Message const &) 0x000007febcffe66a (chrome.dll -render_process_host_impl.cc:3044 ) content::RenderProcessHostImpl::OnMessageReceived(IPC::Message const &) 0x000007febcffe550 (chrome.dll -ipc_channel_proxy.cc:320 ) IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message const &) 0x000007febca5ff14 (chrome.dll -task_annotator.cc:101 ) base::debug::TaskAnnotator::RunTask(char const *,base::PendingTask *) 0x000007febca5f9eb (chrome.dll -message_loop.cc:319 ) base::MessageLoop::RunTask(base::PendingTask *) 0x000007febca5f437 (chrome.dll -message_loop.cc:373 ) base::MessageLoop::DoWork() 0x000007febcb4f868 (chrome.dll -message_pump_win.cc:173 ) base::MessagePumpForUI::DoRunLoop() 0x000007febcaa4ac7 (chrome.dll -message_pump_win.cc:56 ) base::MessagePumpWin::Run(base::MessagePump::Delegate *) 0x000007febca5ef90 (chrome.dll -run_loop.cc:102 ) base::RunLoop::Run() 0x000007febcde1873 (chrome.dll -chrome_browser_main.cc:2194 ) ChromeBrowserMainParts::MainMessageLoopRun(int *) 0x000007febcde1677 (chrome.dll -browser_main_loop.cc:983 ) content::BrowserMainLoop::RunMainMessageLoopParts() 0x000007febcde1622 (chrome.dll -browser_main_runner_impl.cc:169 ) content::BrowserMainRunnerImpl::Run() 0x000007febd5c56ce (chrome.dll -browser_main.cc:51 ) content::BrowserMain(content::MainFunctionParams const &,std::unique_ptr<content::BrowserProcessSubThread,std::default_delete<content::BrowserProcessSubThread> >) 0x000007febdb177fa (chrome.dll -content_main_runner_impl.cc:620 ) content::RunBrowserProcessMain(content::MainFunctionParams const &,content::ContentMainDelegate *,std::unique_ptr<content::BrowserProcessSubThread,std::default_delete<content::BrowserProcessSubThread> >) 0x000007febca55765 (chrome.dll -content_main_runner_impl.cc:964 ) content::ContentMainRunnerImpl::Run() 0x000007febca45172 (chrome.dll -main.cc:459 ) service_manager::Main(service_manager::MainParams const &) 0x000007febca44a07 (chrome.dll -content_main.cc:19 ) content::ContentMain(content::ContentMainParams const &) 0x000007febca41af1 (chrome.dll -chrome_main.cc:101 ) ChromeMain 0x000000013fde35d5 (chrome.exe -main_dll_loader_win.cc:201 ) MainDllLoader::Launch(HINSTANCE__ *,base::TimeTicks) 0x000000013fde1698 (chrome.exe -chrome_exe_main_win.cc:230 ) wWinMain 0x000000013fea07d5 (chrome.exe -exe_common.inl:283 ) __scrt_common_main_seh 0x778559cc (kernel32.dll + 0x000159cc ) BaseThreadInitThunk 0x779b383c (ntdll.dll + 0x0005383c ) RtlUserThreadStart
,
Jun 20 2018
This is still broken on canary. Looking at that CL, I don't think it'd fix it as there's still no null check in that loop. While having only non-http or non-https domain in a page isn't the most common scenario, it's trivial to hit this, so we should fix it.
,
Jun 20 2018
From the crash server stats seems like this regression was introduced in M66. A reasonable # of instances are reported in Stable versions as well. 69.0.3466.0 0.12% 1 69.0.3465.0 0.12% 1 69.0.3464.0 0.12% 1 69.0.3452.0 0.12% 1 68.0.3409.2 0.12% 1 68.0.3405.0 0.12% 1 67.0.3396.87 21.85% 182 67.0.3396.79 5.88% 49 67.0.3396.62 3.96% 33 67.0.3396.48 0.36% 3 67.0.3396.40 0.12% 1 67.0.3396.30 0.24% 2 67.0.3396.18 0.24% 2 67.0.3396.10 0.12% 1 67.0.3381.1 0.96% 8 67.0.3377.1 0.24% 2 67.0.3371.0 0.24% 2 66.0.3359.181 38.30% 319 66.0.3359.170 1.32% 11 66.0.3359.158 0.12% 1 66.0.3359.139 21.61% 180 66.0.3359.126 0.12% 1 66.0.3359.117 2.52% 21 66.0.3359.106 0.24% 2 66.0.3359.66 0.24% 2 66.0.3359.45 0.24% 2 66.0.3359.33 0.12% 1 66.0.3359.22 0.24% 2 Link to the builds which introduced the crash ============================================== https://crash.corp.google.com/browse?q=expanded_custom_data.ChromeCrashProto.ptype%3D%27browser%27+AND+expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D%27%60anonymous+namespace%5C%27%3A%3AGetDialogOriginRelationship%27#-property-selector,productname:1000,productversion:1000,-magicsignature:50,-magicsignature2:50,-stablesignature:50,-magicsignaturesorted:50 Possible suspect ================= https://chromium.googlesource.com/chromium/src/+/5eb6e6f5c005711ee0cb3b18c8b928fb5164f560 Assigning to Avi for more updates.
,
Jun 20 2018
Since avi@ is OOO, assigning back to reviewer estark@.
,
Jun 25 2018
Re comment 5: "Looking at that CL, I don't think it'd fix it as there's still no null check in that loop." Not having a null check is intentional, because the very first thing in the GetDialogOriginRelationship() function is a return if the top-level isn't HTTP(S). It *should* be guaranteed to terminate. I just tried this, logging the URL of the main frame, and in the repro from the OP I get: main_frame_url http://www.example.com/ Which is **wrong**. We're in the middle of a beforeunload on the about:blank page. Why is the WebContents saying that its URL is example.com before we've even finished the beforeunload of the previous page?
,
Jun 25 2018
,
Jun 25 2018
// Gets the URL that is currently being displayed, if there is one. // This method is deprecated. DO NOT USE! Pick either |GetVisibleURL| or // |GetLastCommittedURL| as appropriate. virtual const GURL& GetURL() const = 0; Oh.
,
Jun 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/da0c64a5292fa2d930ce74f525023b708e6f3dc1 commit da0c64a5292fa2d930ce74f525023b708e6f3dc1 Author: Avi Drissman <avi@chromium.org> Date: Mon Jun 25 18:00:01 2018 Don't use WebContents::GetURL(). WebContents::GetURL() is deprecated because there's no way to know *what* URL it's giving you. In this particular case, it pulled a URL from the *future* of a page's navigation history, which caused a loop to not terminate properly. (And all the metrics are wrong :( ouch ) BUG= 854430 TEST=as in bug Change-Id: I798af0278ffde3421fa933644513fafba8bd0b2d Reviewed-on: https://chromium-review.googlesource.com/1113660 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Commit-Queue: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#570092} [modify] https://crrev.com/da0c64a5292fa2d930ce74f525023b708e6f3dc1/chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
,
Jun 25 2018
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by dcheng@chromium.org
, Jun 20 2018Owner: est...@chromium.org