Fix threading/race condition issues with LocalNtpSource |
||
Issue descriptionCurrently, LocalNtpSource lives partially on the UI thread and partially on the IO thread, which is generally Not A Good Thing. Given the threading model imposed by content::URLDataSource, that probably can't be avoided completely, but LocalNtpSource has more specific problems. In particular, "config.js" is dynamically generated no less than three times during an NTP load: Once to generate the checksum for the CSP (on the IO thread), once to generate the checksum that goes into local_ntp.html (on the UI thread), and once for actually serving its contents (on the UI thread). If e.g. the default search provider changes in the meantime, then things are inconsistent and stuff breaks. One consequence is flaky tests, e.g. bug 791969 , but this likely has real impact too. Idea for fixing this: Load config.js dynamically (i.e. create a script tag from local_ntp.js). Then thanks to the 'strict-dynamic' CSP, no checksum for config.js will be required (neither in the html nor in the CSP). Profit! Potential downsides: It'll make the overall loading process a bit more complicated. This might have a perf impact, though it should be small. It might also make the setup for tests a bit more tricky, though I think that should work out.
,
Dec 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/19b74198ef8cb9c9fa13171b9881df40bf7e79ad commit 19b74198ef8cb9c9fa13171b9881df40bf7e79ad Author: Marc Treib <treib@chromium.org> Date: Fri Dec 08 11:33:39 2017 LocalNtpSource: move code that runs on the IO thread into non-member functions This makes it impossible to accidentally use member variables that live on the UI thread. Bug: 792490 Change-Id: Ie17510d6a57c445fc40ddf0c5f358323797cd4bf Reviewed-on: https://chromium-review.googlesource.com/814515 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Chris Pickel <sfiera@chromium.org> Cr-Commit-Position: refs/heads/master@{#522762} [modify] https://crrev.com/19b74198ef8cb9c9fa13171b9881df40bf7e79ad/chrome/browser/search/local_ntp_source.cc
,
Dec 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/06a334799815e68ff66ad09e40175d01e2955310 commit 06a334799815e68ff66ad09e40175d01e2955310 Author: Marc Treib <treib@chromium.org> Date: Fri Dec 08 13:36:46 2017 LocalNtpSource: Remove off-UI-thread member access While I haven't seen evidence of any actual problems caused by this, let's not leave threading issues around. Bug: 792490 Change-Id: I7727064c28de359a95edde75ffb1901584ed0631 Reviewed-on: https://chromium-review.googlesource.com/817115 Reviewed-by: Chris Pickel <sfiera@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#522774} [modify] https://crrev.com/06a334799815e68ff66ad09e40175d01e2955310/chrome/browser/search/local_ntp_source.cc
,
Dec 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/15d802093c0807ecffc1ada5534bc2db6202fe18 commit 15d802093c0807ecffc1ada5534bc2db6202fe18 Author: Marc Treib <treib@chromium.org> Date: Fri Dec 08 16:38:47 2017 LocalNtpSource: Add comment about thread model NOTRY=true Bug: 792490 Change-Id: Icdfb83175955a4578cbcee4697552144ef3e5259 Reviewed-on: https://chromium-review.googlesource.com/817774 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Chris Pickel <sfiera@chromium.org> Cr-Commit-Position: refs/heads/master@{#522793} [modify] https://crrev.com/15d802093c0807ecffc1ada5534bc2db6202fe18/chrome/browser/search/local_ntp_source.h
,
Dec 8 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by bugdroid1@chromium.org
, Dec 7 2017