Mac a11y: Fix AXTextMarker leaks.

Mac a11y: Fix AXTextMarker leaks.

We have to rely on undocumented APIs for VoiceOver on Mac.

AXTextMarkerCreate() copies from the buffer it's provided, and doesn't
retain it.

AXTextMarkerRangeCreate() adds a retain count to the pair of
AXTextMarker it's provided, so CreatePositionFromTextMarker must do a
CFRelease.

BUG=735793
Review-Url: https://codereview.chromium.org/2949833006
Cr-Commit-Position: refs/heads/master@{#481463}
Committed: https://chromium.googlesource.com/chromium/src/+/1ed607555484664fd396c3422d02f5c40470b9a8

[tapted, 2017-06-22 04:55:21]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 04:55:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-06-22 04:58:14]

Description was changed from

==========
Fix AXTextMarker leaks.

We have to rely on undocumented APIs for VoiceOver on Mac.

AXTextMarkerCreate() copies from the buffer it's provided.

AXTextMarkerRangeCreate() adds a retain count to the pair of AXTextMarker it's
provided, so CreatePositionFromTextMarker must do a CFRelease.

BUG=735793
==========

to

==========
Mac a11y: Fix AXTextMarker leaks.

We have to rely on undocumented APIs for VoiceOver on Mac.

AXTextMarkerCreate() copies from the buffer it's provided.

AXTextMarkerRangeCreate() adds a retain count to the pair of
AXTextMarker it's provided, so CreatePositionFromTextMarker must do a
CFRelease.

BUG=735793
==========

[tapted, 2017-06-22 04:58:14]

tapted@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[tapted, 2017-06-22 04:58:39]

Description was changed from

==========
Mac a11y: Fix AXTextMarker leaks.

We have to rely on undocumented APIs for VoiceOver on Mac.

AXTextMarkerCreate() copies from the buffer it's provided.

AXTextMarkerRangeCreate() adds a retain count to the pair of
AXTextMarker it's provided, so CreatePositionFromTextMarker must do a
CFRelease.

BUG=735793
==========

to

==========
Mac a11y: Fix AXTextMarker leaks.

We have to rely on undocumented APIs for VoiceOver on Mac.

AXTextMarkerCreate() copies from the buffer it's provided, and doesn't
retain it.

AXTextMarkerRangeCreate() adds a retain count to the pair of
AXTextMarker it's provided, so CreatePositionFromTextMarker must do a
CFRelease.

BUG=735793
==========

[tapted, 2017-06-22 05:00:00]

Hi Dominic, please take a look

[dmazzoni, 2017-06-22 05:04:19]

lgtm

[commit-bot: I haz the power, 2017-06-22 05:15:24]

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

[commit-bot: I haz the power, 2017-06-22 05:15:25]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-06-22 06:27:10]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-06-22 06:27:20]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 06:30:20]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1498112830536790, "parent_rev":
"b4190af5c81b3dae3343d5c1b43f42454433f3a9", "commit_rev":
"fadf3accc11e59f223931c65add52b23a8795ff4"}

[commit-bot: I haz the power, 2017-06-22 06:30:35]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1498112830536790, "parent_rev":
"7be23cd3d8fc0d05f84c76768072f32f4e51cba0", "commit_rev":
"1ed607555484664fd396c3422d02f5c40470b9a8"}

[commit-bot: I haz the power, 2017-06-22 06:30:48]

Description was changed from

==========
Mac a11y: Fix AXTextMarker leaks.

We have to rely on undocumented APIs for VoiceOver on Mac.

AXTextMarkerCreate() copies from the buffer it's provided, and doesn't
retain it.

AXTextMarkerRangeCreate() adds a retain count to the pair of
AXTextMarker it's provided, so CreatePositionFromTextMarker must do a
CFRelease.

BUG=735793
==========

to

==========
Mac a11y: Fix AXTextMarker leaks.

We have to rely on undocumented APIs for VoiceOver on Mac.

AXTextMarkerCreate() copies from the buffer it's provided, and doesn't
retain it.

AXTextMarkerRangeCreate() adds a retain count to the pair of
AXTextMarker it's provided, so CreatePositionFromTextMarker must do a
CFRelease.

BUG=735793

