Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2688413006:
Fixed crash if tab closes while WebShare picker dialog is open. (Closed)
|
Created:
3 years, 10 months ago by Matt Giuca Modified:
3 years, 10 months ago Reviewers:
Sam McNally CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixed crash if tab closes while WebShare picker dialog is open.
Now, in this case, the picker dialog will do nothing (it has a weak
pointer to the share service which is invalidated if the tab closes).
Significantly reworked unit test to make testing this case possible:
instead of synchronously closing the picker in ShowPickerDialog, it now
stores the callback and exits the run-loop, allowing the individual
tests to call the callback as they wish. The new test
ShareServiceDeletion deletes the share service before calling the
callback, which would have crashed before this fix.
BUG=690775
Review-Url: https://codereview.chromium.org/2688413006
Cr-Commit-Position: refs/heads/master@{#451169}
Committed: https://chromium.googlesource.com/chromium/src/+/bd4b24d3cb10b535d47005567c3d5589125585fe
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix comments. #
Total comments: 10
Patch Set 3 : Do Sam's suggestions. #
Messages
Total messages: 28 (17 generated)
The CQ bit was checked by mgiuca@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...
mgiuca@chromium.org changed reviewers: + sammc@chromium.org
This is pretty complex and I have some questions about it. Let's discuss tomorrow.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2688413006/diff/1/chrome/browser/webshare/sha... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2688413006/diff/1/chrome/browser/webshare/sha... chrome/browser/webshare/share_service_impl_unittest.cc:163: std::vector<std::pair<base::string16, GURL>> targets_in_picker = Move these out. https://codereview.chromium.org/2688413006/diff/1/chrome/browser/webshare/sha... chrome/browser/webshare/share_service_impl_unittest.cc:309: [](const base::Optional<std::string>& error) { NOTREACHED(); }); FAIL()
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
https://codereview.chromium.org/2688413006/diff/1/chrome/browser/webshare/sha... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2688413006/diff/1/chrome/browser/webshare/sha... chrome/browser/webshare/share_service_impl_unittest.cc:163: std::vector<std::pair<base::string16, GURL>> targets_in_picker = On 2017/02/15 06:27:55, Sam McNally wrote: > Move these out. Done. Oh hey, this can be a regular function now (moved up). Could even be a lambda but I think that makes the tests a bit unreadable. https://codereview.chromium.org/2688413006/diff/1/chrome/browser/webshare/sha... chrome/browser/webshare/share_service_impl_unittest.cc:309: [](const base::Optional<std::string>& error) { NOTREACHED(); }); On 2017/02/15 06:27:55, Sam McNally wrote: > FAIL() Done.
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.
lgtm https://codereview.chromium.org/2688413006/diff/20001/chrome/browser/webshare... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2688413006/diff/20001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl_unittest.cc:78: void set_runloop(base::RunLoop* runloop) { run_loop https://codereview.chromium.org/2688413006/diff/20001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl_unittest.cc:121: base::Closure quit_runloop_; quit_run_loop_ https://codereview.chromium.org/2688413006/diff/20001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl_unittest.cc:182: std::vector<std::pair<base::string16, GURL>> expected_targets{ Create these closer to where they're used. Also consider making them constants. Fix everywhere. https://codereview.chromium.org/2688413006/diff/20001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl_unittest.cc:222: EXPECT_EQ(expected_targets, share_service_helper()->GetTargetsInPicker()); EXPECT_TRUE(share_service_helper()->GetTargetsInPicker().empty()); https://codereview.chromium.org/2688413006/diff/20001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl_unittest.cc:258: EXPECT_EQ(GURL(), share_service_helper()->GetLastUsedTargetURL()); EXPECT_TRUE(share_service_helper()->GetLastUsedTargetURL().is_empty());
Thanks, good suggestions. https://codereview.chromium.org/2688413006/diff/20001/chrome/browser/webshare... File chrome/browser/webshare/share_service_impl_unittest.cc (right): https://codereview.chromium.org/2688413006/diff/20001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl_unittest.cc:78: void set_runloop(base::RunLoop* runloop) { On 2017/02/16 02:12:24, Sam McNally wrote: > run_loop Done. https://codereview.chromium.org/2688413006/diff/20001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl_unittest.cc:121: base::Closure quit_runloop_; On 2017/02/16 02:12:24, Sam McNally wrote: > quit_run_loop_ Done. https://codereview.chromium.org/2688413006/diff/20001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl_unittest.cc:182: std::vector<std::pair<base::string16, GURL>> expected_targets{ On 2017/02/16 02:12:24, Sam McNally wrote: > Create these closer to where they're used. Also consider making them constants. > Fix everywhere. Done. https://codereview.chromium.org/2688413006/diff/20001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl_unittest.cc:222: EXPECT_EQ(expected_targets, share_service_helper()->GetTargetsInPicker()); On 2017/02/16 02:12:24, Sam McNally wrote: > EXPECT_TRUE(share_service_helper()->GetTargetsInPicker().empty()); Done. https://codereview.chromium.org/2688413006/diff/20001/chrome/browser/webshare... chrome/browser/webshare/share_service_impl_unittest.cc:258: EXPECT_EQ(GURL(), share_service_helper()->GetLastUsedTargetURL()); On 2017/02/16 02:12:24, Sam McNally wrote: > EXPECT_TRUE(share_service_helper()->GetLastUsedTargetURL().is_empty()); Done.
The CQ bit was checked by mgiuca@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org Link to the patchset: https://codereview.chromium.org/2688413006/#ps40001 (title: "Do Sam's suggestions.")
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mgiuca@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mgiuca@chromium.org
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": 1487288359988040,
"parent_rev": "98ec036d053570d83cbad754e229b68e4984bed9", "commit_rev":
"bd4b24d3cb10b535d47005567c3d5589125585fe"}
Message was sent while issue was closed.
Description was changed from ========== Fixed crash if tab closes while WebShare picker dialog is open. Now, in this case, the picker dialog will do nothing (it has a weak pointer to the share service which is invalidated if the tab closes). Significantly reworked unit test to make testing this case possible: instead of synchronously closing the picker in ShowPickerDialog, it now stores the callback and exits the run-loop, allowing the individual tests to call the callback as they wish. The new test ShareServiceDeletion deletes the share service before calling the callback, which would have crashed before this fix. BUG=690775 ========== to ========== Fixed crash if tab closes while WebShare picker dialog is open. Now, in this case, the picker dialog will do nothing (it has a weak pointer to the share service which is invalidated if the tab closes). Significantly reworked unit test to make testing this case possible: instead of synchronously closing the picker in ShowPickerDialog, it now stores the callback and exits the run-loop, allowing the individual tests to call the callback as they wish. The new test ShareServiceDeletion deletes the share service before calling the callback, which would have crashed before this fix. BUG=690775 Review-Url: https://codereview.chromium.org/2688413006 Cr-Commit-Position: refs/heads/master@{#451169} Committed: https://chromium.googlesource.com/chromium/src/+/bd4b24d3cb10b535d47005567c3d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/bd4b24d3cb10b535d47005567c3d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
