New issue
Advanced search Search tips

Issue 854430 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

GetDialogOriginRelationship breaks in the presence of non-http(s) pages

Project Member Reported by dcheng@chromium.org, Jun 20 2018

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.
 

Comment 1 by dcheng@chromium.org, Jun 20 2018

Cc: a...@chromium.org
Owner: est...@chromium.org

Comment 2 by stegau...@gmail.com, Jun 20 2018

Same thing for me... Web browsing is impossible. Always crash.

Comment 3 by gov...@chromium.org, Jun 20 2018

Cc: ligim...@chromium.org ajha@chromium.org
Labels: -M-67 Needs-Triage-M69 M-69

Comment 4 by ajha@chromium.org, Jun 20 2018

Labels: -ReleaseBlock-Dev Needs-Feedback
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

Comment 5 by dcheng@chromium.org, Jun 20 2018

Labels: -Needs-Feedback ReleaseBlock-Dev
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.
Cc: -a...@chromium.org est...@chromium.org
Labels: -ReleaseBlock-Dev ReleaseBlock-Stable FoundIn-67 Target-69 FoundIn-68 FoundIn-69
Owner: a...@chromium.org
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.
Cc: -est...@chromium.org a...@chromium.org
Owner: est...@chromium.org
Since avi@ is OOO, assigning back to reviewer estark@.

Comment 8 by a...@chromium.org, 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?

Comment 9 by a...@chromium.org, Jun 25 2018

Owner: a...@chromium.org
Status: Started (was: Assigned)

Comment 10 by a...@chromium.org, 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.
Project Member

Comment 11 by bugdroid1@chromium.org, 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

Comment 12 by a...@chromium.org, Jun 25 2018

Status: Fixed (was: Started)

Sign in to add a comment