Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2699403003:
Avoid touching fonts in ResourceBundle while reloading locale (Closed)
DescriptionAvoid touching fonts in ResourceBundle while reloading locale
gfx::Font is not thread safe due to non-thread-safe ref counts in
gfx::PlatformFont and gfx::FontListImpl, and ui::ResouceBundle doesn't
protect them from data race. So, we should not touch them only on UI
thread to avoid the race on the ref count manipulation.
BUG=688072, 468010
Patch Set 1 #
Total comments: 2
Patch Set 2 : +comment. rebase. #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 22 (15 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Avoid touching fonts in ResourceBundle while reloading locale BUG= ========== to ========== Avoid touching fonts in ResourceBundle while reloading locale gfx::Font is not thread safe due to non-thread-safe ref counts in gfx::PlatformFont and gfx::FontListImpl, and ui::ResouceBundle doesn't protect them from data race. So, we should not touch them only on UI thread to avoid the race on the ref count manipulation. BUG=688072 ==========
tzik@chromium.org changed reviewers: + oshima@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Avoid touching fonts in ResourceBundle while reloading locale gfx::Font is not thread safe due to non-thread-safe ref counts in gfx::PlatformFont and gfx::FontListImpl, and ui::ResouceBundle doesn't protect them from data race. So, we should not touch them only on UI thread to avoid the race on the ref count manipulation. BUG=688072 ========== to ========== Avoid touching fonts in ResourceBundle while reloading locale gfx::Font is not thread safe due to non-thread-safe ref counts in gfx::PlatformFont and gfx::FontListImpl, and ui::ResouceBundle doesn't protect them from data race. So, we should not touch them only on UI thread to avoid the race on the ref count manipulation. BUG=688072, 468010 ==========
lgtm with a nit https://codereview.chromium.org/2699403003/diff/1/chrome/browser/chromeos/bas... File chrome/browser/chromeos/base/locale_util.cc (right): https://codereview.chromium.org/2699403003/diff/1/chrome/browser/chromeos/bas... chrome/browser/chromeos/base/locale_util.cc:94: Please add comment that this should be in UI thread due to cache cleanup
https://codereview.chromium.org/2699403003/diff/1/chrome/browser/chromeos/bas... File chrome/browser/chromeos/base/locale_util.cc (right): https://codereview.chromium.org/2699403003/diff/1/chrome/browser/chromeos/bas... chrome/browser/chromeos/base/locale_util.cc:94: On 2017/02/21 00:24:05, oshima wrote: > Please add comment that this should be in UI thread due to cache cleanup Done.
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2699403003/#ps40001 (title: "+comment. rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487651153112920,
"parent_rev": "8ee1cf1bafcb0d0fa2b2e5a7a460f62ed06c1b05", "commit_rev":
"3b2b2e23c656e16592fb6e2d4c75b5e4c353d061"}
The CQ bit was unchecked by commit-bot@chromium.org
Prior attempt to commit was detected, but we were not able to check whether the issue was successfully committed. Please check Git history manually and re-check CQ or close this issue as needed.
The CQ bit was checked by tzik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by tzik@chromium.org
Message was sent while issue was closed.
sky@chromium.org changed reviewers: + sky@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2699403003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/base/locale_util.cc (right): https://codereview.chromium.org/2699403003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/base/locale_util.cc:97: ResourceBundle::GetSharedInstance().ReloadFonts(); Could you make ReloadFonts DCHECK it's on the main thread? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
