New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 643123 link

Starred by 24 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug



Sign in to add a comment

sierra: certificate viewer has unconstrained height

Project Member Reported by ellyjo...@chromium.org, Sep 1 2016

Issue description

OS: 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?
 
Screen Shot 2016-09-01 at 6.21.26 AM.png
2.5 MB View Download
Cc: patricia...@chromium.org
 Issue 643124  has been merged into this issue.
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.
This repros on today's canary (55.0.2853.0).
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]
Screen Shot 2016-09-13 at 5.00.04 PM.png
656 KB View Download
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.

Comment 6 by zzat...@gmail.com, Sep 15 2016

I am running macOS Sierra and am able to re-produce this on v53.0.2785.116.
Cc: shrike@chromium.org
Components: Security>UX
Labels: M-54 ReleaseBlock-Stable
Want to make sure this is on our radar for Sierra. Appears it dropped through the cracks of triage. 
Cc: tapted@chromium.org
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?

Comment 9 by tapted@chromium.org, Sep 16 2016

Cc: rsesek@chromium.org
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.
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?

Can we file a radar to make sure it's captured? Once that's done I can escalate to Apple. 
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.
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.
Owner: tapted@chromium.org
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 :/
Cc: sdy@chromium.org
And I guess this means the screenshots in  Issue 645629  were a custom build, not a Canary? (sdy?).

Comment 16 by sdy@chromium.org, 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.

Comment 17 by sdy@chromium.org, Sep 20 2016

See attachment — it's not infinite, just very long.
Screen Shot 2016-09-20 at 1.49.42 AM.png
3.3 MB View Download
Summary: sierra: certificate viewer has unconstrained height (was: sierra: certificate viewer is infinite height and uncloseable)
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.
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.

Comment 20 by mark@chromium.org, 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.

Comment 21 by mark@chromium.org, Sep 20 2016

Cc: mark@chromium.org
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.
Status: Started (was: Assigned)
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.
certsends2
4.9 MB View Download
Wow, that is awful. Nicely done tracking it down :)
Project Member

Comment 24 by bugdroid1@chromium.org, 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

Thanks for the fix, Geetha could you please verify the fix in tomorrows canary.

tapted@ Please request a merge to M54 once its verified.
 Issue 649134  has been merged into this issue.
Labels: Merge-Request-54
Verified that the fix is working as intended in 55.0.2868.0 on 10.12 Sierra. Requesting merge of r420038 to m54

Comment 28 by dimu@chromium.org, Sep 22 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)
Project Member

Comment 29 by bugdroid1@chromium.org, Sep 23 2016

Labels: -merge-approved-54 merge-merged-2840
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

Project Member

Comment 30 by bugdroid1@chromium.org, 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

 Issue 649699  has been merged into this issue.

Comment 32 by mark@chromium.org, 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.
takeasheet.png
43.0 KB View Download
Status: Fixed (was: Started)
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/ :)

Comment 34 by mark@chromium.org, Sep 23 2016

Fair enough. I moved the new ugliness to  bug 649873 .
 Issue 649915  has been merged into this issue.
Cc: mattm@chromium.org ellyjo...@chromium.org
 Issue 650323  has been merged into this issue.
Labels: TE-Verified-54.0.2840.41 TE-Verified-M54
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.
643123.mp4
1.2 MB View Download
 Issue 651222  has been merged into this issue.
 Issue 652055  has been merged into this issue.
 Issue 652619  has been merged into this issue.

Comment 41 by barsh...@gmail.com, 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.
The fix will be available in M54.
Just updated and the issue is gone. Nice work! And thanks for updating this ticket 😘
Project Member

Comment 44 by bugdroid1@chromium.org, 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

Project Member

Comment 45 by bugdroid1@chromium.org, 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

Components: -Security>UX
Labels: Team-Security-UX
Security>UX component is deprecated in favor of the Team-Security-UX label

Comment 47 by mark@chromium.org, Jan 3 2017

I think that the workaround may no longer be necessary as of 10.12.2.

Sign in to add a comment