Fix memory leak in ActiveTabPermissionManager.

Fix memory leak in ActiveTabPermissionManager.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149665

[not at google - send to devlin, 2012-08-02 17:15:09]

Hey Aaron, quick review? Last patch had a memory leak.

[Aaron Boodman, 2012-08-02 17:21:33]

LGTM

On Thu, Aug 2, 2012 at 7:15 PM,  <kalman@chromium.org> wrote:
> Reviewers: Aaron Boodman,
>
> Message:
> Hey Aaron, quick review? Last patch had a memory leak.
>
> Description:
> Fix memory leak in ActiveTabPermissionManager.
>
>
> Please review this at https://chromiumcodereview.appspot.com/10824149/
>
> SVN Base: http://git.chromium.org/chromium/src.git@master
>
> Affected files:
>   M chrome/browser/extensions/active_tab_permission_manager.cc
>   M chrome/common/extensions/extension.h
>   M chrome/common/extensions/extension.cc
>
>
> Index: chrome/browser/extensions/active_tab_permission_manager.cc
> diff --git a/chrome/browser/extensions/active_tab_permission_manager.cc
> b/chrome/browser/extensions/active_tab_permission_manager.cc
> index
>
67d6d58243137341ec85b9ce06cc5e456d053dbd..6b6fd6c43a1833addfdee89ed26517a527663d75
> 100644
> --- a/chrome/browser/extensions/active_tab_permission_manager.cc
> +++ b/chrome/browser/extensions/active_tab_permission_manager.cc
> @@ -53,9 +53,10 @@ void ActiveTabPermissionManager::GrantIfRequested(const
> Extension* extension) {
>    new_apis.insert(APIPermission::kTab);
>    URLPatternSet new_hosts;
>    new_hosts.AddPattern(pattern);
> +  scoped_refptr<const PermissionSet> new_permissions =
> +      new PermissionSet(new_apis, new_hosts, URLPatternSet());
>
> -  extension->UpdateTabSpecificPermissions(
> -      tab_id_, new PermissionSet(new_apis, new_hosts, URLPatternSet()));
> +  extension->UpdateTabSpecificPermissions(tab_id_, new_permissions);
>    granted_extensions_.Insert(extension);
>    Send(new ExtensionMsg_UpdateTabSpecificPermissions(GetPageID(),
>                                                       tab_id_,
> Index: chrome/common/extensions/extension.cc
> diff --git a/chrome/common/extensions/extension.cc
> b/chrome/common/extensions/extension.cc
> index
>
231445b197bf1bde9d68646ec0520334b59e83d7..712e65cc20d73c0c638012e57bae432918ef9b1f
> 100644
> --- a/chrome/common/extensions/extension.cc
> +++ b/chrome/common/extensions/extension.cc
> @@ -3829,7 +3829,7 @@ scoped_refptr<const PermissionSet>
> Extension::GetTabSpecificPermissions(
>
>  void Extension::UpdateTabSpecificPermissions(
>      int tab_id,
> -    const PermissionSet* permissions) const {
> +    scoped_refptr<const PermissionSet> permissions) const {
>    base::AutoLock auto_lock(runtime_data_lock_);
>    runtime_data_.UpdateTabSpecificPermissions(tab_id, permissions);
>  }
> @@ -3887,12 +3887,12 @@ scoped_refptr<const PermissionSet>
>
>  void Extension::RuntimeData::UpdateTabSpecificPermissions(
>      int tab_id,
> -    const PermissionSet* permissions) {
> +    scoped_refptr<const PermissionSet> permissions) {
>    CHECK_GE(tab_id, 0);
>    if (tab_specific_permissions_.count(tab_id)) {
>      tab_specific_permissions_[tab_id] = PermissionSet::CreateUnion(
>          tab_specific_permissions_[tab_id],
> -        permissions);
> +        permissions.get());
>    } else {
>      tab_specific_permissions_[tab_id] = permissions;
>    }
> Index: chrome/common/extensions/extension.h
> diff --git a/chrome/common/extensions/extension.h
> b/chrome/common/extensions/extension.h
> index
>
bb1b43413f27cff2b59fd7a0eee9b86cf9c6a87b..d3c3bcb0d21233cd5ff804ed17c3f7d91a7a4993
> 100644
> --- a/chrome/common/extensions/extension.h
> +++ b/chrome/common/extensions/extension.h
> @@ -571,8 +571,9 @@ class Extension : public
> base::RefCountedThreadSafe<Extension> {
>
>    // Updates the tab-specific permissions of |tab_id| to include those from
>    // |permissions|.
> -  void UpdateTabSpecificPermissions(int tab_id,
> -                                    const PermissionSet* permissions)
> const;
> +  void UpdateTabSpecificPermissions(
> +      int tab_id,
> +      scoped_refptr<const PermissionSet> permissions) const;
>
>    // Clears the tab-specific permissions of |tab_id|.
>    void ClearTabSpecificPermissions(int tab_id) const;
> @@ -728,8 +729,9 @@ class Extension : public
> base::RefCountedThreadSafe<Extension> {
>
>      scoped_refptr<const PermissionSet> GetTabSpecificPermissions(int
> tab_id)
>          const;
> -    void UpdateTabSpecificPermissions(int tab_id,
> -                                      const PermissionSet* permissions);
> +    void UpdateTabSpecificPermissions(
> +        int tab_id,
> +        scoped_refptr<const PermissionSet> permissions);
>      void ClearTabSpecificPermissions(int tab_id);
>
>     private:
>
>

[commit-bot: I haz the power, 2012-08-02 17:21:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10824149/1

[commit-bot: I haz the power, 2012-08-02 19:02:39]

Change committed as 149665

[Lei Zhang, 2012-08-02 22:19:04]

On 2012/08/02 17:15:09, kalman wrote:
> Hey Aaron, quick review? Last patch had a memory leak.

Thanks for fixing this. The memory.fyi waterfall is mostly green again.