New issue
Advanced search Search tips

Issue 778504 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug



Sign in to add a comment

[CRD iOS] Fix misuse of Objective-C blocks in C++ class

Project Member Reported by yuweih@chromium.org, Oct 26 2017

Issue description

See example here:

    class GlDisplayHander::Core {
      ...
      runtime_->ui_task_runner()->PostTask(FROM_HERE, base::BindBlockArc(^() {
                                             [handler_delegate_ rendererTicked];
                                           }));
    };

https://cs.chromium.org/chromium/src/remoting/ios/display/gl_display_handler.mm?l=174&rcl=b8a6fd5abbbdb8471f49672b7ea71caa378328aa

It is thread unsafe to capture (or implicitly capture) C++ objects in an Objective-C block because the C++ object doesn't do ARC. In this case the app may occasionally (<10%) crash when disconnecting from the host because the block attempts to dereference |this| to get back |handler_delegate_| while |this| is being destroyed.
 

Comment 1 by yuweih@chromium.org, Oct 26 2017

Description: Show this description
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0955999c5d92165889d95c50509c8c704a976038

commit 0955999c5d92165889d95c50509c8c704a976038
Author: Yuwei Huang <yuweih@chromium.org>
Date: Thu Oct 26 21:01:28 2017

[CRD iOS] Fix blocks that implicitly dereference |this|.

Objective-C's block doesn't do ARC on C++ objects, so implicitly
capturing |this| in a C++ class is thread unsafe. We did see crashes in
GlDisplayHandler::Core that are related to that.

This CL fixes these block so that they don't dereference |this|.

Bug:  778504 
Change-Id: I47855fb601bbe07c7242bec4278ef3e48fa5e910
Reviewed-on: https://chromium-review.googlesource.com/738806
Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511956}
[modify] https://crrev.com/0955999c5d92165889d95c50509c8c704a976038/remoting/ios/display/gl_display_handler.mm

Comment 3 by yuweih@chromium.org, Oct 27 2017

Status: Fixed (was: Assigned)

Sign in to add a comment