Fix race condition in MD Settings Languages page causing inconsistent settings

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

Committed: https://crrev.com/6de7389ad5a4245ea4d3a8c4aa7e9584725511a7
Cr-Commit-Position: refs/heads/master@{#413624}

[michaelpg, 2016-08-20 00:02:37]

Description was changed from

==========
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 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.

BUG=639523
R=stevenjb@chromium.org
==========

to

==========
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 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.

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

[michaelpg, 2016-08-20 00:06:14]

Description was changed from

==========
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 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.

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

to

==========
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
==========

[michaelpg, 2016-08-20 00:10:26]

PTAL.

[stevenjb, 2016-08-22 17:03:50]

lgtm

https://codereview.chromium.org/2261903002/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc
(right):

https://codereview.chromium.org/2261903002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:270:
parameters->language_code) == languages.end()) {
We should invert this and set a 'Language already enabled' error. The UI can
safely ignore the error, but it could be useful for debugging.

https://codereview.chromium.org/2261903002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:301:
if (it != languages.end()) {
ditto.

[michaelpg, 2016-08-22 18:24:46]

michaelpg@chromium.org changed reviewers:
+ isherman@chromium.org, rdevlin.cronin@chromium.org

[michaelpg, 2016-08-22 18:24:46]

PTAL -
devlin:
  chrome/common/extensions/api/language_settings_private.idl
isherman:
  extensions/browser/extension_function_histogram_value.h

Thanks!

https://codereview.chromium.org/2261903002/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc
(right):

https://codereview.chromium.org/2261903002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:270:
parameters->language_code) == languages.end()) {
On 2016/08/22 17:03:50, stevenjb wrote:
> We should invert this and set a 'Language already enabled' error. The UI can
> safely ignore the error, but it could be useful for debugging.

Done.

https://codereview.chromium.org/2261903002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:301:
if (it != languages.end()) {
On 2016/08/22 17:03:49, stevenjb wrote:
> ditto.

Done.

[Devlin, 2016-08-22 18:38:20]

idl lgtm, feel free to ignore my random drive-bys in the api.cc if they don't
make sense.

https://codereview.chromium.org/2261903002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc
(right):

https://codereview.chromium.org/2261903002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:276:
return RespondNow(Error(kErrorLanguageAlreadyEnabled));
drive-by: To me, this seems a little weird.  I would just silently let it
return.  But, you'll know best.

https://codereview.chromium.org/2261903002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:279:
languages.push_back(parameters->language_code);
drive-by: do we need to check that the language exists?

[stevenjb, 2016-08-22 18:41:31]

https://codereview.chromium.org/2261903002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc
(right):

https://codereview.chromium.org/2261903002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:276:
return RespondNow(Error(kErrorLanguageAlreadyEnabled));
On 2016/08/22 18:38:19, Devlin wrote:
> drive-by: To me, this seems a little weird.  I would just silently let it
> return.  But, you'll know best.

I requested this. The UI shouldn't generally do this and if it is, this could
help debug any problems.

Alternatively I guess we could return a boolean result instead, would that be
preferable?

[Devlin, 2016-08-22 19:01:00]

https://codereview.chromium.org/2261903002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc
(right):

https://codereview.chromium.org/2261903002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:276:
return RespondNow(Error(kErrorLanguageAlreadyEnabled));
On 2016/08/22 18:41:30, stevenjb wrote:
> On 2016/08/22 18:38:19, Devlin wrote:
> > drive-by: To me, this seems a little weird.  I would just silently let it
> > return.  But, you'll know best.
> 
> I requested this. The UI shouldn't generally do this and if it is, this could
> help debug any problems.
> 
> Alternatively I guess we could return a boolean result instead, would that be
> preferable?

Given this is a private API, I don't care that much. :)  Whatever y'all want. 
It just struck me as a little unusual.

[michaelpg, 2016-08-22 19:06:28]

On 2016/08/22 19:01:00, Devlin wrote:
>
https://codereview.chromium.org/2261903002/diff/40001/chrome/browser/extensio...
> File
>
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc
> (right):
> 
>
https://codereview.chromium.org/2261903002/diff/40001/chrome/browser/extensio...
>
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:276:
> return RespondNow(Error(kErrorLanguageAlreadyEnabled));
> On 2016/08/22 18:41:30, stevenjb wrote:
> > On 2016/08/22 18:38:19, Devlin wrote:
> > > drive-by: To me, this seems a little weird.  I would just silently let it
> > > return.  But, you'll know best.
> > 
> > I requested this. The UI shouldn't generally do this and if it is, this
could
> > help debug any problems.
> > 
> > Alternatively I guess we could return a boolean result instead, would that
be
> > preferable?
> 
> Given this is a private API, I don't care that much. :)  Whatever y'all want. 
> It just struck me as a little unusual.

It's unusual to me too, but I think it would be better if more APIs did this
instead of silently returning early. that said, we couldn't do anything with
this error if we wanted to, can we? Would we have to add a callback to the API
methods' signatures, or is there another way to detect errors on the JS side?

[stevenjb, 2016-08-22 19:18:28]

On 2016/08/22 19:06:28, michaelpg wrote:
> On 2016/08/22 19:01:00, Devlin wrote:
> >
>
https://codereview.chromium.org/2261903002/diff/40001/chrome/browser/extensio...
> > File
> >
>
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2261903002/diff/40001/chrome/browser/extensio...
> >
>
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:276:
> > return RespondNow(Error(kErrorLanguageAlreadyEnabled));
> > On 2016/08/22 18:41:30, stevenjb wrote:
> > > On 2016/08/22 18:38:19, Devlin wrote:
> > > > drive-by: To me, this seems a little weird.  I would just silently let
it
> > > > return.  But, you'll know best.
> > > 
> > > I requested this. The UI shouldn't generally do this and if it is, this
> could
> > > help debug any problems.
> > > 
> > > Alternatively I guess we could return a boolean result instead, would that
> be
> > > preferable?
> > 
> > Given this is a private API, I don't care that much. :)  Whatever y'all
want. 
> > It just struck me as a little unusual.
> 
> It's unusual to me too, but I think it would be better if more APIs did this
> instead of silently returning early. that said, we couldn't do anything with
> this error if we wanted to, can we? Would we have to add a callback to the API
> methods' signatures, or is there another way to detect errors on the JS side?

The error will be assigned to chrome.runtime.lastError.

[michaelpg, 2016-08-22 19:28:10]

On 2016/08/22 19:18:28, stevenjb wrote:
> On 2016/08/22 19:06:28, michaelpg wrote:
> > On 2016/08/22 19:01:00, Devlin wrote:
> > >
> >
>
https://codereview.chromium.org/2261903002/diff/40001/chrome/browser/extensio...
> > > File
> > >
> >
>
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2261903002/diff/40001/chrome/browser/extensio...
> > >
> >
>
chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:276:
> > > return RespondNow(Error(kErrorLanguageAlreadyEnabled));
> > > On 2016/08/22 18:41:30, stevenjb wrote:
> > > > On 2016/08/22 18:38:19, Devlin wrote:
> > > > > drive-by: To me, this seems a little weird.  I would just silently let
> it
> > > > > return.  But, you'll know best.
> > > > 
> > > > I requested this. The UI shouldn't generally do this and if it is, this
> > could
> > > > help debug any problems.
> > > > 
> > > > Alternatively I guess we could return a boolean result instead, would
that
> > be
> > > > preferable?
> > > 
> > > Given this is a private API, I don't care that much. :)  Whatever y'all
> want. 
> > > It just struck me as a little unusual.
> > 
> > It's unusual to me too, but I think it would be better if more APIs did this
> > instead of silently returning early. that said, we couldn't do anything with
> > this error if we wanted to, can we? Would we have to add a callback to the
API
> > methods' signatures, or is there another way to detect errors on the JS
side?
> 
> The error will be assigned to chrome.runtime.lastError.

it needs to be checked asynchronous, though, or it gets thrown away:

\> chrome.languageSettingsPrivate.enableLanguage('en-US');
console.log(chrome.runtime.lastError);
undefined

Unchecked runtime.lastError while running
languageSettingsPrivate.enableLanguage: Language already enabled

\> chrome.runtime.lastError
undefined

[stevenjb, 2016-08-22 19:35:22]

Oh, right, yes, it would need a void callback (which I guess could also
have been used to fix this instead of enable/disable, but I think this is a
better api anyway). So yeah, we should either add a callback or just use
LOG(ERROR) instead.


On Mon, Aug 22, 2016 at 1:28 PM, <michaelpg@chromium.org> wrote:

> On 2016/08/22 19:18:28, stevenjb wrote:
> > On 2016/08/22 19:06:28, michaelpg wrote:
> > > On 2016/08/22 19:01:00, Devlin wrote:
> > > >
> > >
> >
> https://codereview.chromium.org/2261903002/diff/40001/
> chrome/browser/extensions/api/language_settings_private/
> language_settings_private_api.cc
> > > > File
> > > >
> > >
> >
> chrome/browser/extensions/api/language_settings_private/
> language_settings_private_api.cc
> > > > (right):
> > > >
> > > >
> > >
> >
> https://codereview.chromium.org/2261903002/diff/40001/
> chrome/browser/extensions/api/language_settings_private/
> language_settings_private_api.cc#newcode276
> > > >
> > >
> >
> chrome/browser/extensions/api/language_settings_private/
> language_settings_private_api.cc:276:
> > > > return RespondNow(Error(kErrorLanguageAlreadyEnabled));
> > > > On 2016/08/22 18:41:30, stevenjb wrote:
> > > > > On 2016/08/22 18:38:19, Devlin wrote:
> > > > > > drive-by: To me, this seems a little weird. I would just
> silently let
> > it
> > > > > > return. But, you'll know best.
> > > > >
> > > > > I requested this. The UI shouldn't generally do this and if it is,
> this
> > > could
> > > > > help debug any problems.
> > > > >
> > > > > Alternatively I guess we could return a boolean result instead,
> would
> that
> > > be
> > > > > preferable?
> > > >
> > > > Given this is a private API, I don't care that much. :) Whatever
> y'all
> > want.
> > > > It just struck me as a little unusual.
> > >
> > > It's unusual to me too, but I think it would be better if more APIs
> did this
> > > instead of silently returning early. that said, we couldn't do
> anything with
> > > this error if we wanted to, can we? Would we have to add a callback to
> the
> API
> > > methods' signatures, or is there another way to detect errors on the JS
> side?
> >
> > The error will be assigned to chrome.runtime.lastError.
>
> it needs to be checked asynchronous, though, or it gets thrown away:
>
> \> chrome.languageSettingsPrivate.enableLanguage('en-US');
> console.log(chrome.runtime.lastError);
> undefined
>
> Unchecked runtime.lastError while running
> languageSettingsPrivate.enableLanguage: Language already enabled
>
> \> chrome.runtime.lastError
> undefined
>
> https://codereview.chromium.org/2261903002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[michaelpg, 2016-08-22 20:35:08]

Patchset 4 uses logging instead of error callbacks.

[stevenjb, 2016-08-22 20:37:34]

lgtm++

[Ilya Sherman, 2016-08-22 20:54:05]

https://codereview.chromium.org/2261903002/diff/60001/extensions/browser/exte...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/2261903002/diff/60001/extensions/browser/exte...
extensions/browser/extension_function_histogram_value.h:1209: // python
tools/metrics/histograms/update_extension_histograms.py
^^^ Please run the script to update histograms.xml

[michaelpg, 2016-08-22 22:36:15]

https://codereview.chromium.org/2261903002/diff/60001/extensions/browser/exte...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/2261903002/diff/60001/extensions/browser/exte...
extensions/browser/extension_function_histogram_value.h:1209: // python
tools/metrics/histograms/update_extension_histograms.py
On 2016/08/22 20:54:05, Ilya Sherman wrote:
> ^^^ Please run the script to update histograms.xml

Rgh. Done. I don't know why I keep losing those changes on CLs like this. I
remember running this and even verified that it's in my scrollback. So weird.
Anyway, done, thanks.

[Ilya Sherman, 2016-08-22 22:58:55]

histograms.xml lgtm, thanks

[michaelpg, 2016-08-23 00:01:12]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2016-08-23 00:01:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2261903002/#ps80001
(title: "update histograms")

[commit-bot: I haz the power, 2016-08-23 00:02:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 01:50:43]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-08-23 01:50:45]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-23 01:53:43]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/6de7389ad5a4245ea4d3a8c4aa7e9584725511a7
Cr-Commit-Position: refs/heads/master@{#413624}
==========

[commit-bot: I haz the power, 2016-08-23 01:53:44]

Patchset 5 (id:??) landed as
https://crrev.com/6de7389ad5a4245ea4d3a8c4aa7e9584725511a7
Cr-Commit-Position: refs/heads/master@{#413624}