Investigate making JumpList not refcounted |
|||
Issue descriptionOwned by BrowserView, JumpList is RefCounted but there doesn't appear to be a good reason for that (it has a single owner). We should investigate making it not refcounted.
,
Jun 12 2017
,
Jun 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4a5397999a0f493c47632e1a8bd93686de54adbf commit 4a5397999a0f493c47632e1a8bd93686de54adbf Author: chengx <chengx@chromium.org> Date: Tue Jun 13 19:24:57 2017 Fix stability and data racing issues, coalesce more updates for JumpList This CL contains the following five main changes plus lots of updates on variable/method names and comments. 1. Fix the JumpList data-racing issue by separating different thread tasks. There are two threads involved in updating the JumpList. The UI thread lives the module to collect JumpList data for update, and a non-UI thread lives the RunUpdateJumpList module which updates the JumpList and notifies the shell. Previously, the two modules were interleaved to some extent, which required the JumpList data to be shared between the two threads. This caused the data racing issue and was solved by introducing a lock. The interleaving of these two modules complicates the code and makes it very difficult to add unit tests. This CL separates these two modules. In more details, the JumpList data is now only accessible to the UI thread. Local copies of the data are made and sent to the non-UI thread to run the update task. Once that update task finishes on the non-UI thread, the update results are transferred to the UI thread via PostTaskAndReply. The UI thread then decides how to update the JumpList data according to the update results. 2. Fix the JumpList stability issue by making the RunUpdateJumpList API static so it outlives the JumpList instance. When a JumpList::RunUpdateJumpList task is posted on a non-UI thread and about to run, the JumpList instance may have already been destructed. This caused stability issues as the RunUpdateJumpList API was non-static. This CL fixes it by making this API and other APIs it calls static. 3. Coalesce JumpList updates from two services by keeping only one timer. Previously, there were two timers to coalesce updates from the TopSites service and the TabRestore service, respectively. When notifications from both services came, even at the same time, multiple RunUpdateJumpList tasks were posted where they should be coalesced into one. This caused a waste of many shell notification runs. Note that there was no wasted work on icon operations as different JumpList categories were processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates triggered by different services. 4. Cancel fetching most-visited URLs when Jumplist is destroyed. This CL makes it possible to cancel the tasks of fetching most-visited URLs when JumpList is destroyed, which otherwise is wasted. This also avoids unnecessary favicon loading triggered upon the completion of the URL fetch. 5. Make JumpList not refcounted. JumpList is owned by BrowserView. It is RefCounted but there doesn't appear to be a reason for that. This CL changes JumpList to be not refcounted by making it a regular keyed service, rather than a refcounted keyed service as it was. BUG= 40407 , 179576 , 498987 , 717236 , 720028 , 732586 Review-Url: https://codereview.chromium.org/2931573003 Cr-Commit-Position: refs/heads/master@{#479095} [modify] https://crrev.com/4a5397999a0f493c47632e1a8bd93686de54adbf/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/4a5397999a0f493c47632e1a8bd93686de54adbf/chrome/browser/ui/views/frame/browser_view.h [modify] https://crrev.com/4a5397999a0f493c47632e1a8bd93686de54adbf/chrome/browser/win/jumplist.cc [modify] https://crrev.com/4a5397999a0f493c47632e1a8bd93686de54adbf/chrome/browser/win/jumplist.h [modify] https://crrev.com/4a5397999a0f493c47632e1a8bd93686de54adbf/chrome/browser/win/jumplist_factory.cc [modify] https://crrev.com/4a5397999a0f493c47632e1a8bd93686de54adbf/chrome/browser/win/jumplist_factory.h
,
Jun 13 2017
,
Jun 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/184a0e32ca40eac7c5f046aa840b31845e06afc0 commit 184a0e32ca40eac7c5f046aa840b31845e06afc0 Author: chengx <chengx@chromium.org> Date: Wed Jun 14 17:28:17 2017 Revert of Fix stability and data racing issues, coalesce more updates for JumpList (patchset #6 id:420001 of https://codereview.chromium.org/2931573003/ ) Reason for revert: Caused crash as recorded in crbug/733319 and crbug/733098 Original issue's description: > Fix stability and data racing issues, coalesce more updates for JumpList > > This CL contains the following five main changes plus lots of updates > on variable/method names and comments. > > 1. Fix the JumpList data-racing issue by separating different thread > tasks. > > There are two threads involved in updating the JumpList. The UI thread > lives the module to collect JumpList data for update, and a non-UI > thread lives the RunUpdateJumpList module which updates the JumpList > and notifies the shell. Previously, the two modules were interleaved > to some extent, which required the JumpList data to be shared between > the two threads. This caused the data racing issue and was solved by > introducing a lock. The interleaving of these two modules complicates > the code and makes it very difficult to add unit tests. This CL > separates these two modules. In more details, the JumpList data is now > only accessible to the UI thread. Local copies of the data are made > and sent to the non-UI thread to run the update task. Once that update > task finishes on the non-UI thread, the update results are transferred > to the UI thread via PostTaskAndReply. The UI thread then decides how > to update the JumpList data according to the update results. > > 2. Fix the JumpList stability issue by making the RunUpdateJumpList > API static so it outlives the JumpList instance. > > When a JumpList::RunUpdateJumpList task is posted on a non-UI thread > and about to run, the JumpList instance may have already been > destructed. This caused stability issues as the RunUpdateJumpList API > was non-static. This CL fixes it by making this API and other APIs it > calls static. > > 3. Coalesce JumpList updates from two services by keeping only one > timer. > > Previously, there were two timers to coalesce updates from the > TopSites service and the TabRestore service, respectively. When > notifications from both services came, even at the same time, multiple > RunUpdateJumpList tasks were posted where they should be coalesced > into one. This caused a waste of many shell notification runs. Note > that there was no wasted work on icon operations as different JumpList > categories were processed on demand. This CL trims out this waste by > keeping only one timer to coalesce updates triggered by different > services. > > 4. Cancel fetching most-visited URLs when Jumplist is destroyed. > > This CL makes it possible to cancel the tasks of fetching most-visited > URLs when JumpList is destroyed, which otherwise is wasted. This also > avoids unnecessary favicon loading triggered upon the completion of > the URL fetch. > > 5. Make JumpList not refcounted. > > JumpList is owned by BrowserView. It is RefCounted but there doesn't > appear to be a reason for that. This CL changes JumpList to be not > refcounted by making it a regular keyed service, rather than a > refcounted keyed service as it was. > > BUG= 40407 , 179576 , 498987 , 717236 , 720028 , 732586 > > Review-Url: https://codereview.chromium.org/2931573003 > Cr-Commit-Position: refs/heads/master@{#479095} > Committed: https://chromium.googlesource.com/chromium/src/+/4a5397999a0f493c47632e1a8bd93686de54adbf TBR=grt@chromium.org,pkasting@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 40407 , 179576 , 498987 , 717236 , 720028 , 732586 Review-Url: https://codereview.chromium.org/2937993002 Cr-Commit-Position: refs/heads/master@{#479434} [modify] https://crrev.com/184a0e32ca40eac7c5f046aa840b31845e06afc0/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/184a0e32ca40eac7c5f046aa840b31845e06afc0/chrome/browser/ui/views/frame/browser_view.h [modify] https://crrev.com/184a0e32ca40eac7c5f046aa840b31845e06afc0/chrome/browser/win/jumplist.cc [modify] https://crrev.com/184a0e32ca40eac7c5f046aa840b31845e06afc0/chrome/browser/win/jumplist.h [modify] https://crrev.com/184a0e32ca40eac7c5f046aa840b31845e06afc0/chrome/browser/win/jumplist_factory.cc [modify] https://crrev.com/184a0e32ca40eac7c5f046aa840b31845e06afc0/chrome/browser/win/jumplist_factory.h
,
Jun 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f1c120eb244a567ec94bec0e8f43e076963bf550 commit f1c120eb244a567ec94bec0e8f43e076963bf550 Author: chengx <chengx@chromium.org> Date: Thu Jun 15 21:36:36 2017 Fix stability and data racing issues, coalesce more updates for JumpList This CL contains the following five main changes plus lots of updates on variable/method names and comments. 1. Fix the JumpList data-racing issue by separating different thread tasks. There are two threads involved in updating the JumpList. The UI thread lives the module to collect JumpList data for update, and a non-UI thread lives the RunUpdateJumpList module which updates the JumpList and notifies the shell. Previously, the two modules were interleaved to some extent, which required the JumpList data to be shared between the two threads. This caused the data racing issue and was solved by introducing a lock. The interleaving of these two modules complicates the code and makes it very difficult to add unit tests. This CL separates these two modules. In more details, the JumpList data is now only accessible to the UI thread. Local copies of the data are made and sent to the non-UI thread to run the update task. Once that update task finishes on the non-UI thread, the update results are transferred to the UI thread via PostTaskAndReply. The UI thread then decides how to update the JumpList data according to the update results. 2. Fix the JumpList stability issue by making the RunUpdateJumpList API static so it outlives the JumpList instance. When a JumpList::RunUpdateJumpList task is posted on a non-UI thread and about to run, the JumpList instance may have already been destructed. This caused stability issues as the RunUpdateJumpList API was non-static. This CL fixes it by making this API and other APIs it calls static. 3. Coalesce JumpList updates from two services by keeping only one timer. Previously, there were two timers to coalesce updates from the TopSites service and the TabRestore service, respectively. When notifications from both services came, even at the same time, multiple RunUpdateJumpList tasks were posted where they should be coalesced into one. This caused a waste of many shell notification runs. Note that there was no wasted work on icon operations as different JumpList categories were processed on demand. This CL trims out this waste by keeping only one timer to coalesce updates triggered by different services. 4. Cancel fetching most-visited URLs when Jumplist is destroyed. This CL makes it possible to cancel the tasks of fetching most-visited URLs when JumpList is destroyed, which otherwise is wasted. This also avoids unnecessary favicon loading triggered upon the completion of the URL fetch. 5. Make JumpList not refcounted. JumpList is owned by BrowserView. It is RefCounted but there doesn't appear to be a reason for that. This CL changes JumpList to be not refcounted by making it a regular keyed service, rather than a refcounted keyed service as it was. BUG= 40407 , 179576 , 498987 , 717236 , 720028 , 732586 Review-Url: https://codereview.chromium.org/2931573003 Cr-Original-Commit-Position: refs/heads/master@{#479095} Committed: https://chromium.googlesource.com/chromium/src/+/4a5397999a0f493c47632e1a8bd93686de54adbf Review-Url: https://codereview.chromium.org/2931573003 Cr-Commit-Position: refs/heads/master@{#479848} [modify] https://crrev.com/f1c120eb244a567ec94bec0e8f43e076963bf550/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/f1c120eb244a567ec94bec0e8f43e076963bf550/chrome/browser/ui/views/frame/browser_view.h [modify] https://crrev.com/f1c120eb244a567ec94bec0e8f43e076963bf550/chrome/browser/win/jumplist.cc [modify] https://crrev.com/f1c120eb244a567ec94bec0e8f43e076963bf550/chrome/browser/win/jumplist.h [modify] https://crrev.com/f1c120eb244a567ec94bec0e8f43e076963bf550/chrome/browser/win/jumplist_factory.cc [modify] https://crrev.com/f1c120eb244a567ec94bec0e8f43e076963bf550/chrome/browser/win/jumplist_factory.h |
|||
►
Sign in to add a comment |
|||
Comment 1 by chengx@chromium.org
, Jun 12 2017