Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2957213002:
[TTS] Fix assert after TTS and Context Menu. (Closed)
|
Created:
3 years, 5 months ago by Donn Denman Modified:
3 years, 5 months ago Reviewers:
Theresa CC:
chromium-reviews, twellington+watch_chromium.org, donnd+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[TTS] Fix assert after TTS and Context Menu.
ContextualSearchSelectionController was checking whether the context menu
had been shown to sort out confusing event sequences. This is
causing the CSM to assert because the selection type is not set up
when a long-press follows Context Menu dismissal.
BUG=737285, 628436
Review-Url: https://codereview.chromium.org/2957213002
Cr-Commit-Position: refs/heads/master@{#483068}
Committed: https://chromium.googlesource.com/chromium/src/+/ae01e3f319b3d133c7c54c84519dc753b4922cf2
Patch Set 1 #
Total comments: 4
Patch Set 2 : Removed the check for null before trim() -- it's redundant. #
Dependent Patchsets: Messages
Total messages: 20 (10 generated)
The CQ bit was checked by donnd@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...
donnd@chromium.org changed reviewers: + twellington@chromium.org
Theresa, PTAL when convenient.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2957213002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2957213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:444: if (!isTap && mSearchPanel.isPeeking() && selection != null) { TextUtils.isEmpty() should return true if the selection is empty. Is something clearing it between the check on line 434 and here? https://codereview.chromium.org/2957213002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (left): https://codereview.chromium.org/2957213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:255: if (!mIsContextMenuShown) { It appears that this was added so that CS would ignore incoming selection events while the context menu was showing. With the refactoring, it's possible that's no longer needed but we should check that we're not getting into this block and inadvertently setting values.
Theresa, PTAL. Thanks! https://codereview.chromium.org/2957213002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2957213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:444: if (!isTap && mSearchPanel.isPeeking() && selection != null) { On 2017/06/27 23:40:20, Theresa wrote: > TextUtils.isEmpty() should return true if the selection is empty. Is something > clearing it between the check on line 434 and here? Oh, you're right! I didn't see that check -- all I saw was a crash report for trim() being called on null in this method and the only other trims I found were clearly null-checked too. I don't think it's possible for the selection to go null in this single-threaded environment. I guess it must be from older versions of the code. I'll revert this file. https://codereview.chromium.org/2957213002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (left): https://codereview.chromium.org/2957213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:255: if (!mIsContextMenuShown) { On 2017/06/27 23:40:20, Theresa wrote: > It appears that this was added so that CS would ignore incoming selection events > while the context menu was showing. With the refactoring, it's possible that's > no longer needed but we should check that we're not getting into this block and > inadvertently setting values. Humm, not sure how to check for that. I did manually test with the context menu showing, and that's how I found this issue and some pretty wonky behavior (see issue 737252). The converse is certainly true -- there's a sequence where we skip setting the mSelectionType and then showContextualSearch asserts. That's what got me to research this and clean it up. Do you think that manual testing is good enough?
On 2017/06/27 23:59:19, Donn Denman wrote: > Theresa, PTAL. Thanks! > > https://codereview.chromium.org/2957213002/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java > (right): > > https://codereview.chromium.org/2957213002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:444: > if (!isTap && mSearchPanel.isPeeking() && selection != null) { > On 2017/06/27 23:40:20, Theresa wrote: > > TextUtils.isEmpty() should return true if the selection is empty. Is something > > clearing it between the check on line 434 and here? > > Oh, you're right! I didn't see that check -- all I saw was a crash report for > trim() being called on null in this method and the only other trims I found were > clearly null-checked too. > > I don't think it's possible for the selection to go null in this single-threaded > environment. I guess it must be from older versions of the code. I'll revert > this file. > > https://codereview.chromium.org/2957213002/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java > (left): > > https://codereview.chromium.org/2957213002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:255: > if (!mIsContextMenuShown) { > On 2017/06/27 23:40:20, Theresa wrote: > > It appears that this was added so that CS would ignore incoming selection > events > > while the context menu was showing. With the refactoring, it's possible that's > > no longer needed but we should check that we're not getting into this block > and > > inadvertently setting values. > > Humm, not sure how to check for that. I did manually test with the context menu > showing, and that's how I found this issue and some pretty wonky behavior (see > issue 737252). > > The converse is certainly true -- there's a sequence where we skip setting the > mSelectionType and then showContextualSearch asserts. That's what got me to > research this and clean it up. > > Do you think that manual testing is good enough? I'm okay with manual as long as the repro steps in that original issue don't produce a bug. It'd also be interesting to do some log-based testing and check what selection events we're getting while the context menu is showing. In general, it seems odd to me that it's possible to generate selection events on the base page while a context menu is showing on top of it, so that may indicate something we should prohibit at an application level rather than feature level.
Description was changed from ========== [TTS] Fix minor assert and obscure crash issues. For some reason CSSC was checking whether the context menu had been shown -- maybe got that mixed up with the Action Bar. That was causing the CSM to assert because the selection type was not set up in that case. Also check for null before calling trim(). BUG=737286,737285 ========== to ========== [TTS] Fix assert after CL and Context Menu. ContextualSearchSelectionController was checking whether the context menu had been shown to sort out confusing event sequences. This is causing the CSM to assert because the selection type is not set up when a long-press follows Context Menu dismissal. BUG=737285,628436 ==========
Description was changed from ========== [TTS] Fix assert after CL and Context Menu. ContextualSearchSelectionController was checking whether the context menu had been shown to sort out confusing event sequences. This is causing the CSM to assert because the selection type is not set up when a long-press follows Context Menu dismissal. BUG=737285,628436 ========== to ========== [TTS] Fix assert after TTS and Context Menu. ContextualSearchSelectionController was checking whether the context menu had been shown to sort out confusing event sequences. This is causing the CSM to assert because the selection type is not set up when a long-press follows Context Menu dismissal. BUG=737285,628436 ==========
On 2017/06/28 15:15:11, Theresa wrote: > On 2017/06/27 23:59:19, Donn Denman wrote: > > Theresa, PTAL. Thanks! > > > > > https://codereview.chromium.org/2957213002/diff/1/chrome/android/java/src/org... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java > > (right): > > > > > https://codereview.chromium.org/2957213002/diff/1/chrome/android/java/src/org... > > > chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:444: > > if (!isTap && mSearchPanel.isPeeking() && selection != null) { > > On 2017/06/27 23:40:20, Theresa wrote: > > > TextUtils.isEmpty() should return true if the selection is empty. Is > something > > > clearing it between the check on line 434 and here? > > > > Oh, you're right! I didn't see that check -- all I saw was a crash report for > > trim() being called on null in this method and the only other trims I found > were > > clearly null-checked too. > > > > I don't think it's possible for the selection to go null in this > single-threaded > > environment. I guess it must be from older versions of the code. I'll revert > > this file. > > > > > https://codereview.chromium.org/2957213002/diff/1/chrome/android/java/src/org... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java > > (left): > > > > > https://codereview.chromium.org/2957213002/diff/1/chrome/android/java/src/org... > > > chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:255: > > if (!mIsContextMenuShown) { > > On 2017/06/27 23:40:20, Theresa wrote: > > > It appears that this was added so that CS would ignore incoming selection > > events > > > while the context menu was showing. With the refactoring, it's possible > that's > > > no longer needed but we should check that we're not getting into this block > > and > > > inadvertently setting values. > > > > Humm, not sure how to check for that. I did manually test with the context > menu > > showing, and that's how I found this issue and some pretty wonky behavior (see > > issue 737252). > > > > The converse is certainly true -- there's a sequence where we skip setting the > > mSelectionType and then showContextualSearch asserts. That's what got me to > > research this and clean it up. > > > > Do you think that manual testing is good enough? > > I'm okay with manual as long as the repro steps in that original issue don't > produce a bug. It'd also be interesting to do some log-based testing and check > what selection events we're getting while the context menu is showing. In > general, it seems odd to me that it's possible to generate selection events on > the base page while a context menu is showing on top of it, so that may indicate > something we should prohibit at an application level rather than feature level. I looked at that original bug again, which is about a year old, and tried the sequence documented there (tap-select followed by context menu). We don't get any events now, so I think whatever was causing the confusing event sequence was corrected.
Theresa, PTAL. Added that original bug to the BUG= list for this CL and updated the CL description. Thanks!
LGTM Thank you for all of the testing.
The CQ bit was checked by donnd@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the thoughtful reviews!
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1498671206826530,
"parent_rev": "a4b8c993abf3c08f3761dc41666aacc008460da6", "commit_rev":
"ae01e3f319b3d133c7c54c84519dc753b4922cf2"}
Message was sent while issue was closed.
Description was changed from ========== [TTS] Fix assert after TTS and Context Menu. ContextualSearchSelectionController was checking whether the context menu had been shown to sort out confusing event sequences. This is causing the CSM to assert because the selection type is not set up when a long-press follows Context Menu dismissal. BUG=737285,628436 ========== to ========== [TTS] Fix assert after TTS and Context Menu. ContextualSearchSelectionController was checking whether the context menu had been shown to sort out confusing event sequences. This is causing the CSM to assert because the selection type is not set up when a long-press follows Context Menu dismissal. BUG=737285,628436 Review-Url: https://codereview.chromium.org/2957213002 Cr-Commit-Position: refs/heads/master@{#483068} Committed: https://chromium.googlesource.com/chromium/src/+/ae01e3f319b3d133c7c54c84519d... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/ae01e3f319b3d133c7c54c84519d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
