Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2532203005:
Work around an AppKit bug that can crash Chrome after using the client certificate prompt. (Closed)
|
Created:
4 years ago by Sidney San Martín Modified:
4 years ago Reviewers:
Robert Sesek CC:
chromium-reviews, mac-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWork around an AppKit bug that can crash Chrome after using the client certificate prompt.
BUG=653093
Committed: https://crrev.com/892a81ed3ba0c0ec6a2a5c79f1b91a3759be8c54
Cr-Commit-Position: refs/heads/master@{#435335}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Address first round of comments, use ObjC fast enumeration instead of a range-based for loop. #Patch Set 3 : Drop __unsafe_unretained, add comments, dispatch_async() → PostTask() #Patch Set 4 : Clearer comment #Messages
Total messages: 22 (13 generated)
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Description was changed from ========== Work around an AppKit bug that can crash Chrome after using the client certificate prompt. BUG= ========== to ========== Work around an AppKit bug that can crash Chrome after using the client certificate prompt. BUG=653093 ==========
sdy@chromium.org changed reviewers: + rsesek@chromium.org
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/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm (right): https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:115: // NSTableView.dataSource becomes a zeroing weak reference starting in 10.11, Use the availability macros to only do this work if our SDK is <10.11 maybe? https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:119: // BUG=653093 Make this a https://crbug.com/ URL please. (Could collapse with the rdar line like, "See https://crbug.com/x and rdar://y for more information"). https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:122: if (auto tableView = base::mac::ObjCCast<NSTableView>(view)) { naming: table_view https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:132: __unsafe_unretained NSWindow* leakedWindow) { Is the __unsafe_unretained so the block doesn't grab a ref? Comment. https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:132: __unsafe_unretained NSWindow* leakedWindow) { naming: leaked_window https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:133: dispatch_async(dispatch_get_main_queue(), ^{ Why does this need to be done in dispatch_async()? Leave a comment if it's necessary. Though I'd probably have used PostTask from within -sheetDidEnd:.
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
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/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm (right): https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:115: // NSTableView.dataSource becomes a zeroing weak reference starting in 10.11, On 2016/11/29 at 21:31:36, Robert Sesek wrote: > Use the availability macros to only do this work if our SDK is <10.11 maybe? Done. https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:119: // BUG=653093 On 2016/11/29 at 21:31:35, Robert Sesek wrote: > Make this a https://crbug.com/ URL please. (Could collapse with the rdar line like, "See https://crbug.com/x and rdar://y for more information"). Done. https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:122: if (auto tableView = base::mac::ObjCCast<NSTableView>(view)) { On 2016/11/29 at 21:31:35, Robert Sesek wrote: > naming: table_view Done. https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:132: __unsafe_unretained NSWindow* leakedWindow) { On 2016/11/29 at 21:31:35, Robert Sesek wrote: > naming: leaked_window Done. https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:132: __unsafe_unretained NSWindow* leakedWindow) { On 2016/11/29 21:31:35, Robert Sesek wrote: > Is the __unsafe_unretained so the block doesn't grab a ref? Comment. That's the idea. Without ARC it's a noop, but I think it serves as good documentation/future proofing if we do switch on ARC. https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:133: dispatch_async(dispatch_get_main_queue(), ^{ On 2016/11/29 21:31:35, Robert Sesek wrote: > Why does this need to be done in dispatch_async()? Leave a comment if it's > necessary. Though I'd probably have used PostTask from within -sheetDidEnd:. At the call site, the window is still in an autorelease pool. The dispatch_async() lets us skip the rest of the workaround if/when Apple fixes the leak. Could you tell me more about PostTask? I haven't used it yet.
https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm (right): https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:132: __unsafe_unretained NSWindow* leakedWindow) { On 2016/11/29 22:55:07, Sidney San Martín wrote: > On 2016/11/29 21:31:35, Robert Sesek wrote: > > Is the __unsafe_unretained so the block doesn't grab a ref? Comment. > > That's the idea. Without ARC it's a noop, but I think it serves as good > documentation/future proofing if we do switch on ARC. I don't think this is a practice we currently follow (and I don't think a Mac ARC switch is really on the horizon), but I don't feel strongly against it. https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:133: dispatch_async(dispatch_get_main_queue(), ^{ On 2016/11/29 22:55:07, Sidney San Martín wrote: > On 2016/11/29 21:31:35, Robert Sesek wrote: > > Why does this need to be done in dispatch_async()? Leave a comment if it's > > necessary. Though I'd probably have used PostTask from within -sheetDidEnd:. > > At the call site, the window is still in an autorelease pool. The > dispatch_async() lets us skip the rest of the workaround if/when Apple fixes the > leak. > > Could you tell me more about PostTask? I haven't used it yet. It's Chrome's task posting system. You can use it to post a closure to the current thread like so: base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, base::Bind(ClearTableViewDataSourcesIfWindowStillAround, base::Unretained(sheet))); I am a little hesitant about using dispatch_async() for something like this, since blocks get processed by the runloop in several places, and when the autorelease pool drains is not defined. We have a better sense of when MessageLoop/PostTask work runs, especially with respect to the autorelease pool.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm (right): https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:132: __unsafe_unretained NSWindow* leakedWindow) { On 2016/11/30 00:23:32, Robert Sesek wrote: > On 2016/11/29 22:55:07, Sidney San Martín wrote: > > On 2016/11/29 21:31:35, Robert Sesek wrote: > > > Is the __unsafe_unretained so the block doesn't grab a ref? Comment. > > > > That's the idea. Without ARC it's a noop, but I think it serves as good > > documentation/future proofing if we do switch on ARC. > > I don't think this is a practice we currently follow (and I don't think a Mac > ARC switch is really on the horizon), but I don't feel strongly against it. That's fair — and base::Unretained also gets that point across. Removed. https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:133: dispatch_async(dispatch_get_main_queue(), ^{ On 2016/11/30 00:23:32, Robert Sesek wrote: > On 2016/11/29 22:55:07, Sidney San Martín wrote: > > On 2016/11/29 21:31:35, Robert Sesek wrote: > > > Why does this need to be done in dispatch_async()? Leave a comment if it's > > > necessary. Though I'd probably have used PostTask from within -sheetDidEnd:. > > > > At the call site, the window is still in an autorelease pool. The > > dispatch_async() lets us skip the rest of the workaround if/when Apple fixes > the > > leak. > > > > Could you tell me more about PostTask? I haven't used it yet. > > It's Chrome's task posting system. You can use it to post a closure to the > current thread like so: > > base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, > base::Bind(ClearTableViewDataSourcesIfWindowStillAround, > base::Unretained(sheet))); > > I am a little hesitant about using dispatch_async() for something like this, > since blocks get processed by the runloop in several places, and when the > autorelease pool drains is not defined. We have a better sense of when > MessageLoop/PostTask work runs, especially with respect to the autorelease pool. Done (mostly). If I get what you're saying, then moving the PostTask() into -sheetDidEnd: would require a second SDK check. Is that what you meant?
LGTM https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm (right): https://codereview.chromium.org/2532203005/diff/1/chrome/browser/ui/cocoa/ssl... chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:133: dispatch_async(dispatch_get_main_queue(), ^{ On 2016/11/30 04:19:50, Sidney San Martín wrote: > On 2016/11/30 00:23:32, Robert Sesek wrote: > > On 2016/11/29 22:55:07, Sidney San Martín wrote: > > > On 2016/11/29 21:31:35, Robert Sesek wrote: > > > > Why does this need to be done in dispatch_async()? Leave a comment if it's > > > > necessary. Though I'd probably have used PostTask from within > -sheetDidEnd:. > > > > > > At the call site, the window is still in an autorelease pool. The > > > dispatch_async() lets us skip the rest of the workaround if/when Apple fixes > > the > > > leak. > > > > > > Could you tell me more about PostTask? I haven't used it yet. > > > > It's Chrome's task posting system. You can use it to post a closure to the > > current thread like so: > > > > base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, > > base::Bind(ClearTableViewDataSourcesIfWindowStillAround, > > base::Unretained(sheet))); > > > > I am a little hesitant about using dispatch_async() for something like this, > > since blocks get processed by the runloop in several places, and when the > > autorelease pool drains is not defined. We have a better sense of when > > MessageLoop/PostTask work runs, especially with respect to the autorelease > pool. > > Done (mostly). If I get what you're saying, then moving the PostTask() into > -sheetDidEnd: would require a second SDK check. Is that what you meant? It is, but what you did is fine as well.
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": 60001, "attempt_start_ts": 1480527835016070,
"parent_rev": "183bedf51615cb2ba172531d1155320b5657ac74", "commit_rev":
"4ae6f6dd58240e4d6ba397b5e06fe198ea43fcdb"}
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Work around an AppKit bug that can crash Chrome after using the client certificate prompt. BUG=653093 ========== to ========== Work around an AppKit bug that can crash Chrome after using the client certificate prompt. BUG=653093 Committed: https://crrev.com/892a81ed3ba0c0ec6a2a5c79f1b91a3759be8c54 Cr-Commit-Position: refs/heads/master@{#435335} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/892a81ed3ba0c0ec6a2a5c79f1b91a3759be8c54 Cr-Commit-Position: refs/heads/master@{#435335} |
