Browser tests crashing when Refresh is enabled in OmniboxViewViews::InstallPlaceholderText |
|||||
Issue descriptionI investigated some failures in these two tests: PolicyPrefsTest.PolicyToPrefsMapping PolicyTest.DefaultSearchProvider It looks like TemplateURLService::GetDefaultSearchProvider is returning nullptr (loaded_ is true, default_search_provider_ is nullptr). That doesn't seem like a valid state to me, and even if it is I don't know what the correct behavior is. tommycli@, can you look at this? Example failures: https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux-chromeos-rel/24537 https://ci.chromium.org/p/chromium/builders/luci.chromium.try/win10_chromium_x64_rel_ng/28636
,
Jun 14 2018
,
Jun 14 2018
Alright let me take a look.
,
Jun 14 2018
Are these failures intermittent?
,
Jun 14 2018
No, seems pretty consistent. I can repro locally too.
,
Jun 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4bbe66b05051f19733841c417a0bc7154d5bd8fd commit 4bbe66b05051f19733841c417a0bc7154d5bd8fd Author: Tommy C. Li <tommycli@chromium.org> Date: Fri Jun 15 17:25:01 2018 Omnibox UI Refresh: Fix placeholder text if no default search provider During unit tests, or if the default search provider is disabled by policy, there may not be any default search provider. Previously, we would attempt to dereference a nullptr in that case, and would intermittently crash. This CL fixes that by simply clearing the placeholder text when there is no default search provider. This is fine - as before Material Refresh there was no placeholder text at all. Bug: 852630 Change-Id: Id7933f7972de20a41252f8d4f0f7bb16ce623037 Reviewed-on: https://chromium-review.googlesource.com/1101710 Reviewed-by: Bret Sepulveda <bsep@chromium.org> Commit-Queue: Tommy Li <tommycli@chromium.org> Cr-Commit-Position: refs/heads/master@{#567709} [modify] https://crrev.com/4bbe66b05051f19733841c417a0bc7154d5bd8fd/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
,
Jun 15 2018
bsep: I'm going to have to submit on more patch related to this issue. I'll send this to you in a second.
,
Jun 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c50826d338c27c1346136ace4a60df294a35c86f commit c50826d338c27c1346136ace4a60df294a35c86f Author: Tommy C. Li <tommycli@chromium.org> Date: Sat Jun 16 00:00:26 2018 Omnibox UI Refresh: Fix search favicon if no default search provider During unit tests, or if the default search provider is disabled by policy, there may not be any default search provider. Previously, when we went to fetch the favicon for the search provider, we did not account for the defaults search provider being potentially null, and this CL fixes that. Bug: 852630 Change-Id: I8637c684b1df286ac06506138b86b3237190ebb6 Reviewed-on: https://chromium-review.googlesource.com/1102882 Commit-Queue: Bret Sepulveda <bsep@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Cr-Commit-Position: refs/heads/master@{#567832} [modify] https://crrev.com/c50826d338c27c1346136ace4a60df294a35c86f/chrome/browser/ui/omnibox/chrome_omnibox_client.cc
,
Jun 16 2018
Okay I hope that should do the trick. Please ping me again if I'm causing you any more troubles on the Refresh-enabled tests.
,
Jun 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b1a1706d619e9758c4af9aabf1b5c1e86396118b commit b1a1706d619e9758c4af9aabf1b5c1e86396118b Author: Tommy C. Li <tommycli@chromium.org> Date: Mon Jun 18 23:08:58 2018 [Merge to M68] Omnibox UI: Fix bad merge due to usage of future flag This was a merge failed to compile, as we are using a flag that did not make branch cut. This CL removes those usages harmlessly and should fix compile. TBR=jdonnelly@chromium.org Bug: 852630 Change-Id: If83962e2475d268f661e4c55fe0ba5f547d25aea Reviewed-on: https://chromium-review.googlesource.com/1105407 Reviewed-by: Steven Holte <holte@chromium.org> Cr-Commit-Position: refs/branch-heads/3440@{#431} Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733} [modify] https://crrev.com/b1a1706d619e9758c4af9aabf1b5c1e86396118b/components/omnibox/browser/omnibox_field_trial.cc |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by bsep@chromium.org
, Jun 14 2018