New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 833807 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug

Blocking:
issue 792131



Sign in to add a comment

enumerate_input_method_editors_win.cc(51)] Check failed: std::is_sorted with lld

Project Member Reported by h...@chromium.org, Apr 17 2018

Issue description

For example from https://ci.chromium.org/buildbot/tryserver.chromium.win/win10_chromium_x64_rel_ng/125026:

[ RUN      ] PlatformAppBrowserTest.SandboxedLocalFile
[6232:7768:0416/152558.298:ERROR:install_util.cc(589)] Unable to create registry key HKLM\SOFTWARE\Policies\Chromium for reading result=2
[6232:6108:0416/152558.350:WARNING:discovery_network_list_win.cc(195)] Failed to open Wlan client handle: 1062
[6232:7768:0416/152558.350:WARNING:chrome_browser_main_win.cc(630)] Command line too long for RegisterApplicationRestart:  --brave-new-test-launcher --cfi-diag=0 --gtest_also_run_disabled_tests --gtest_filter=PlatformAppBrowserTest.SandboxedLocalFile --single_process --test-launcher-bot-mode --test-launcher-output="C:\Users\CHROME~1\AppData\Local\Temp\scoped_dir4640_28674\results4640_7301\test_results.xml" --test-launcher-summary-output="C:\b\s\w\iobcnji1\output.json" --user-data-dir="C:\Users\CHROME~1\AppData\Local\Temp\scoped_dir4640_28674\d4640_15282" --disable-offline-auto-reload --allow-legacy-extension-manifests --no-first-run --no-default-browser-check --enable-logging=stderr --disable-default-apps --wm-window-animations-disabled --disable-component-update --test-type=browser --force-color-profile=srgb --disable-zero-browsers-open-for-tests --ipc-connection-timeout=30 --allow-file-access-from-files --dom-automation --log-gpu-control-list-decisions --disable-backgrounding-occluded-windows --disable-gl-drawing-for-tests --override-use-software-gl-for-tests --force-color-profile=srgb --disable-compositor-ukm-for-tests --enable-features=TestFeatureForBrowserTest1 --disable-features=NetworkPrediction,TestFeatureForBrowserTest2 --flag-switches-begin --flag-switches-end --restore-last-session about:blank
[6232:7768:0416/152558.462:WARNING:gaia_auth_fetcher.cc(873)] Could not reach Google Accounts servers: errno -11
[6232:6780:0416/152558.381:FATAL:enumerate_input_method_editors_win.cc(51)] Check failed: std::is_sorted(std::begin(kMicrosoftImeGuids), std::end(kMicrosoftImeGuids)). 
Backtrace:
	base::debug::StackTrace::StackTrace [0x00007FF61D47C0B4+36]
	logging::LogMessage::~LogMessage [0x00007FF61D497AC2+98]
	EnumerateInputMethodEditors [0x00007FF61FB017C5+617]
	base::internal::FunctorTraits<void (__cdecl*)(scoped_refptr<base::SequencedTaskRunner>,base::RepeatingCallback<void __cdecl(base::FilePath const & __ptr64,unsigned int,unsigned int)>,base::OnceCallback<void __cdecl(void)>),void>::Invoke<scoped_refptr<base [0x00007FF61FB0141A+94]
	base::debug::TaskAnnotator::RunTask [0x00007FF61D47CA8E+382]
	base::internal::TaskTracker::RunOrSkipTask [0x00007FF61E679D76+1158]
	base::internal::TaskTracker::RunAndPopNextTask [0x00007FF61E6790FE+494]
	base::internal::SchedulerWorker::Thread::ThreadMain [0x00007FF620705526+918]
	base::PlatformThread::GetCurrentThreadPriority [0x00007FF61D4E830C+572]
	BaseThreadInitThunk [0x00007FFF2AC02774+20]
	RtlUserThreadStart [0x00007FFF2D260D51+33]
[4/832] PlatformAppBrowserTest.SandboxedLocalFile (CRASHED)


It looks like the code is actually checking that the pointers are sorted rather than the strings being sorted.
 

Comment 2 by thakis@chromium.org, Apr 17 2018