Review-Url: https://codereview.chromium.org/2949833006
Cr-Commit-Position: refs/heads/master@{#481463}
Committed:
https://chromium.googlesource.com/chromium/src/+/1ed607555484664fd396c3422d02...
==========

[commit-bot: I haz the power, 2017-06-22 06:30:50]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/1ed607555484664fd396c3422d02...

[chromium-reviews, 2017-06-22 15:55:31]

+// AXTextMarkerCreate copies from data buffer given to it.
> id CreateTextMarker(AXPlatformPositionInstance position) {
> AXTextMarkerRef text_marker = AXTextMarkerCreate(
> - kCFAllocatorDefault, reinterpret_cast<const UInt8*>(position.release()),
> + kCFAllocatorDefault, reinterpret_cast<const UInt8*>(position.get()),

What's the difference? Doesn't |Release| return a pointer to the buffer 
with the text marker same as |Get|?
Reason I used Release instead of Get is to make ownership transfer 
explicit so that no code that might later be added to the function will 
be tempted to use the unique_ptr again.

> sizeof(AXPlatformPosition));
> return static_cast<id>(
> base::mac::CFTypeRefToNSObjectAutorelease(text_marker));
> }
>
> -// |range| is destructed at the end of this method and ownership of 
> its |anchor|
> -// and |focus| are transfered to the marker range object.
> +// |range| is destructed at the end of this method. |anchor| and 
> |focus| are
> +// copied into the individual text markers.
> id CreateTextMarkerRange(const AXPlatformRange range) {
> - AXTextMarkerRef start_marker = AXTextMarkerCreate(
> + base::ScopedCFTypeRef<AXTextMarkerRef> start_marker(AXTextMarkerCreate(
> kCFAllocatorDefault, reinterpret_cast<const UInt8*>(range.anchor()),
> - sizeof(AXPlatformPosition));
> - AXTextMarkerRef end_marker = AXTextMarkerCreate(
> + sizeof(AXPlatformPosition)));
> + base::ScopedCFTypeRef<AXTextMarkerRef> end_marker(AXTextMarkerCreate(
> kCFAllocatorDefault, reinterpret_cast<const UInt8*>(range.focus()),
> - sizeof(AXPlatformPosition));
> + sizeof(AXPlatformPosition)));
> AXTextMarkerRangeRef marker_range =
> AXTextMarkerRangeCreate(kCFAllocatorDefault, start_marker, end_marker);
> return static_cast<id>(
>
> How do I find these bugs out so that I don't repeat them in the future?
Is there a tool for this?


-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tapted, 2017-06-22 23:48:38]

On 23 June 2017 at 01:55, Nektarios Paisios <nektar@google.com> wrote:

> +// AXTextMarkerCreate copies from data buffer given to it.
>
> id CreateTextMarker(AXPlatformPositionInstance position) {
> AXTextMarkerRef text_marker = AXTextMarkerCreate(
> - kCFAllocatorDefault, reinterpret_cast<const UInt8*>(position.release()),
> + kCFAllocatorDefault, reinterpret_cast<const UInt8*>(position.get()),
>
>
> What's the difference? Doesn't |Release| return a pointer to the buffer
> with the text marker same as |Get|?
> Reason I used Release instead of Get is to make ownership transfer
> explicit so that no code that might later be added to the function will be
> tempted to use the unique_ptr again.
>

There isn't a way for the private API (i.e. AXTextMarkerCreate) to take
ownership -- it only supports copying. So we still need the unique_ptr in
|position| to free its copy, which it won't do after release().

This does mean that we have always been copying a unique_ptr, which is not
meant to be copyable. It still works fine, since AXPosition is just
plain-old-data apart from the vtable pointer (which is static).

Note the copy being done internally by AXTextMarkerCreate is balanced by
the memcpy when we take it back out in CreatePositionFromTextMarker.

I'm exploring a more explicit approach in the follow-up that doesn't copy
the vtable pointer --
https://codereview.chromium.org/2940923003/diff/140001/ui/accessibility/platf...
. But I didn't want to mix in the fix for the leak in the refactor.


> How do I find these bugs out so that I don't repeat them in the future?
>
> Is there a tool for this?
>

Instruments.app is what I used - there are notes and repro steps in the
linked bug, http://crbug.com/735793 . But Instruments is very information
dense - I tried to extract a textual report in the style of ASAN and could
not. Also we don't run the lsan leak checker on Mac -- only Linux. Mac
doesn't support lsan and valgrind stop supporting Mac a long time ago (we
once did have a valgrind bot). We are _finally_ moving our bots past XCode
5 now that the OS on all the VMs is upgraded, so we may be able to figure
out some way to invoke Instruments.app from the command line and run it on
the bots.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.