Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2857743003:
Android screencapture: fix possible hang in stopCapture() (Closed)
|
Created:
3 years, 7 months ago by braveyao Modified:
3 years, 7 months ago Reviewers:
miu CC:
chromium-reviews, chfremer+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, agrieve+watch_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid screencapture: fix possible hang in stopCapture()
When user denies screencapture by clicking 'Cancel' in OS permission dialog,
Chrome will stop the capture after receiving the OnError() report. At this
point, the |mMediaProjection| is not created yet and |mCaptureState| has moved
to |ATTACHED|. So the capture state won't be |STOPPED| which will keep
stopCapture() waiting at [1].
The fix is to change the capture state to |STOPPED| for such a scenario.
[1]: https://cs.chromium.org/chromium/src/media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java?l=324
BUG=718082
Review-Url: https://codereview.chromium.org/2857743003
Cr-Commit-Position: refs/heads/master@{#469255}
Committed: https://chromium.googlesource.com/chromium/src/+/8c6cff65cbe641a213c46a086fd7efb939b0e355
Patch Set 1 #
Total comments: 2
Patch Set 2 : address nit #Messages
Total messages: 19 (14 generated)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Android screencapture: fix possible hang in stopCapture() When user denies screencapture by clicking 'Cancel' in OS permission dialog, Chrome will stop the capture after receiving the OnError() report. At this moment, the |mMediaProjection| is not created yet and |mCaptureState| has moved to |ATTACHED|. So the capture state won't be |STOPPED| which will keep stopCapture() waiting at [1]. The fix is to change the capture state to |STOPPED| for such a scenario. [1]: https://cs.chromium.org/chromium/src/media/capture/content/android/java/src/o... BUG=718082 ========== to ========== Android screencapture: fix possible hang in stopCapture() When user denies screencapture by clicking 'Cancel' in OS permission dialog, Chrome will stop the capture after receiving the OnError() report. At this point, the |mMediaProjection| is not created yet and |mCaptureState| has moved to |ATTACHED|. So the capture state won't be |STOPPED| which will keep stopCapture() waiting at [1]. The fix is to change the capture state to |STOPPED| for such a scenario. [1]: https://cs.chromium.org/chromium/src/media/capture/content/android/java/src/o... BUG=718082 ==========
braveyao@chromium.org changed reviewers: + miu@chromium.org
Hi miu@, Please take a look.
lgtm % one nit: https://codereview.chromium.org/2857743003/diff/1/media/capture/content/andro... File media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/2857743003/diff/1/media/capture/content/andro... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:326: while (mCaptureState != CaptureState.STOPPED) { This whole while-block should be in the then-clause above, because it will never execute if your new else-clause path is taken.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2857743003/diff/1/media/capture/content/andro... File media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/2857743003/diff/1/media/capture/content/andro... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:326: while (mCaptureState != CaptureState.STOPPED) { On 2017/05/03 21:43:14, miu wrote: > This whole while-block should be in the then-clause above, because it will never > execute if your new else-clause path is taken. Done.
The CQ bit was checked by braveyao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org Link to the patchset: https://codereview.chromium.org/2857743003/#ps20001 (title: "address nit")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493867076759650,
"parent_rev": "4e8a529c38afa4770486d8033cb37af0d6caf311", "commit_rev":
"8c6cff65cbe641a213c46a086fd7efb939b0e355"}
Message was sent while issue was closed.
Description was changed from ========== Android screencapture: fix possible hang in stopCapture() When user denies screencapture by clicking 'Cancel' in OS permission dialog, Chrome will stop the capture after receiving the OnError() report. At this point, the |mMediaProjection| is not created yet and |mCaptureState| has moved to |ATTACHED|. So the capture state won't be |STOPPED| which will keep stopCapture() waiting at [1]. The fix is to change the capture state to |STOPPED| for such a scenario. [1]: https://cs.chromium.org/chromium/src/media/capture/content/android/java/src/o... BUG=718082 ========== to ========== Android screencapture: fix possible hang in stopCapture() When user denies screencapture by clicking 'Cancel' in OS permission dialog, Chrome will stop the capture after receiving the OnError() report. At this point, the |mMediaProjection| is not created yet and |mCaptureState| has moved to |ATTACHED|. So the capture state won't be |STOPPED| which will keep stopCapture() waiting at [1]. The fix is to change the capture state to |STOPPED| for such a scenario. [1]: https://cs.chromium.org/chromium/src/media/capture/content/android/java/src/o... BUG=718082 Review-Url: https://codereview.chromium.org/2857743003 Cr-Commit-Position: refs/heads/master@{#469255} Committed: https://chromium.googlesource.com/chromium/src/+/8c6cff65cbe641a213c46a086fd7... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/8c6cff65cbe641a213c46a086fd7... |