Cc: p...@chromium.org
Nice digging.

I wonder why we didn't hit this earlier. Shouldn't the pointers be sorted too? Do we ICF them now when we weren't before?

Also, why didn't the tot bots see this?


  static constexpr const wchar_t* kMicrosoftImeGuids[] = {
      L"{0000897b-83df-4b96-be07-0fb58b01c4a4}",
      L"{03b5835f-f03c-411b-9ce2-aa23e1171e36}",
      L"{07eb03d6-b001-41df-9192-bf9b841ee71f}",
      L"{3697c5fa-60dd-4b56-92d4-74a569205c16}",
      L"{531fdebf-9b4c-4a43-a2aa-960e8fcdc732}",
      L"{6a498709-e00b-4c45-a018-8f9e4081ae40}",
      L"{78cb5b0e-26ed-4fcc-854c-77e8f3d1aa80}",
      L"{81d4e9c9-1d3b-41bc-9e6c-4b40bf79e35e}",
      L"{8613e14c-d0c0-4161-ac0f-1dd2563286bc}",
      L"{a028ae76-01b1-46c2-99c4-acd9858ae02f}",
      L"{a1e2b86b-924a-4d43-80f6-8a820df7190f}",
      L"{ae6be008-07fb-400d-8beb-337a64f7051f}",
      L"{b115690a-ea02-48d5-a231-e3578d2fdf80}",
      L"{c1ee01f2-b3b6-4a6a-9ddd-e988c088ec82}",
      L"{dcbd6fa8-032f-11d3-b5b1-00c04fc324a1}",
      L"{e429b25a-e5d3-4d1f-9be3-0c608477e3a1}",
      L"{f25e9f57-2fc8-4eb3-a41a-cce5f08541e6}",
      L"{f89e9e58-bd2f-4008-9ac2-0f816c09f4ee}",
      L"{fa445657-9379-11d6-b41a-00065b83ee53}",
  };

Comment 3 by thakis@chromium.org, Apr 17 2018

Cc: ruiu@google.com
+ruiu too, maybe he knows what changed so that lld no longer emits the pointers in comment 2 in order. (assuming it was an ld change.)

Looks like there are two copies of this list in the code:

https://cs.chromium.org/chromium/src/chrome_elf/third_party_dlls/imes.cc?q=0000897b-83df-4b96-be07-0fb58b01c4a4&sq=package:chromium&dr=C&l=176
https://cs.chromium.org/chromium/src/chrome/browser/conflicts/enumerate_input_method_editors_win.cc?type=cs&q=0000897b-83df-4b96-be07-0fb58b01c4a4&sq=package:chromium&l=29

Maybe we ICF them and pick different canonical elements for different elements of the array?
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 17 2018

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

commit fc360e99b44bb77294f9ca099ab6126455910abd
Author: Hans Wennborg <hans@chromium.org>
Date: Tue Apr 17 13:24:22 2018

Fix DCHECK to actually check the strings are sorted

The previous DCHECK was comparing the pointer values, not the strings.

Bug:  833807 
Change-Id: I0e63eda10838de481c51630f8932b41537a76847
Reviewed-on: https://chromium-review.googlesource.com/1013525
Commit-Queue: Hans Wennborg <hans@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551308}
[modify] https://crrev.com/fc360e99b44bb77294f9ca099ab6126455910abd/chrome/browser/conflicts/enumerate_input_method_editors_win.cc

Comment 5 by thakis@chromium.org, Apr 17 2018

Status: Fixed (was: Started)

Comment 6 by p...@chromium.org, Apr 17 2018

FWIW, there's no guarantee about the relative order of strings; the compiler/linker is free to emit them in any order and even to merge them (see issue 810154, which might have been part of the cause).

Comment 7 by thakis@chromium.org, Apr 17 2018

Understood; the code was clearly wrong before, I'm just curious what changed.

I thought I had put in a link to the tail merging revision but apparently I didn't in the end -- but I had checked and I think that landed before the last switch attempt already, and we didn't see this failure then.

I just thought it'd be good to know what changed, but if nobody knows off the top of their heads, it's not super important.

Sign in to add a comment