sierra: certificate viewer has unconstrained height |
|||||||||||||||
Issue descriptionOS: 10.12 Beta 16A313 Chrome: 55.0.2846.0 1) Navigate a tab to https://www.google.com 2) Click the padlock 3) Click the details link 4) Click "view certificate" Something like the attached screenshot happens, and the certificate viewer does not respond to Esc or any other key binding to close. patricialor@, can you take a look at this please?
,
Sep 8 2016
Are you still seeing this? I was able to reproduce it when I tried a couple of days ago on Canary, but after building Chromium locally I can't seem to repro it any more.
,
Sep 8 2016
This repros on today's canary (55.0.2853.0).
,
Sep 14 2016
This is still reproducible on Canary Version 55.0.2859.0 canary (64-bit) & Stable # Version 53.0.2785.113 (64-bit) - Mac OSX 10.12 [Gold Star Version]
,
Sep 14 2016
Yup, so the issue is I can't seem to repro it on a local build, but can on Canary. I tried two combos, mac_views_browser=true with default flags, and mac_views_browser=false with all the mac-views chrome:flags turned on.
,
Sep 15 2016
I am running macOS Sierra and am able to re-produce this on v53.0.2785.116.
,
Sep 15 2016
Want to make sure this is on our radar for Sierra. Appears it dropped through the cracks of triage.
,
Sep 16 2016
Ok, so I tried this again and it is now also reproducible with Chrome 52.0.2743.116 (the current stable release) now on Sierra. tapted@ thinks it might be a macOS bug?
,
Sep 16 2016
I played around a bit. The certificate viewer we invoke is provided by macOS. It probably uses Autolayout which has rumours of regressions. It means that even though the `Details` section is not expanded, the sheet is laid out assuming that it is, and for some reason decides that it doesn't need to scroll. Although scrolling happens if you expand details and move the pane slider down. We might be able to add a workaround to constrain the sheet size (+rsesek do you know if this is a thing?). If we can repro in a standalone NSWindow we should file a radar. But I suspect it's something weird we are doing. For example, Opera seems to use the same dialog as Chrome and its dialog is not broken. Safari isn't affected - it doesn't use the same dialog. Firefox doesn't use the native dialog at all but an ugly one of their own making. We'll have to work on a fix early next week and merge it as far as we can - it's pretty bad. If we can't find a fix, another viable approach (which _might_ (also) fix it) could be to make the sheet window modal, and parent it to the actual browser NSWindow rather than the fake one we add that allows the certificate viewer to be tab modal. There may be dragons.
,
Sep 16 2016
Re: The certificate viewer we invoke is provided by macOS, how do we access this panel? It's hard for me to believe that this isn't a bit of UI we create and display, but apparently that's the case?
,
Sep 16 2016
Can we file a radar to make sure it's captured? Once that's done I can escalate to Apple.
,
Sep 19 2016
Ugh. So this keeps getting weirder. I can't reproduce this in a Chromium build - it works fine there. This will make fixing/testing hard. It might be a you-built-against-SDK-foo thing. But getting versions of XCode willing to build on old SDKs on Sierra is hard.. There could be an upside: Issue 645629 is another bug about the Certificate viewer on Sierra, and the screenshots there show that it is working just fine. So this might be a problem that is specific to the Sierra beta releases. Elly's report and this computer are running 10.12 Beta 16A313, Issue 645629 is running the GM seed (16A323?) Another thing I've noticed, we get an error spat out to stderr on Sierra: 2016-09-19 11:53:03.526 Google Chrome[9037:34617] Failed to connect (_okButton) outlet from (SFCertificatePanel) to (NSButton): missing setter or instance variable This is output on Stable, Canary, and Chromium. I think it's a red-herring wrt this bug, but worth investigating. So next step is probably to get this machine updated to a new beta seed, or switched over to the GM seed to see if this is something Apple have already fixed. We should file a radar to track anyway.
,
Sep 19 2016
M54 Stable release is scheduled during the first week of OCT, please have the fix baked/verified in canary and request a merge to M54 ASAP.
,
Sep 20 2016
I've filed rdar://28380422 . Managed to set up a VM with the 16A323 GM and was able to reproduce (sadly). Built an Official build with XCode 7.3 (which does not support the 10.10 SDK), ran it on the VM and it did not reproduce. So I think this only affects Chrome when built against the 10.10 SDK and run with the 10.12 SDK on Sierra. Since XCode 7.3 refuses to run the 10.10 SDK, I'm attempting to downgrade to XCode 7.2.1 in order to get a custom build of Chrome that uses the 10.10SDK so I can finally start toying with potential workarounds :/
,
Sep 20 2016
And I guess this means the screenshots in Issue 645629 were a custom build, not a Canary? (sdy?).
,
Sep 20 2016
Not a custom build (but sorry, I should've filed this). I was able to make the window large enough to grab the screenshot, and the return key closed it. I don't think esc has ever closed it.
,
Sep 20 2016
See attachment — it's not infinite, just very long.
,
Sep 20 2016
Yah - I said this in the rdar:
When an app linked to the 10.10 SDK allocates an SFCertificatePanel and invokes beginSheetForWindow:.. the panel height is not properly constrained. It appears as though the autolayout logic is broken because the NSScrollView containing the certificate information adopts the entire size of its contents, and does not add scrollbars.
Expected Results:
Should behave the same as El Capitan - Certificate viewer opens with a height constrained to 280 DIP.
Actual Results:
Certificate viewer opens with unconstrained height. Initially certificate is not visible - just the "OK" button at the bottom of the sheet. Resizing the parent window can cause the attached sheet to adjust its position, allowing the top of the panel to be seen ("OK" button goes off-screen). Note that expanding "Details" does not add scrollbars to the details area, but moving the slider down does add scrollbars, so I suspect the scroll view has chosen to size itself to the full/exact size of its contents, when details are expanded.
,
Sep 20 2016
Arrrrgh. And it just happened that the build I made was one from a version still affected by Issue 647241 ("Sierra doesn't like the CreateSSLServerPolicy with no hostname"). So... rebuilding again.
,
Sep 20 2016
Right, it’s tall enough to host its fully-expanded contents. I can hit space-tab-space (FKA is on) and the contents expand so that I wind up seeing something. You can also use Grab to snap the sheet’s window, and you’ll see that there is in fact something in there.
,
Sep 20 2016
Comment 14: Xcode 7.2 is broken, don’t use it. You should be using Xcode 7.3.1. Xcode 8 works as well. Both will claim to not support the 10.10 SDK, but you can fix this by editing Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Info.plist, setting MinimumSDKVersion appropriately. (I use 10.5.) Then, stick the SDKs you need in Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs.
,
Sep 21 2016
Egads that was agony. I think this will "fix" it - https://codereview.chromium.org/2356113002 patchset 0 has an exploration of all the other things I tried. Best approach seems to be to add an override for [NSWindow setFrame:] but even attempting that required a bit of ingenuity... Attaching a bunch of ObjC message tracing around this goop.
,
Sep 21 2016
Wow, that is awful. Nicely done tracking it down :)
,
Sep 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/027233cd43df4596af6b3ba501a15090f1284c05 commit 027233cd43df4596af6b3ba501a15090f1284c05 Author: tapted <tapted@chromium.org> Date: Wed Sep 21 13:43:32 2016 Mac: Hack around Sierra autolayout bug in the certificate viewer. Currently on macOS 10.12 Sierra, invoking the OS-provided SSL certificate viewer used by Chrome will show a window sheet with an improperly constrained height. Tracing shows the autolayout algorithm going berserk - it gets invoked a few thousand times and decides that the NSScrollView containing the certificate details should be sized as though details are expanded, and doesn't add scrollbars. This usually results in an attached panel window much larger than the screen height, which is mostly unusable. This happens when linking to the 10.10 SDK, but not the 10.11 SDK, and only when running against Sierra's 10.12 SDK. To fix, append an override of -[NSWindow setFrame:display:animate] to the certificate viewer's class implementation using the Objective C runtime. This override prevents programmatic changes to the frame from setting a height more than 400 pixels. User-initiated resizes function as usual. BUG= 643123 TEST=On macOS 10.12 Sierra, go to a secure site (e.g. https://www.google.com) and view the SSL certificate (click padlock, click Details, click View Certificate). It should fit within the screen area. On other OSX versions, there should be no change. Review-Url: https://codereview.chromium.org/2356113002 Cr-Commit-Position: refs/heads/master@{#420038} [modify] https://crrev.com/027233cd43df4596af6b3ba501a15090f1284c05/chrome/browser/ui/certificate_viewer_mac.mm
,
Sep 21 2016
Thanks for the fix, Geetha could you please verify the fix in tomorrows canary. tapted@ Please request a merge to M54 once its verified.
,
Sep 21 2016
Issue 649134 has been merged into this issue.
,
Sep 22 2016
Verified that the fix is working as intended in 55.0.2868.0 on 10.12 Sierra. Requesting merge of r420038 to m54
,
Sep 22 2016
Your change meets the bar and is auto-approved for M54 (branch: 2840)
,
Sep 23 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/50f08e85195bab38b86e0fa116a52cf81d23d12a commit 50f08e85195bab38b86e0fa116a52cf81d23d12a Author: Trent Apted <tapted@chromium.org> Date: Thu Sep 22 23:56:39 2016 [merge-m54] Mac: Hack around Sierra autolayout bug in the certificate viewer. Currently on macOS 10.12 Sierra, invoking the OS-provided SSL certificate viewer used by Chrome will show a window sheet with an improperly constrained height. Tracing shows the autolayout algorithm going berserk - it gets invoked a few thousand times and decides that the NSScrollView containing the certificate details should be sized as though details are expanded, and doesn't add scrollbars. This usually results in an attached panel window much larger than the screen height, which is mostly unusable. This happens when linking to the 10.10 SDK, but not the 10.11 SDK, and only when running against Sierra's 10.12 SDK. To fix, append an override of -[NSWindow setFrame:display:animate] to the certificate viewer's class implementation using the Objective C runtime. This override prevents programmatic changes to the frame from setting a height more than 400 pixels. User-initiated resizes function as usual. BUG= 643123 TBR=tapted@chromium.org TEST=On macOS 10.12 Sierra, go to a secure site (e.g. https://www.google.com) and view the SSL certificate (click padlock, click Details, click View Certificate). It should fit within the screen area. On other OSX versions, there should be no change. Review-Url: https://codereview.chromium.org/2356113002 Cr-Commit-Position: refs/heads/master@{#420038} (cherry picked from commit 027233cd43df4596af6b3ba501a15090f1284c05) Review URL: https://codereview.chromium.org/2363823003 . Cr-Commit-Position: refs/branch-heads/2840@{#502} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/50f08e85195bab38b86e0fa116a52cf81d23d12a/chrome/browser/ui/cocoa/certificate_viewer_mac.mm
,
Sep 23 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5d43ce01b8b4ac162661315043d28c2d043892ae commit 5d43ce01b8b4ac162661315043d28c2d043892ae Author: Trent Apted <tapted@chromium.org> Date: Fri Sep 23 01:20:08 2016 [m54] Fix build breakage from branch commit 50f08e85195bab38b86e0fa116a52cf81d23d12a Methods in mac_util.h got renamed in m55. BUG= 643123 TBR=tapted@chromium.org Review URL: https://codereview.chromium.org/2366823002 . Cr-Commit-Position: refs/branch-heads/2840@{#507} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/5d43ce01b8b4ac162661315043d28c2d043892ae/chrome/browser/ui/cocoa/certificate_viewer_mac.mm
,
Sep 23 2016
Issue 649699 has been merged into this issue.
,
Sep 23 2016
This is “fixed” now in that we can see the whole sheet and interact with it, but things still don’t look great on 10.12. What’s all that white space above Details? (Is it…a clue?) Sticking this here instead of filing a new bug because it’s probably all related.
,
Sep 23 2016
Yah it's not ideal. The problem goes away if we link Chrome with the 10.11 SDK instead of 10.10. I think that's the best "proper" fix, if there's no activity from Apple on rdar://28380422 . I tried poking autolayout properties on the OS-supplied view hierarchy without success - see https://codereview.chromium.org/2356113002/#ps1 . I think it will be too hard to try to fix the autolayout bugs without access to the source. I'll move this to Fixed so it drops off the ReleaseBlock list for m54. It's still pretty bad having this in m53, but probably not worth a new release. Also: https://xkcd.com/1479/ :)
,
Sep 23 2016
Fair enough. I moved the new ugliness to bug 649873 .
,
Sep 24 2016
Issue 649915 has been merged into this issue.
,
Sep 27 2016
,
Sep 28 2016
Tested the issue on MacOS Sierra 10.12 using chrome version 54.0.2840.41.Observed the certificate is rendered fine as like screen shot mentioned in comment #32.Please find the attached screen cast for the same. Adding TE-Verified labels.
,
Sep 29 2016
Issue 651222 has been merged into this issue.
,
Oct 3 2016
Issue 652055 has been merged into this issue.
,
Oct 4 2016
Issue 652619 has been merged into this issue.
,
Oct 19 2016
I was able to reproduce this both in Chrome 53.0.2785.143 (64-bit), and oddly enough in Microsoft Outlook 2016. It could be a macOS issue.
,
Oct 19 2016
The fix will be available in M54.
,
Oct 20 2016
Just updated and the issue is gone. Nice work! And thanks for updating this ticket 😘
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/50f08e85195bab38b86e0fa116a52cf81d23d12a commit 50f08e85195bab38b86e0fa116a52cf81d23d12a Author: Trent Apted <tapted@chromium.org> Date: Thu Sep 22 23:56:39 2016 [merge-m54] Mac: Hack around Sierra autolayout bug in the certificate viewer. Currently on macOS 10.12 Sierra, invoking the OS-provided SSL certificate viewer used by Chrome will show a window sheet with an improperly constrained height. Tracing shows the autolayout algorithm going berserk - it gets invoked a few thousand times and decides that the NSScrollView containing the certificate details should be sized as though details are expanded, and doesn't add scrollbars. This usually results in an attached panel window much larger than the screen height, which is mostly unusable. This happens when linking to the 10.10 SDK, but not the 10.11 SDK, and only when running against Sierra's 10.12 SDK. To fix, append an override of -[NSWindow setFrame:display:animate] to the certificate viewer's class implementation using the Objective C runtime. This override prevents programmatic changes to the frame from setting a height more than 400 pixels. User-initiated resizes function as usual. BUG= 643123 TBR=tapted@chromium.org TEST=On macOS 10.12 Sierra, go to a secure site (e.g. https://www.google.com) and view the SSL certificate (click padlock, click Details, click View Certificate). It should fit within the screen area. On other OSX versions, there should be no change. Review-Url: https://codereview.chromium.org/2356113002 Cr-Commit-Position: refs/heads/master@{#420038} (cherry picked from commit 027233cd43df4596af6b3ba501a15090f1284c05) Review URL: https://codereview.chromium.org/2363823003 . Cr-Commit-Position: refs/branch-heads/2840@{#502} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/50f08e85195bab38b86e0fa116a52cf81d23d12a/chrome/browser/ui/cocoa/certificate_viewer_mac.mm
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5d43ce01b8b4ac162661315043d28c2d043892ae commit 5d43ce01b8b4ac162661315043d28c2d043892ae Author: Trent Apted <tapted@chromium.org> Date: Fri Sep 23 01:20:08 2016 [m54] Fix build breakage from branch commit 50f08e85195bab38b86e0fa116a52cf81d23d12a Methods in mac_util.h got renamed in m55. BUG= 643123 TBR=tapted@chromium.org Review URL: https://codereview.chromium.org/2366823002 . Cr-Commit-Position: refs/branch-heads/2840@{#507} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/5d43ce01b8b4ac162661315043d28c2d043892ae/chrome/browser/ui/cocoa/certificate_viewer_mac.mm
,
Dec 9 2016
Security>UX component is deprecated in favor of the Team-Security-UX label
,
Jan 3 2017
I think that the workaround may no longer be necessary as of 10.12.2. |
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by shrike@chromium.org
, Sep 2 2016