New issue
Advanced search Search tips

Issue 639523 link

Starred by 2 users
Status: Fixed
Owner:
michae...@chromium.org
Closed: Aug 2016
Cc:
steve...@chromium.org
dpa...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 630982



Sign in to add a comment

MD Settings race condition in Languages

Project Member Reported by michae...@chromium.org, Aug 20 2016 
Making multiple changes in Languages could theoretically discard some changes,
because two different sources are used for the list of enabled languages (JS and
C++). With the new UI mocks, this can *always* happen, so we need to fix it.

Currently, to enable or disable a language, languages.js adds/removes that
language from its copy of the languages pref. It then sends that list to Chrome
via chrome.languageSettingsPrivate.SetLanguageList, which updates the pref; that
update is eventually sent back to the WebUI.

If languages.js tries to enable two languages in a row, it would be using a
stale pref for the second update. This could happen even if languages.js is
invoked asynchronous, but happens deterministically if the calls are made
synchronously because the old pref is used before the new one is updated.
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 23 2016 

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

commit 6de7389ad5a4245ea4d3a8c4aa7e9584725511a7
Author: michaelpg <michaelpg@chromium.org>
Date: Tue Aug 23 01:50:25 2016

Fix race condition in MD Settings Languages page causing inconsistent settings

A theoretical race condition becomes problematic when adding the ability to
enable multiple languages at once (as the new UI mocks specify).

This is easily fixed by replacing SetLanguageList with EnableLanguage and
DisableLanguage.

Currently, to enable or disable a language, languages.js adds/removes that
language from its copy of the languages pref. It then sends that list to Chrome
via chrome.languageSettingsPrivate.SetLanguageList, which updates the pref; that
update is eventually -- but not immediately -- sent back to the WebUI to update
its copy of the pref.

If languages.js tries to enable two languages in a row, it would be using a
stale pref for the second update. This could happen even if languages.js is
invoked asynchronous, but happens deterministically if the calls are made
synchronously because the old pref is used before the new one is updated.

BUG= 639523 
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2261903002
Cr-Commit-Position: refs/heads/master@{#413624}

[modify] https://crrev.com/6de7389ad5a4245ea4d3a8c4aa7e9584725511a7/chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc
[modify] https://crrev.com/6de7389ad5a4245ea4d3a8c4aa7e9584725511a7/chrome/browser/extensions/api/language_settings_private/language_settings_private_api.h
[modify] https://crrev.com/6de7389ad5a4245ea4d3a8c4aa7e9584725511a7/chrome/browser/resources/settings/languages_page/languages.js
[modify] https://crrev.com/6de7389ad5a4245ea4d3a8c4aa7e9584725511a7/chrome/common/extensions/api/language_settings_private.idl
[modify] https://crrev.com/6de7389ad5a4245ea4d3a8c4aa7e9584725511a7/chrome/test/data/webui/settings/fake_language_settings_private.js
[modify] https://crrev.com/6de7389ad5a4245ea4d3a8c4aa7e9584725511a7/extensions/browser/extension_function_histogram_value.h
[modify] https://crrev.com/6de7389ad5a4245ea4d3a8c4aa7e9584725511a7/third_party/closure_compiler/externs/language_settings_private.js
[modify] https://crrev.com/6de7389ad5a4245ea4d3a8c4aa7e9584725511a7/third_party/closure_compiler/interfaces/language_settings_private_interface.js
[modify] https://crrev.com/6de7389ad5a4245ea4d3a8c4aa7e9584725511a7/tools/metrics/histograms/histograms.xml

Comment 2 by michae...@chromium.org, Aug 26 2016

Status: Fixed (was: Started)
►

Sign in to add a comment