Leaking CGBitmapContext in SkFontHost_mac

Leaking CGBitmapContext in SkFontHost_mac

third_party/skia/src/ports/SkFontHost_mac.cpp: don't increase retain count of
newly assigned CFTypeRef in AutoCFRelease::reset(). Each time CFTypeRef is
assigned to AutoCFRelease, AutoCFRelease becomes its sole owner.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/e9d2d09ea53d20828d2f5320fc1f19a9952f2b31

[Lukasz Jagielski, 2014-07-14 12:36:37]

Hi,
PTAL at my change proposition.
Regards,
Łukasz

[reed1, 2014-07-14 14:45:47]

Good catch. How about the following:

    void reset(CFRef newCFRef = NULL) {
        if (fCFRef != newCFRef) {
            CFSafeRelease(fCFRef);
            fCFRef = that;
        }
    }

This guards against self-asignment, which would be bad since we would unref
twice (eventually).

I think we should do the same guard in related classes as well, but not part of
this CL.

[Lukasz Jagielski, 2014-07-14 14:47:36]

On 2014/07/14 14:45:47, reed1 wrote:
> Good catch. How about the following:
> 
>     void reset(CFRef newCFRef = NULL) {
>         if (fCFRef != newCFRef) {
>             CFSafeRelease(fCFRef);
>             fCFRef = that;
>         }
>     }
> 
> This guards against self-asignment, which would be bad since we would unref
> twice (eventually).
> 
> I think we should do the same guard in related classes as well, but not part
of
> this CL.

Yeah, sure. I'll prepare the fixup.

[bungeman-skia, 2014-07-14 17:38:30]

lgtm, and I'll follow up.

https://codereview.chromium.org/393513003/diff/20001/src/ports/SkFontHost_mac...
File src/ports/SkFontHost_mac.cpp (right):

https://codereview.chromium.org/393513003/diff/20001/src/ports/SkFontHost_mac...
src/ports/SkFontHost_mac.cpp:91: AutoCFRelease& operator =(CFRef that) {
I think I actually wrote this, but now I hate this operator= . The issue is that
it is actually a 'move' operator=. If I had c++11 I would write this as
'AutoCFRelease& operator=(CFRef&& that)' so that way one would be able to
appropriately steal references. This is actually all it is used for, so this
proposed change does work. But it is dangerous because with this patch it still
works by accident; there is nothing preventing an lvalue from appearing on the
rhs. 'reset' on the other hand is an explicit rvalue refcount stealing
construct, and in the futre will be easier to convert to a move assignment
operator.

Since we don't have c++11 yet, I'm in favor of removing this method entirely and
replacing all of its uses with calls to 'reset'. However, to get things moving,
I'm going to lgtm this for the moment so it goes in, and then I'll follow up
with removing the operator= .

[bungeman-skia, 2014-07-14 17:39:06]

The CQ bit was checked by bungeman@google.com

[commit-bot: I haz the power, 2014-07-14 17:39:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-14 17:39:39]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[bungeman-skia, 2014-07-14 17:54:57]

On 2014/07/14 17:39:39, I haz the power (commit-bot) wrote:
> Commit queue rejected this change because it did not recognize the base URL.
> Please commit your change manually.

We're currently working on the combined interaction of the commit-bot, rietveld,
and chromium re-direct of the Skia git repository situation. If there is no
progress on this situation by the 17th, I'll land this manually. If this issue
hasn't been closed by the 18th, ping me and I'll feel bad for having forgotten
and belatedly get this in.

[rmistry, 2014-07-15 16:36:54]

The CQ bit was checked by rmistry@google.com

[sheyang, 2014-07-16 01:07:27]

The CQ bit was unchecked by sheyang@chromium.org

[sheyang, 2014-07-16 01:07:28]

The CQ bit was checked by sheyang@chromium.org

[commit-bot: I haz the power, 2014-07-16 01:07:40]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/ljagielski@opera.com/393513003/20001

[commit-bot: I haz the power, 2014-07-16 03:02:08]

Change committed as e9d2d09ea53d20828d2f5320fc1f19a9952f2b31