Fixed by https://chromium-review.googlesource.com/c/chromium/src/+/937762

+isharman

Looks like our fix (https://chromium-review.googlesource.com/c/chromium/src/+/908371) reaches NOTREACHED() at https://cs.chromium.org/chromium/src/components/autofill/content/renderer/autofill_agent.cc?q=autofill_agent.cc&sq=package:chromium&dr&l=475 on the ASAN bots, although I couldn't reproduce this on my local machine.

isherman@, do you think it is OK to remove NOTREACHED as we can now have a reproduceable case for this assertion?

Ref: https://bugs.chromium.org/p/chromium/issues/detail?id=96321#c6

Over to Mathieu to triage. That said, I would be surprised if a browser test were genuinely providing a reproducible and realistic test case, rather than somehow accidentally tickling a code path by performing a sequence of actions that wouldn't normally come up in a real page. In other words, I'd be surprised if this test failure corresponded to clear user-performable repro steps.

This could be reproduced on tasak@'s machine, with dcheck on.

> I'd be surprised if this test failure corresponded to clear user-performable repro steps.

Before our leak fix, it was really hard to reproduce this problem since the element's lifetime is determined by the autofill manager, which holds the element much longer than needed. Our leak fix derefs the element as soon as possible and now we encountered this NONREACHED() assertion error reliably.

The symbolized crash log:

$ ./out/Asan/browser_tests --no-sandbox --gtest_filter=PasswordManagerBrowserTestBase.PasswordOverridenUpdateBubbleShown

[150154:150154:0227/181512.509438:FATAL:autofill_agent.cc(468)] Check failed: false.
    #0 0x00000a235af1 in __interceptor_backtrace /b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:3957:13
    #1 0x000014fb70be in base::debug::StackTrace::StackTrace(unsigned long) ./../../base/debug/stack_trace_posix.cc:808:41
    #2 0x00001503061c in logging::LogMessage::~LogMessage() ./../../base/logging.cc:581:29
    #3 0x000024f1feb4 in autofill::AutofillAgent::ClearPreviewedForm() ./../../components/autofill/content/renderer/autofill_agent.cc:468:5
    #4 0x00000ca52cf9 in autofill::mojom::AutofillAgentStubDispatch::Accept(autofill::mojom::AutofillAgent*, mojo::Message*) ./gen/components/autofill/content/common/autofill_agent.mojom.cc:721:13
    #5 0x000018d2ab45 in mojo::InterfaceEndpointClient::HandleValidatedMessage(mojo::Message*) ./../../mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:419:32
    #6 0x000018d291dc in mojo::FilterChain::Accept(mojo::Message*) ./../../mojo/public/cpp/bindings/lib/filter_chain.cc:40:17
    #7 0x000018d2e96d in mojo::InterfaceEndpointClient::HandleIncomingMessage(mojo::Message*) ./../../mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:306:19
    #8 0x000018d46578 in mojo::internal::MultiplexRouter::ProcessIncomingMessage(mojo::internal::MultiplexRouter::MessageWrapper*, mojo::internal::MultiplexRouter::ClientCallBehavior, base::SequencedTaskRunner*) ./../../mojo/public/cpp/bindings/lib/multiplex_router.cc:880:42
    #9 0x000018d452e0 in mojo::internal::MultiplexRouter::Accept(mojo::Message*) ./../../mojo/public/cpp/bindings/lib/multiplex_router.cc:604:38
    #10 0x000018d291dc in mojo::FilterChain::Accept(mojo::Message*) ./../../mojo/public/cpp/bindings/lib/filter_chain.cc:40:17
    #11 0x000018d199e2 in mojo::Connector::ReadSingleMessage(unsigned int*) ./../../mojo/public/cpp/bindings/lib/connector.cc:445:51
    #12 0x000018d1b914 in mojo::Connector::ReadAllAvailableMessages() ./../../mojo/public/cpp/bindings/lib/connector.cc:474:10
    #13 0x000018d1b2a5 in mojo::Connector::OnHandleReadyInternal(unsigned int) ./../../mojo/public/cpp/bindings/lib/connector.cc:375:3
    #14 0x000018d1eb21 in void base::internal::Invoker<base::internal::BindState<void (mojo::Connector::*)(unsigned int), base::internal::UnretainedWrapper<mojo::Connector> >, void (unsigned int)>::RunImpl<void (mojo::Connector::* const&)(unsigned int), std::__1::tuple<base::internal::UnretainedWrapper<mojo::Connector> > const&, 0ul>(void (mojo::Connector::* c\
onst&)(unsigned int), std::__1::tuple<base::internal::UnretainedWrapper<mojo::Connector> > const&, std::__1::integer_sequence<unsigned long, 0ul>, unsigned int&&) ./../../base/bind_internal.h:604:12
    #15 0x000018d1e984 in base::internal::Invoker<base::internal::BindState<void (mojo::Connector::*)(unsigned int), base::internal::UnretainedWrapper<mojo::Connector> >, void (unsigned int)>::Run(base::internal::BindStateBase*, unsigned int) ./../../base/bind_internal.h:586:12
    #16 0x00000f44a81c in base::internal::Invoker<base::internal::BindState<void (*)(base::RepeatingCallback<void (unsigned int)> const&, unsigned int, mojo::HandleSignalsState const&), base::RepeatingCallback<void (unsigned int)> >, void (unsigned int, mojo::HandleSignalsState const&)>::Run(base::internal::BindStateBase*, unsigned int, mojo::HandleSignalsStat\
