Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2609503002:
Fix cast_channel::KeepAliveDelegate DCHECK failure. (Closed)
DescriptionFix cast_channel::KeepAliveDelegate DCHECK failure.
Fixes a DCHECK crash caused by KeepAliveDelegate receiving messages
after an error occurs. In this case, the DCHECK's assumption seems to
have been wrong; and so some extra implementation was added to account
for the started versus not-started state.
BUG=677367
R=wez@chromium.org
Review-Url: https://codereview.chromium.org/2609503002
Cr-Commit-Position: refs/heads/master@{#443809}
Committed: https://chromium.googlesource.com/chromium/src/+/b4dbf044c53ead8422a063e7c27ae9999cbecb4d
Patch Set 1 #
Total comments: 6
Patch Set 2 : We must go deeper. #
Total comments: 6
Patch Set 3 : Tweaks, per wez's last round of comments. +REBASE #
Messages
Total messages: 16 (8 generated)
The CQ bit was checked by miu@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...
wez: PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2609503002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/keep_alive_delegate.cc (right): https://codereview.chromium.org/2609503002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/keep_alive_delegate.cc:201: // PING and PONG messages are intentionally suppressed from layers above. nit: This comment is not very clear, especially after the rewrite - perhaps rephrase to "If we reach here then the message is neither PING nor PONG, so pass it on for handling by the layer above." IMO it'd be more straightforward to read wrt the suppression of ping/pongs if you have the top-level conditionals be on the payload type, and then the internal behaviour conditional on |started_|, though I appreciate that that means checking |started_| in two distinct cases. https://codereview.chromium.org/2609503002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/keep_alive_delegate_unittest.cc (right): https://codereview.chromium.org/2609503002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/keep_alive_delegate_unittest.cc:185: EXPECT_CALL(*ping_timer_, Stop()).Times(1); Strictly speaking, GMock has undefined behaviour if you mix EXPECT_CALL() specifications with calls to the object as you're doing here - you should really specify all the expectations up-front and then have the code that drives the test execution. You may want to use Sequence / After to express any ordering constraints on the API that you also want to enforce.
Are you still planning to land this? :)
wez: PTAL. https://codereview.chromium.org/2609503002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/keep_alive_delegate.cc (right): https://codereview.chromium.org/2609503002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/keep_alive_delegate.cc:201: // PING and PONG messages are intentionally suppressed from layers above. On 2017/01/04 00:10:17, Wez wrote: > nit: This comment is not very clear, especially after the rewrite - perhaps > rephrase to "If we reach here then the message is neither PING nor PONG, so pass > it on for handling by the layer above." > > IMO it'd be more straightforward to read wrt the suppression of ping/pongs if > you have the top-level conditionals be on the payload type, and then the > internal behaviour conditional on |started_|, though I appreciate that that > means checking |started_| in two distinct cases. Done. Looking at this code again, I also couldn't help myself: The multiple calls to NestedPayloadTypeEquals() were very wasteful! Each does lots of string copying and JSON parsing and then throws away all that work after doing a simple string equals. So, I changed it to a function that just does the parsing once and extracts the needed field, and now we can compare it multiple times cheaply. https://codereview.chromium.org/2609503002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/keep_alive_delegate_unittest.cc (right): https://codereview.chromium.org/2609503002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/keep_alive_delegate_unittest.cc:185: EXPECT_CALL(*ping_timer_, Stop()).Times(1); On 2017/01/04 00:10:17, Wez wrote: > Strictly speaking, GMock has undefined behaviour if you mix EXPECT_CALL() > specifications with calls to the object as you're doing here - My understanding is it always works so long as the EXPECT_CALL() statements are before the code that will (or shouldn't) cause the calls. But maybe I'm wrong. > you should really > specify all the expectations up-front and then have the code that drives the > test execution. Done. > You may want to use Sequence / After to express any ordering constraints on the > API that you also want to enforce. Done.
LGTM https://codereview.chromium.org/2609503002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/keep_alive_delegate.cc (right): https://codereview.chromium.org/2609503002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/keep_alive_delegate.cc:201: // PING and PONG messages are intentionally suppressed from layers above. On 2017/01/12 22:31:27, miu wrote: > On 2017/01/04 00:10:17, Wez wrote: > > nit: This comment is not very clear, especially after the rewrite - perhaps > > rephrase to "If we reach here then the message is neither PING nor PONG, so > pass > > it on for handling by the layer above." > > > > IMO it'd be more straightforward to read wrt the suppression of ping/pongs if > > you have the top-level conditionals be on the payload type, and then the > > internal behaviour conditional on |started_|, though I appreciate that that > > means checking |started_| in two distinct cases. > > Done. > > Looking at this code again, I also couldn't help myself: The multiple calls to > NestedPayloadTypeEquals() were very wasteful! Each does lots of string copying > and JSON parsing and then throws away all that work after doing a simple string > equals. So, I changed it to a function that just does the parsing once and > extracts the needed field, and now we can compare it multiple times cheaply. Nice :) https://codereview.chromium.org/2609503002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/keep_alive_delegate_unittest.cc (right): https://codereview.chromium.org/2609503002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/keep_alive_delegate_unittest.cc:185: EXPECT_CALL(*ping_timer_, Stop()).Times(1); On 2017/01/12 22:31:27, miu wrote: > On 2017/01/04 00:10:17, Wez wrote: > > Strictly speaking, GMock has undefined behaviour if you mix EXPECT_CALL() > > specifications with calls to the object as you're doing here - > > My understanding is it always works so long as the EXPECT_CALL() statements are > before the code that will (or shouldn't) cause the calls. But maybe I'm wrong. I believe you're right, but strictly it is undefined, so I prefer to try to stick to the defined approach, which is easier to sanity-check. > > you should really > > specify all the expectations up-front and then have the code that drives the > > test execution. > > Done. > > > You may want to use Sequence / After to express any ordering constraints on > the > > API that you also want to enforce. > > Done. https://codereview.chromium.org/2609503002/diff/20001/extensions/browser/api/... File extensions/browser/api/cast_channel/keep_alive_delegate.cc (right): https://codereview.chromium.org/2609503002/diff/20001/extensions/browser/api/... extensions/browser/api/cast_channel/keep_alive_delegate.cc:30: // found. Sorry, I completely missed that this was parsing JSON - is JSONReader::Read() safe to use in the browser process? https://codereview.chromium.org/2609503002/diff/20001/extensions/browser/api/... extensions/browser/api/cast_channel/keep_alive_delegate.cc:183: return; nit: Either remove the return's here, since they're unnecessary given the else's, or move the OnMessage callback back out of an else block, change this block to a plain if, and keep the return's as early-exits. https://codereview.chromium.org/2609503002/diff/20001/extensions/browser/api/... File extensions/browser/api/cast_channel/keep_alive_delegate_unittest.cc (right): https://codereview.chromium.org/2609503002/diff/20001/extensions/browser/api/... extensions/browser/api/cast_channel/keep_alive_delegate_unittest.cc:211: // Process a late-arriving PING/PONG message, which should have no effect.. nit: Trailing .
https://codereview.chromium.org/2609503002/diff/20001/extensions/browser/api/... File extensions/browser/api/cast_channel/keep_alive_delegate.cc (right): https://codereview.chromium.org/2609503002/diff/20001/extensions/browser/api/... extensions/browser/api/cast_channel/keep_alive_delegate.cc:30: // found. On 2017/01/13 22:37:11, Wez wrote: > Sorry, I completely missed that this was parsing JSON - is JSONReader::Read() > safe to use in the browser process? I'm not sure. It was already doing that. I'll mention this to folks I know are maintaining this. https://codereview.chromium.org/2609503002/diff/20001/extensions/browser/api/... extensions/browser/api/cast_channel/keep_alive_delegate.cc:183: return; On 2017/01/13 22:37:11, Wez wrote: > nit: Either remove the return's here, since they're unnecessary given the > else's, or move the OnMessage callback back out of an else block, change this > block to a plain if, and keep the return's as early-exits. Done. That was an oops on my part (left-over from prior patch set, I meant to remove them since they are redundant now). https://codereview.chromium.org/2609503002/diff/20001/extensions/browser/api/... File extensions/browser/api/cast_channel/keep_alive_delegate_unittest.cc (right): https://codereview.chromium.org/2609503002/diff/20001/extensions/browser/api/... extensions/browser/api/cast_channel/keep_alive_delegate_unittest.cc:211: // Process a late-arriving PING/PONG message, which should have no effect.. On 2017/01/13 22:37:11, Wez wrote: > nit: Trailing . Done.
The CQ bit was checked by miu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/2609503002/#ps40001 (title: "Tweaks, per wez's last round of comments. +REBASE")
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": 40001, "attempt_start_ts": 1484446655791430,
"parent_rev": "c50ed7de30182bfd6d5eb36e049be98b497d13ac", "commit_rev":
"b4dbf044c53ead8422a063e7c27ae9999cbecb4d"}
Message was sent while issue was closed.
Description was changed from ========== Fix cast_channel::KeepAliveDelegate DCHECK failure. Fixes a DCHECK crash caused by KeepAliveDelegate receiving messages after an error occurs. In this case, the DCHECK's assumption seems to have been wrong; and so some extra implementation was added to account for the started versus not-started state. BUG=677367 R=wez@chromium.org ========== to ========== Fix cast_channel::KeepAliveDelegate DCHECK failure. Fixes a DCHECK crash caused by KeepAliveDelegate receiving messages after an error occurs. In this case, the DCHECK's assumption seems to have been wrong; and so some extra implementation was added to account for the started versus not-started state. BUG=677367 R=wez@chromium.org Review-Url: https://codereview.chromium.org/2609503002 Cr-Commit-Position: refs/heads/master@{#443809} Committed: https://chromium.googlesource.com/chromium/src/+/b4dbf044c53ead8422a063e7c27a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b4dbf044c53ead8422a063e7c27a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
