|
|
DescriptionAvoid touching ResourceBundle fonts from non-UI thread
SharedResourcesDataSource touches shared fonts in ResourceBundle from
IO thread. However, gfx::Font is not thread safe due to non-thread-safe
ref counts in gfx::PlatformFont and gfx::FontListImpl. That causes a
data race on the ref count.
So, we should touch fonts in ResourceBundle only from UI thread.
BUG=688072, 468010
Review-Url: https://codereview.chromium.org/2701183002
Cr-Commit-Position: refs/heads/master@{#452390}
Committed: https://chromium.googlesource.com/chromium/src/+/bd171b13a4e04434cf2ee4d3b79ce28f0aeddd0f
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : rebase #Patch Set 4 : rebase (swapped CL sequence order) #
Total comments: 2
Patch Set 5 : PostTaskAndReply -> TaskRunnerForRequestPath #
Total comments: 6
Patch Set 6 : . #
Total comments: 4
Patch Set 7 : s/IDR/Idr/ #Messages
Total messages: 44 (33 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== . ========== to ========== Avoid touching ResourceBundle fonts from non-UI thread SharedResourcesDataSource touches shared fonts in ResourceBundle from IO thread. However, gfx::Font is not thread safe due to non-thread-safe ref counts in gfx::PlatformFont and gfx::FontListImpl. That causes a data race on the ref count. So, we should touch fonts in ResourceBundle only from UI thread. ==========
tzik@chromium.org changed reviewers: + dbeam@chromium.org
Description was changed from ========== Avoid touching ResourceBundle fonts from non-UI thread SharedResourcesDataSource touches shared fonts in ResourceBundle from IO thread. However, gfx::Font is not thread safe due to non-thread-safe ref counts in gfx::PlatformFont and gfx::FontListImpl. That causes a data race on the ref count. So, we should touch fonts in ResourceBundle only from UI thread. ========== to ========== Avoid touching ResourceBundle fonts from non-UI thread SharedResourcesDataSource touches shared fonts in ResourceBundle from IO thread. However, gfx::Font is not thread safe due to non-thread-safe ref counts in gfx::PlatformFont and gfx::FontListImpl. That causes a data race on the ref count. So, we should touch fonts in ResourceBundle only from UI thread. BUG=688072 ==========
PTAL
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Avoid touching ResourceBundle fonts from non-UI thread SharedResourcesDataSource touches shared fonts in ResourceBundle from IO thread. However, gfx::Font is not thread safe due to non-thread-safe ref counts in gfx::PlatformFont and gfx::FontListImpl. That causes a data race on the ref count. So, we should touch fonts in ResourceBundle only from UI thread. BUG=688072 ========== to ========== Avoid touching ResourceBundle fonts from non-UI thread SharedResourcesDataSource touches shared fonts in ResourceBundle from IO thread. However, gfx::Font is not thread safe due to non-thread-safe ref counts in gfx::PlatformFont and gfx::FontListImpl. That causes a data race on the ref count. So, we should touch fonts in ResourceBundle only from UI thread. BUG=688072, 468010 ==========
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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...
Patchset #4 (id:60001) has been deleted
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping. dbeam: WDYT?
https://codereview.chromium.org/2701183002/diff/80001/content/browser/webui/s... File content/browser/webui/shared_resources_data_source.cc (right): https://codereview.chromium.org/2701183002/diff/80001/content/browser/webui/s... content/browser/webui/shared_resources_data_source.cc:158: return nullptr; i think you should only return nullptr when GetMime(path) != "application/font-woff2"
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...
https://codereview.chromium.org/2701183002/diff/80001/content/browser/webui/s... File content/browser/webui/shared_resources_data_source.cc (right): https://codereview.chromium.org/2701183002/diff/80001/content/browser/webui/s... content/browser/webui/shared_resources_data_source.cc:158: return nullptr; On 2017/02/22 22:56:10, Dan Beam wrote: > i think you should only return nullptr when GetMime(path) != > "application/font-woff2" Sounds nice! Updated. Let me use "text/css" since the problematic resource is a CSS.
https://codereview.chromium.org/2701183002/diff/100001/content/browser/webui/... File content/browser/webui/shared_resources_data_source.cc (right): https://codereview.chromium.org/2701183002/diff/100001/content/browser/webui/... content/browser/webui/shared_resources_data_source.cc:100: bytes = base::RefCountedString::TakeString(&css); these are the calls that actually ask for a gfx::Font(). just move these 2 calls (GetWebUiCssTextDefaults*()) on the UI thread. https://codereview.chromium.org/2701183002/diff/100001/content/browser/webui/... content/browser/webui/shared_resources_data_source.cc:155: return BrowserThread::GetTaskRunnerForThread(BrowserThread::UI); sorry, don't do this
https://codereview.chromium.org/2701183002/diff/100001/content/browser/webui/... File content/browser/webui/shared_resources_data_source.cc (right): https://codereview.chromium.org/2701183002/diff/100001/content/browser/webui/... content/browser/webui/shared_resources_data_source.cc:155: return BrowserThread::GetTaskRunnerForThread(BrowserThread::UI); On 2017/02/23 00:27:56, Dan Beam wrote: > sorry, don't do this and by that i mean, push all text/css to the UI thread. also, should we just call the super? ... SharedResourcesDataSource::TaskRunnerForRequestPath(...) const { const ResourcesMap& resources = GetResourcesMap(); auto it = resources.find(path); if (it != resources.end() && (it->second == IDR_WEBUI_CSS_TEXT_DEFAULTS || it->second == IDR_WEBUI_CSS_TEXT_DEFAULTS_MD)) { return URLDataSource::TaskRunnerForRequestPath(path); } return nullptr; }
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
https://codereview.chromium.org/2701183002/diff/100001/content/browser/webui/... File content/browser/webui/shared_resources_data_source.cc (right): https://codereview.chromium.org/2701183002/diff/100001/content/browser/webui/... content/browser/webui/shared_resources_data_source.cc:155: return BrowserThread::GetTaskRunnerForThread(BrowserThread::UI); On 2017/02/23 00:32:26, Dan Beam wrote: > On 2017/02/23 00:27:56, Dan Beam wrote: > > sorry, don't do this > > and by that i mean, push all text/css to the UI thread. > > also, should we just call the super? > > ... SharedResourcesDataSource::TaskRunnerForRequestPath(...) const { > const ResourcesMap& resources = GetResourcesMap(); > auto it = resources.find(path); > if (it != resources.end() && > (it->second == IDR_WEBUI_CSS_TEXT_DEFAULTS || > it->second == IDR_WEBUI_CSS_TEXT_DEFAULTS_MD)) { > return URLDataSource::TaskRunnerForRequestPath(path); > } > return nullptr; > } Updated. https://codereview.chromium.org/2701183002/diff/100001/content/browser/webui/... content/browser/webui/shared_resources_data_source.cc:155: return BrowserThread::GetTaskRunnerForThread(BrowserThread::UI); On 2017/02/23 00:32:26, Dan Beam wrote: > On 2017/02/23 00:27:56, Dan Beam wrote: > > sorry, don't do this > > and by that i mean, push all text/css to the UI thread. > > also, should we just call the super? > > ... SharedResourcesDataSource::TaskRunnerForRequestPath(...) const { > const ResourcesMap& resources = GetResourcesMap(); > auto it = resources.find(path); > if (it != resources.end() && > (it->second == IDR_WEBUI_CSS_TEXT_DEFAULTS || > it->second == IDR_WEBUI_CSS_TEXT_DEFAULTS_MD)) { > return URLDataSource::TaskRunnerForRequestPath(path); > } > return nullptr; > } Hmm, I think it's unclear from this code that the super class returns the UI thread task runner. Can we take it from BrowserThread?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2701183002/diff/100001/content/browser/webui/... File content/browser/webui/shared_resources_data_source.cc (right): https://codereview.chromium.org/2701183002/diff/100001/content/browser/webui/... content/browser/webui/shared_resources_data_source.cc:155: return BrowserThread::GetTaskRunnerForThread(BrowserThread::UI); On 2017/02/23 00:58:19, tzik wrote: > On 2017/02/23 00:32:26, Dan Beam wrote: > > On 2017/02/23 00:27:56, Dan Beam wrote: > > > sorry, don't do this > > > > and by that i mean, push all text/css to the UI thread. > > > > also, should we just call the super? > > > > ... SharedResourcesDataSource::TaskRunnerForRequestPath(...) const { > > const ResourcesMap& resources = GetResourcesMap(); > > auto it = resources.find(path); > > if (it != resources.end() && > > (it->second == IDR_WEBUI_CSS_TEXT_DEFAULTS || > > it->second == IDR_WEBUI_CSS_TEXT_DEFAULTS_MD)) { > > return URLDataSource::TaskRunnerForRequestPath(path); > > } > > return nullptr; > > } > > Hmm, I think it's unclear from this code that the super class returns the UI > thread task runner. Can we take it from BrowserThread? i'm fine with either https://codereview.chromium.org/2701183002/diff/120001/content/browser/webui/... File content/browser/webui/shared_resources_data_source.cc (right): https://codereview.chromium.org/2701183002/diff/120001/content/browser/webui/... content/browser/webui/shared_resources_data_source.cc:73: int GetIDRForPath(const std::string& path) { GetIdrForPath() https://codereview.chromium.org/2701183002/diff/120001/content/browser/webui/... content/browser/webui/shared_resources_data_source.cc:76: return (it != resources_map.end()) ? it->second : -1; nit: return it != resources_map.end() ? it->second : -1;
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2701183002/diff/120001/content/browser/webui/... File content/browser/webui/shared_resources_data_source.cc (right): https://codereview.chromium.org/2701183002/diff/120001/content/browser/webui/... content/browser/webui/shared_resources_data_source.cc:73: int GetIDRForPath(const std::string& path) { On 2017/02/23 02:11:30, Dan Beam wrote: > GetIdrForPath() Done. https://codereview.chromium.org/2701183002/diff/120001/content/browser/webui/... content/browser/webui/shared_resources_data_source.cc:76: return (it != resources_map.end()) ? it->second : -1; On 2017/02/23 02:11:30, Dan Beam wrote: > nit: return it != resources_map.end() ? it->second : -1; Done.
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2701183002/#ps140001 (title: "s/IDR/Idr/")
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": 140001, "attempt_start_ts": 1487820706698220, "parent_rev": "ec5f9b7821faea7d2083376a934e306e0763572e", "commit_rev": "bd171b13a4e04434cf2ee4d3b79ce28f0aeddd0f"}
Message was sent while issue was closed.
Description was changed from ========== Avoid touching ResourceBundle fonts from non-UI thread SharedResourcesDataSource touches shared fonts in ResourceBundle from IO thread. However, gfx::Font is not thread safe due to non-thread-safe ref counts in gfx::PlatformFont and gfx::FontListImpl. That causes a data race on the ref count. So, we should touch fonts in ResourceBundle only from UI thread. BUG=688072, 468010 ========== to ========== Avoid touching ResourceBundle fonts from non-UI thread SharedResourcesDataSource touches shared fonts in ResourceBundle from IO thread. However, gfx::Font is not thread safe due to non-thread-safe ref counts in gfx::PlatformFont and gfx::FontListImpl. That causes a data race on the ref count. So, we should touch fonts in ResourceBundle only from UI thread. BUG=688072, 468010 Review-Url: https://codereview.chromium.org/2701183002 Cr-Commit-Position: refs/heads/master@{#452390} Committed: https://chromium.googlesource.com/chromium/src/+/bd171b13a4e04434cf2ee4d3b79c... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/chromium/src/+/bd171b13a4e04434cf2ee4d3b79c... |