e const&) ./../../base/bind_internal.h:586:12
    #17 0x000018d023d8 in mojo::SimpleWatcher::OnHandleReady(int, unsigned int, mojo::HandleSignalsState const&) ./../../mojo/public/cpp/system/simple_watcher.cc:276:14
    #18 0x00000a48ca92 in base::OnceCallback<void ()>::Run() && ./../../base/callback.h:95:12
    #19 0x000014fbd8ca in base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) ./../../base/debug/task_annotator.cc:55:33
    #20 0x0000146d3604 in blink::scheduler::internal::ThreadControllerImpl::DoWork(blink::scheduler::internal::Sequence::WorkType) ./../../third_party/WebKit/Source/platform/scheduler/base/thread_controller_impl.cc:162:21
    #21 0x00000a48ca92 in base::OnceCallback<void ()>::Run() && ./../../base/callback.h:95:12
    #22 0x000014fbd8ca in base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) ./../../base/debug/task_annotator.cc:55:33
    #23 0x000015059d21 in base::internal::IncomingTaskQueue::RunTask(base::PendingTask*) ./../../base/message_loop/incoming_task_queue.cc:124:19
    #24 0x000015053557 in base::MessageLoop::RunTask(base::PendingTask*) ./../../base/message_loop/message_loop.cc:395:25
    #25 0x000015053b77 in base::MessageLoop::DeferOrRunPendingTask(base::PendingTask) ./../../base/message_loop/message_loop.cc:407:5
    #26 0x000015053f3b in base::MessageLoop::DoWork() ./../../base/message_loop/message_loop.cc:451:16
    #27 0x000015062687 in base::MessagePumpDefault::Run(base::MessagePump::Delegate*) ./../../base/message_loop/message_pump_default.cc:37:31
    #28 0x00001505243d in base::MessageLoop::Run(bool) ./../../base/message_loop/message_loop.cc:346:12
    #29 0x00001511794a in base::RunLoop::Run() ./../../base/run_loop.cc:133:14
    #30 0x00002560fc51 in content::RendererMain(content::MainFunctionParams const&) ./../../content/renderer/renderer_main.cc:235:23
    #31 0x000014d09910 in content::RunZygote(content::ContentMainDelegate*) ./../../content/app/content_main_runner.cc:352:14
    #32 0x000014d0ac50 in content::RunNamedProcessTypeMain(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, content::MainFunctionParams const&, content::ContentMainDelegate*) ./../../content/app/content_main_runner.cc:431:12
    #33 0x000014d0e523 in content::ContentMainRunnerImpl::Run() ./../../content/app/content_main_runner.cc:703:12
    #34 0x00001b6d010e in service_manager::Main(service_manager::MainParams const&) ./../../services/service_manager/embedder/main.cc:453:29
    #35 0x000014d09213 in content::ContentMain(content::ContentMainParams const&) ./../../content/app/content_main.cc:19:10
    #36 0x0000168beb42 in content::LaunchTests(content::TestLauncherDelegate*, unsigned long, int, char**) ./../../content/public/test/test_launcher.cc:613:12
    #37 0x000014f8b1d6 in LaunchChromeTests(unsigned long, content::TestLauncherDelegate*, int, char**) ./../../chrome/test/base/chrome_test_launcher.cc:171:10
    #38 0x000014f8a36c in main ./../../chrome/test/base/browser_tests_main.cc:36:10
    #39 0x7f133b74c2b1 in __libc_start_main ??:0:0
    #40 0x00000a1ea02a in _start ??:0:0

 Issue 818640  has been merged into this issue.

