Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2624273003:
Add a test for our workaround of an AppKit crash in SFChooseIdentityPanel. (Closed)
|
Created:
3 years, 11 months ago by Sidney San Martín Modified:
3 years, 11 months ago Reviewers:
Robert Sesek CC:
chromium-reviews, mac-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a test for our workaround of an AppKit crash in SFChooseIdentityPanel.
BUG=679992, 653093
Review-Url: https://codereview.chromium.org/2624273003
Cr-Commit-Position: refs/heads/master@{#443090}
Committed: https://chromium.googlesource.com/chromium/src/+/145b27b6e14ae6dd557c823e39680d62f16b1cf3
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use underscores in C++. #
Total comments: 3
Patch Set 3 : Simplify how we check that the certificate selector was deallocated. #Messages
Total messages: 22 (15 generated)
The CQ bit was checked by sdy@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...
sdy@chromium.org changed reviewers: + rsesek@chromium.org
https://codereview.chromium.org/2624273003/diff/1/chrome/browser/ui/cocoa/ssl... File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm (right): https://codereview.chromium.org/2624273003/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm:145: @end rsesek@: Maybe there's a simpler way to do this? ("This" meaning "make sure that an object was deallocated")
https://codereview.chromium.org/2624273003/diff/1/chrome/browser/ui/cocoa/ssl... File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm (right): https://codereview.chromium.org/2624273003/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm:150: BOOL selectorWasDeallocated = false; I just caught myself not using underscores here :). Updated patch set incoming.
The CQ bit was unchecked by sdy@chromium.org
The CQ bit was checked by sdy@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/2624273003/diff/1/chrome/browser/ui/cocoa/ssl... File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm (right): https://codereview.chromium.org/2624273003/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm:145: @end On 2017/01/11 21:27:03, sdy wrote: > rsesek@: Maybe there's a simpler way to do this? ("This" meaning "make sure that > an object was deallocated") Just a variant of what you're doing here, which is to create a subclass of SSLClientCertificateSelectorCocoa that has two additions: 1) A BOOL* property for wasDeallocated_ 2) Overriden dealloc to deference and set wasDeallocated_ https://codereview.chromium.org/2624273003/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm (right): https://codereview.chromium.org/2624273003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm:172: // Without the workaround, this will crash on Sierra. If you back out the fix, does it indeed crash here?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
https://codereview.chromium.org/2624273003/diff/1/chrome/browser/ui/cocoa/ssl... File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm (right): https://codereview.chromium.org/2624273003/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm:145: @end On 2017/01/11 22:03:31, Robert Sesek wrote: > On 2017/01/11 21:27:03, sdy wrote: > > rsesek@: Maybe there's a simpler way to do this? ("This" meaning "make sure > that > > an object was deallocated") > > Just a variant of what you're doing here, which is to create a subclass of > SSLClientCertificateSelectorCocoa that has two additions: > > 1) A BOOL* property for wasDeallocated_ > 2) Overriden dealloc to deference and set wasDeallocated_ That is better. Done. https://codereview.chromium.org/2624273003/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm (right): https://codereview.chromium.org/2624273003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm:172: // Without the workaround, this will crash on Sierra. On 2017/01/11 22:03:31, Robert Sesek wrote: > If you back out the fix, does it indeed crash here? It does… but not consistently, since the crash happens when AppKit sends a message to a deallocated object. I can't think of simple, non-flaky test that isn't brittle (i.e. likely to break when Apple fixes the bug). Thoughts? How do you feel about this kind of flakiness (false negatives but no false positives)?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2624273003/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm (right): https://codereview.chromium.org/2624273003/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm:172: // Without the workaround, this will crash on Sierra. On 2017/01/11 22:15:33, sdy wrote: > On 2017/01/11 22:03:31, Robert Sesek wrote: > > If you back out the fix, does it indeed crash here? > > It does… but not consistently, since the crash happens when AppKit sends a > message to a deallocated object. I can't think of simple, non-flaky test that > isn't brittle (i.e. likely to break when Apple fixes the bug). Thoughts? How do > you feel about this kind of flakiness (false negatives but no false positives)? As long as it fails sometimes that I think is sufficient (we'd be able to pick up when it started flaking through buildbot logs if it regressed).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sdy@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": 1484181231585420,
"parent_rev": "07068a979591d47d23299eec9b82f099d6ac460e", "commit_rev":
"145b27b6e14ae6dd557c823e39680d62f16b1cf3"}
Message was sent while issue was closed.
Description was changed from ========== Add a test for our workaround of an AppKit crash in SFChooseIdentityPanel. BUG=679992,653093 ========== to ========== Add a test for our workaround of an AppKit crash in SFChooseIdentityPanel. BUG=679992,653093 Review-Url: https://codereview.chromium.org/2624273003 Cr-Commit-Position: refs/heads/master@{#443090} Committed: https://chromium.googlesource.com/chromium/src/+/145b27b6e14ae6dd557c823e3968... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/145b27b6e14ae6dd557c823e3968... |
