Fix crash on first startup in EEA

[t-m-w]

Description

Solves an issue in which, with a system language whose country code is a country within the European Economic Area (extension of the EU), the browser may crash in template_url_prepopulate_data.cc with this error: Check failed: t_urls.size() <= kMaxEeaPrepopulatedEngines (10 vs. 8)

Discovered by a user in the CalyxOS Development channel. Does not appear to affect Cromite itself for some reason, but may impact forks.

All submissions

• there are no other open Pull Requests for the same update/change
• Bromite can be built with these changes
• I have tested that the new change works as intended (AVD or physical device will do)

Format

• patch subject and filename match (e.g. Subject: Alternative cache (NIK-based) -> Alternative-cache-NIK-based.patch)
• patch description contains explanation of changes
• no unnecessary whitespace or unrelated changes

[uazo]

Does not appear to affect Cromite itself for some reason, but may impact forks.

kSearchEngineChoiceTrigger is disabled by default, so that code is not called.

cromite/build/patches/Enable-search-engine-settings-desktop-ui.patch

Line 1900 in 426f48f

+SET_CROMITE_FEATURE_DISABLED(kSearchEngineChoiceTrigger);

Check failed: t_urls.size() <= kMaxEeaPrepopulatedEngines (10 vs. 8)

that check is only required to make static the maximum histogram value generated by RecordChoiceScreenSelectedIndex.
it would be better to remove TemplateURLPrepopulateData::kMaxEeaPrepopulatedEngines directly in order not to run into future crash.

[uazo]

Discovered by a user in the CalyxOS Development channel.

may I ask how to see it?

[t-m-w]

Makes sense, Enable-search-engine-settings-desktop-ui.patch is not one we include currently.

it would be better to remove TemplateURLPrepopulateData::kMaxEeaPrepopulatedEngines directly in order not to run into future crash.

Sounds good to me!

Discovered by a user in the CalyxOS Development channel.

may I ask how to see it?

How to see the chat? Sure, it is the CalyxOS Development channel from this list. Here is a Matrix permalink to around the time this was discussed.

The user discovered the issue because they kept running into it when we couldn't repro it, and they kept investigating. They didn't refer to this as a proper fix but said it worked around the problem for them. When I looked at it, the idea looked okay to me, and it worked. If kMaxEeaPrepopulatedEngines can simply be pulled out, that sounds good, too.

[t-m-w]

it would be better to remove TemplateURLPrepopulateData::kMaxEeaPrepopulatedEngines directly in order not to run into future crash.

It's referenced in a lot of places, particularly if we count the tests. What if we just increase the number to something ridiculously high like 10000? Or what do you prefer?

I got a little stuck on what should go in RecordChoiceScreenSelectedIndex if we remove kMaxEeaPrepopulatedEngines.

[uazo]

It's referenced in a lot of places, particularly if we count the tests

https://source.chromium.org/search?q=kMaxEeaPrepopulatedEngines
4 points including 2 tests, which I will never activate myself: the build of tests is already broken by many other patches

I got a little stuck on what should go in RecordChoiceScreenSelectedIndex if we remove kMaxEeaPrepopulatedEngines.

deletes the code completely: record the data as a histogram for statistics, I don't think you use them, or do you?

[t-m-w]

deletes the code completely: record the data as a histogram for statistics, I don't think you use them, or do you?

Nope, definitely don't, but it looked like it could break tests by removing that, which would be yet another annoyance to change. I'm not worried about tests either, so this sounds good. I'll update this.