Related CL: https://chromium-review.googlesource.com/c/chromium/src/+/939224

hajimehoshi@: Do you have repro steps that I could try in Chrome (outside of the failing test)? I'd like to make sure I understand what's happening.

Thank you!

Hi sebsg@,

Sorry bug not yet since I'm not sure when ClearPreviewedForm is called and when DidCommitProvisionalLoad is called. I'd be glad if you could help me. Thank you!

> Sorry bug
Sorry but

I'll investigate this today and write my findings here.

I think this is more an issue with the password autofill.

The ClearPreviewedForm happens for addresses and cards when the user hovers out of a suggestion from the Autofill popup. My guess is that this never really happens at the same time as a provisional load. I guess it's still possible but should be pretty rare.

The password manager though previews the username and password on page load. Those are then set only when the user interacts with the page. So I think there is a bigger risk there.

Related to isherman@'s comment about the race: You are correct, one comes from the browser and the other from the renderer.

I'm adding some password manager folks for their take on this. But for Addresses and Cards I would not mind removing the DCHECK for now and then investigate this issue. It should have not visible effect for the user.

+battre@ could I have your thoughts on this? Related to comment #13. 

friendly ping?

From point of view of Password Manager I'm ok with removing DCHECK. I think that the reverted CL https://chromium-review.googlesource.com/c/chromium/src/+/597482 is correct, |element_| is not needed after navigation: we don't need to clear Preview Suggesions after navigation.

dvadym@

Thanks! I'd be happy if someone could review https://chromium-review.googlesource.com/c/chromium/src/+/939224, that fixes the reverted cause (NOTREACHED()).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/83c3f50b87c1b4e89be296a5d2adae9777bb0805

commit 83c3f50b87c1b4e89be296a5d2adae9777bb0805
Author: Hajime Hoshi <hajimehoshi@chromium.org>
Date: Wed Mar 14 05:49:01 2018

Re^2-land: Fix a leak of a document that is held by AutofillAgent

This is a retry of https://chromium-review.googlesource.com/c/chromium/src/+/908371,
with fixing the DCHECK (NOTREACHED) error. We were not sure when the
NOTREACHED is reached before the fix, but now this case becomes
deterministic (see the discussion at crbug.com/816533)

Note that this might still cause Telemetry regressions reported at
 crbug.com/753071  before, but this is expected.

A document is leaked because AutofillAgent hold a lastly used element
even after navigation happens, and the document of the element remains
until a new (input) element is focused. This CL fixes this issue by
resetting the element in AutofillAgent when navigation happens.

Bug: 734427, 755489, 816533
Change-Id: I7eb2f9fb879d826297247c4c66e366ec0b73067c
Reviewed-on: https://chromium-review.googlesource.com/939224
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Hajime Hoshi <hajimehoshi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543015}
[modify] https://crrev.com/83c3f50b87c1b4e89be296a5d2adae9777bb0805/components/autofill/content/renderer/autofill_agent.cc