Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2503943002:
Fail gracefully if the postMessage channel is closed (Closed)
DescriptionFail gracefully if the postMessage channel is closed
Although we check for the channel being available before posting
the task on UI thread, it is still possible to get a navigation block
the thread and close the channel before the task is completed. The
testPostMessageBasic case actually enforces this, so ends up flaking.
Avoid crashes and fail gracefully here.
BUG=665513
Committed: https://crrev.com/95e00cf8c6cc687b227d0c3e291b95df211d6d6b
Cr-Commit-Position: refs/heads/master@{#432305}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 23 (11 generated)
yusufo@chromium.org changed reviewers: + jbudorick@chromium.org, lizeb@chromium.org
The CQ bit was checked by yusufo@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...
https://codereview.chromium.org/2503943002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java (right): https://codereview.chromium.org/2503943002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java:148: if (mChannel == null || mChannel[0].isClosed()) return; Should we do something to indicate this kind of failure? I guess at this point, we've already returned RESULT_SUCCESS, so perhaps there's nothing sensible to do here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2503943002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java (right): https://codereview.chromium.org/2503943002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java:148: if (mChannel == null || mChannel[0].isClosed()) return; On 2016/11/15 19:13:12, jbudorick wrote: > Should we do something to indicate this kind of failure? > > I guess at this point, we've already returned RESULT_SUCCESS, so perhaps there's > nothing sensible to do here. Yeah, the return is more about accepting the request than an actual message being delivered, since it can actually fail across the pipeline in different ways as well. postMessage protocol doesn't really make guarantees around that. At this point there is not much we can do other than not crash.
On 2016/11/15 19:16:32, Yusuf wrote: > https://codereview.chromium.org/2503943002/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java > (right): > > https://codereview.chromium.org/2503943002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java:148: > if (mChannel == null || mChannel[0].isClosed()) return; > On 2016/11/15 19:13:12, jbudorick wrote: > > Should we do something to indicate this kind of failure? > > > > I guess at this point, we've already returned RESULT_SUCCESS, so perhaps > there's > > nothing sensible to do here. > > Yeah, the return is more about accepting the request than an actual message > being delivered, since it can actually fail across the pipeline in different > ways as well. postMessage protocol doesn't really make guarantees around that. > At this point there is not much we can do other than not crash. sgtm / lgtm
The CQ bit was checked by yusufo@chromium.org
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
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by yusufo@chromium.org
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
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by yusufo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Fail gracefully if the postMessage channel is closed Although we check for the channel being available before posting the task on UI thread, it is still possible to get a navigation block the thread and close the channel before the task is completed. The testPostMessageBasic case actually enforces this, so ends up flaking. Avoid crashes and fail gracefully here. BUG=665513 ========== to ========== Fail gracefully if the postMessage channel is closed Although we check for the channel being available before posting the task on UI thread, it is still possible to get a navigation block the thread and close the channel before the task is completed. The testPostMessageBasic case actually enforces this, so ends up flaking. Avoid crashes and fail gracefully here. BUG=665513 Committed: https://crrev.com/95e00cf8c6cc687b227d0c3e291b95df211d6d6b Cr-Commit-Position: refs/heads/master@{#432305} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/95e00cf8c6cc687b227d0c3e291b95df211d6d6b Cr-Commit-Position: refs/heads/master@{#432305} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
