1421200 - Fix memory leakage for WebRenderUserData

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Ethan Lin[:ethlin]]

There is a memory leakage problem in WebRenderUserData. It blocks p2 bug, so I should fix it.

Comment 1

[Ethan Lin[:ethlin]]

Attachment: Bug 1421200 - Fix memory leakage problem when getting WebRenderUserData.

Review commit: https://reviewboard.mozilla.org/r/203450/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203450/

Comment 2

[Ethan Lin[:ethlin]]

Comment on attachment 8932387 [details]
Bug 1421200 - Fix memory leakage problem when getting WebRenderUserData.

https://reviewboard.mozilla.org/r/203450/#review208936

::: gfx/layers/wr/WebRenderCommandBuilder.h:161
(Diff revision 1)
>      if (!userDataTable) {
> -      return result.forget();
> +      return nullptr;
>      }
>  
> -    WebRenderUserData* data = nullptr;
> -    if (userDataTable->Get(aPerFrameKey, &data)) {
> +    RefPtr<WebRenderUserData> data = userDataTable->Get(aPerFrameKey);
> +    if (!data || !(data->GetType() == T::Type()) || !data->IsDataValid(mManager)) {

The hash table is a nsRefPtrHashtable. The output data of the function 'Get' is already_addrefed[1].

[1] https://dxr.mozilla.org/mozilla-central/rev/cad9c9573579698c223b4b6cb53ca723cd930ad2/xpcom/ds/nsRefPtrHashtable.h#42

Comment 3

[Kartikaya Gupta (email:[e-mail])]

(In reply to Ethan Lin[:ethlin] from comment #2)
> 
> The hash table is a nsRefPtrHashtable. The output data of the function 'Get'
> is already_addrefed[1].

Good catch!

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8932387 [details]
Bug 1421200 - Fix memory leakage problem when getting WebRenderUserData.

https://reviewboard.mozilla.org/r/203450/#review208948

::: gfx/layers/wr/WebRenderCommandBuilder.h:161
(Diff revision 1)
> -    if (userDataTable->Get(aPerFrameKey, &data)) {
> -      if (data->GetType() == T::Type() && data->IsDataValid(mManager)) {
> +    if (!data || !(data->GetType() == T::Type()) || !data->IsDataValid(mManager)) {
> +      return nullptr;
> -        result = static_cast<T*>(data);
> -      }
>      }

I'd slightly prefer if you inverted the condition, so that it's not all negated:

if (data && data->GetType() == T::Type() && data->IsValid(mManager)) {
  RefPtr<T> result = static_cast<T*>(data.get());
  return result.forget();
}

return nullptr;

I find conditions with a lot of ! harder to read.

Comment 5

[Ethan Lin[:ethlin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Comment on attachment 8932387 [details]
> Bug 1421200 - Fix memory leakage problem when getting WebRenderUserData.
> 
> https://reviewboard.mozilla.org/r/203450/#review208948
> 
> ::: gfx/layers/wr/WebRenderCommandBuilder.h:161
> (Diff revision 1)
> > -    if (userDataTable->Get(aPerFrameKey, &data)) {
> > -      if (data->GetType() == T::Type() && data->IsDataValid(mManager)) {
> > +    if (!data || !(data->GetType() == T::Type()) || !data->IsDataValid(mManager)) {
> > +      return nullptr;
> > -        result = static_cast<T*>(data);
> > -      }
> >      }
> 
> I'd slightly prefer if you inverted the condition, so that it's not all
> negated:
> 
> if (data && data->GetType() == T::Type() && data->IsValid(mManager)) {
>   RefPtr<T> result = static_cast<T*>(data.get());
>   return result.forget();
> }
> 
> return nullptr;
> 
> I find conditions with a lot of ! harder to read.

Okay, I don't like inverse conditions neither!

Comment 6

[Ethan Lin[:ethlin]]

Comment on attachment 8932387 [details]
Bug 1421200 - Fix memory leakage problem when getting WebRenderUserData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203450/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29dcfd9a7ce8
Fix memory leakage problem when getting WebRenderUserData. r=kats

Comment 8

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/29dcfd9a7ce8