Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(421)

Issue 19678028: PAPI: Fix bug in RunWhileLocked, add support for params (Closed)

Created:
7 years, 5 months ago by dmichael (off chromium)
Modified:
7 years, 5 months ago
Reviewers:
teravest, bbudge, yzshen1
CC:
chromium-reviews, darin-cc_chromium.org, yzshen1
Visibility:
Public.

Description

PAPI: Fix bug in RunWhileLocked, add support for params This CL: - Fixes a bug whereby a Callback bound by RunWhileLocked would release the lock before releasing bound values. This could lead to de-refing and possibly destroying objects (like Resources) that should only be manipulated with the ProxyLock held. - Adds support for up to 3 parameters to RunWhileLocked. Should be easy to extend to more if/when we want. - Adds a unit test that checks the bug and tests other utilities from ProxyLock. There is still possibly a rare issue with the Callback being destroyed without being run, e.g. at process shutdown. I'm going to clean that up in a separate CL: https://codereview.chromium.org/19492014/ BUG= R=bbudge@chromium.org, teravest@chromium.org, yzshen@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213693

Patch Set 1 : add the unit test #

Total comments: 6

Patch Set 2 : improve comment #

Patch Set 3 : make CallOnMainThread lock #

Patch Set 4 : other review comments + merge #

Patch Set 5 : Remove ProxyLock::Get() (a.k.a. Sir Not-appearing-in-this-CL) #

Patch Set 6 : Unlock when joining the audio thread so that we don't deadlock if it makes pepper calls. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+336 lines, -17 lines) Patch
M ppapi/ppapi_tests.gypi View 1 chunk +1 line, -0 lines 0 comments Download
M ppapi/proxy/ppb_core_proxy.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M ppapi/shared_impl/ppb_audio_shared.cc View 1 2 3 4 5 2 chunks +11 lines, -2 lines 0 comments Download
M ppapi/shared_impl/proxy_lock.h View 1 2 3 4 2 chunks +143 lines, -10 lines 0 comments Download
M ppapi/shared_impl/proxy_lock.cc View 1 chunk +0 lines, -5 lines 0 comments Download
A ppapi/shared_impl/proxy_lock_unittest.cc View 1 chunk +180 lines, -0 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
dmichael (off chromium)
This is the CL for fixing RunWhileLocked and adding support for params. PTAL. When this ...
7 years, 5 months ago (2013-07-24 16:31:52 UTC) #1
teravest
https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lock.h File ppapi/shared_impl/proxy_lock.h (right): https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lock.h#newcode33 ppapi/shared_impl/proxy_lock.h:33: // you need access to the ProxyLock for e.g. ...
7 years, 5 months ago (2013-07-24 16:38:44 UTC) #2
teravest
On Wed, Jul 24, 2013 at 10:38 AM, <teravest@chromium.org> wrote: > > https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lock.h > File ...
7 years, 5 months ago (2013-07-24 17:02:18 UTC) #3
bbudge
LGTM
7 years, 5 months ago (2013-07-24 17:57:39 UTC) #4
yzshen1
https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lock.h File ppapi/shared_impl/proxy_lock.h (left): https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lock.h#oldcode43 ppapi/shared_impl/proxy_lock.h:43: private: Out of curiosity: Why removing "private:"? https://codereview.chromium.org/19678028/diff/6001/ppapi/shared_impl/proxy_lock.h File ...
7 years, 5 months ago (2013-07-24 18:45:19 UTC) #5
dmichael (off chromium)
Comments addressed, PTAL. I don't want to do ProxyLock::IsAcquiredByCurrentThread()... it would be roughly morally equivalent ...
7 years, 5 months ago (2013-07-24 22:30:11 UTC) #6
yzshen1
Thanks for the detailed reply! I agree that the current way is probably better than ...
7 years, 5 months ago (2013-07-24 22:54:10 UTC) #7
teravest
lgtm
7 years, 5 months ago (2013-07-25 13:46:28 UTC) #8
dmichael (off chromium)
I had to do one more small change to fix a deadlock in the Audio ...
7 years, 5 months ago (2013-07-25 17:40:02 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmichael@chromium.org/19678028/42001
7 years, 5 months ago (2013-07-25 18:06:10 UTC) #10
dmichael (off chromium)
7 years, 5 months ago (2013-07-25 21:09:19 UTC) #11
Message was sent while issue was closed.
Committed patchset #6 manually as r213693 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698