New issue
Advanced search Search tips

Issue 643059 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Sep 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

ArcDownloadsWatcherService accesses ArcBridgeService on the FILE thread

Project Member Reported by hashimoto@chromium.org, Sep 1 2016

Issue description

ArcBridgeService is not thread-safe.
It should be only accessed on the UI thread.
Accessing it on a thread other than the UI thread can result in a crash.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 2 2016

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

commit 3c74f1cb2742e9b050fbc830f1ac0e06163c2370
Author: hashimoto <hashimoto@chromium.org>
Date: Fri Sep 02 05:30:31 2016

arc: Fix thread unsafe behavior of ArcDownloadsWatcherService

ArcBridgeService is not thread-safe.
WeakPtr should be defererenced only on the thread to which the
factory belongs.

BUG= 643059 

Review-Url: https://codereview.chromium.org/2304523002
Cr-Commit-Position: refs/heads/master@{#416194}

[modify] https://crrev.com/3c74f1cb2742e9b050fbc830f1ac0e06163c2370/chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc
[modify] https://crrev.com/3c74f1cb2742e9b050fbc830f1ac0e06163c2370/chrome/browser/chromeos/arc/arc_downloads_watcher_service.h

Labels: Merge-Request-53 Merge-Request-54
Requesting merge of https://codereview.chromium.org/2304523002 to both M53 and M54 branches.
The patch fixes a potential browser crash.

Comment 3 by dimu@chromium.org, Sep 3 2016

Labels: -Merge-Request-53 Merge-Review-53 Hotlist-Merge-Review
[Automated comment] Less than 2 weeks to go before stable on M53, manual review required.

Comment 4 by dimu@chromium.org, Sep 3 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 6 2016

Labels: -merge-approved-54 merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5c1f69d4f8a639a2d63f23c4fd9d32ba7b14de95

commit 5c1f69d4f8a639a2d63f23c4fd9d32ba7b14de95
Author: Ryo Hashimoto <hashimoto@chromium.org>
Date: Tue Sep 06 00:43:22 2016

arc: Fix thread unsafe behavior of ArcDownloadsWatcherService

ArcBridgeService is not thread-safe.
WeakPtr should be defererenced only on the thread to which the
factory belongs.

BUG= 643059 

Review-Url: https://codereview.chromium.org/2304523002
Cr-Commit-Position: refs/heads/master@{#416194}
(cherry picked from commit 3c74f1cb2742e9b050fbc830f1ac0e06163c2370)

Review URL: https://codereview.chromium.org/2312653002 .

Cr-Commit-Position: refs/branch-heads/2840@{#159}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/5c1f69d4f8a639a2d63f23c4fd9d32ba7b14de95/chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc
[modify] https://crrev.com/5c1f69d4f8a639a2d63f23c4fd9d32ba7b14de95/chrome/browser/chromeos/arc/arc_downloads_watcher_service.h

Labels: -Merge-Review-53 Merge-Approved-53
Approving merge to M53. please merge in the next hour if possible.
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 7 2016

Labels: -merge-approved-53 merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8c306683e2476a091d8fd0b94c87a9ecefff87bd

commit 8c306683e2476a091d8fd0b94c87a9ecefff87bd
Author: Ryo Hashimoto <hashimoto@chromium.org>
Date: Wed Sep 07 03:27:46 2016

arc: Fix thread unsafe behavior of ArcDownloadsWatcherService

ArcBridgeService is not thread-safe.
WeakPtr should be defererenced only on the thread to which the
factory belongs.

BUG= 643059 

Review-Url: https://codereview.chromium.org/2304523002
Cr-Commit-Position: refs/heads/master@{#416194}
(cherry picked from commit 3c74f1cb2742e9b050fbc830f1ac0e06163c2370)

Review URL: https://codereview.chromium.org/2313213002 .

Cr-Commit-Position: refs/branch-heads/2785@{#840}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/8c306683e2476a091d8fd0b94c87a9ecefff87bd/chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc
[modify] https://crrev.com/8c306683e2476a091d8fd0b94c87a9ecefff87bd/chrome/browser/chromeos/arc/arc_downloads_watcher_service.h

Project Member

Comment 8 by bugdroid1@chromium.org, Sep 7 2016

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

commit 81cbe625b190b44df96a528364f84df777fb3a31
Author: hashimoto <hashimoto@chromium.org>
Date: Wed Sep 07 03:41:13 2016

Revert of arc: Fix thread unsafe behavior of ArcDownloadsWatcherService (patchset #1 id:1 of https://codereview.chromium.org/2313213002/ )

Reason for revert:
Syntax error

Original issue's description:
> arc: Fix thread unsafe behavior of ArcDownloadsWatcherService
>
> ArcBridgeService is not thread-safe.
> WeakPtr should be defererenced only on the thread to which the
> factory belongs.
>
> BUG= 643059 
>
> Review-Url: https://codereview.chromium.org/2304523002
> Cr-Commit-Position: refs/heads/master@{#416194}
> (cherry picked from commit 3c74f1cb2742e9b050fbc830f1ac0e06163c2370)
>
> Committed: https://chromium.googlesource.com/chromium/src/+/8c306683e2476a091d8fd0b94c87a9ecefff87bd

TBR=
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 643059 

Review-Url: https://codereview.chromium.org/2315013003
Cr-Commit-Position: refs/branch-heads/2785@{#841}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/81cbe625b190b44df96a528364f84df777fb3a31/chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc
[modify] https://crrev.com/81cbe625b190b44df96a528364f84df777fb3a31/chrome/browser/chromeos/arc/arc_downloads_watcher_service.h

Project Member

Comment 9 by bugdroid1@chromium.org, Sep 7 2016

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

commit 5bb81307776aa149e9fbbce05e77bd30fb058e4d
Author: Ryo Hashimoto <hashimoto@chromium.org>
Date: Wed Sep 07 04:41:38 2016

arc: Fix thread unsafe behavior of ArcDownloadsWatcherService

ArcBridgeService is not thread-safe.
WeakPtr should be defererenced only on the thread to which the
factory belongs.

BUG= 643059 

Review-Url: https://codereview.chromium.org/2304523002
Cr-Commit-Position: refs/heads/master@{#416194}
(cherry picked from commit 3c74f1cb2742e9b050fbc830f1ac0e06163c2370)

Review URL: https://codereview.chromium.org/2320453002 .

Cr-Commit-Position: refs/branch-heads/2785@{#842}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/5bb81307776aa149e9fbbce05e77bd30fb058e4d/chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc
[modify] https://crrev.com/5bb81307776aa149e9fbbce05e77bd30fb058e4d/chrome/browser/chromeos/arc/arc_downloads_watcher_service.h

Status: Fixed (was: Started)
Status: Verified (was: Fixed)
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 27 2016

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

commit 5c1f69d4f8a639a2d63f23c4fd9d32ba7b14de95
Author: Ryo Hashimoto <hashimoto@chromium.org>
Date: Tue Sep 06 00:43:22 2016

arc: Fix thread unsafe behavior of ArcDownloadsWatcherService

ArcBridgeService is not thread-safe.
WeakPtr should be defererenced only on the thread to which the
factory belongs.

BUG= 643059 

Review-Url: https://codereview.chromium.org/2304523002
Cr-Commit-Position: refs/heads/master@{#416194}
(cherry picked from commit 3c74f1cb2742e9b050fbc830f1ac0e06163c2370)

Review URL: https://codereview.chromium.org/2312653002 .

Cr-Commit-Position: refs/branch-heads/2840@{#159}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/5c1f69d4f8a639a2d63f23c4fd9d32ba7b14de95/chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc
[modify] https://crrev.com/5c1f69d4f8a639a2d63f23c4fd9d32ba7b14de95/chrome/browser/chromeos/arc/arc_downloads_watcher_service.h

Sign in to add a comment