1424311 - Clicking Get Certificate causes the Add Security Exception window to expand indefinitely

Status: VERIFIED FIXED

(Core :: Security: PSM, defect, P1)

Description

[joemaffei]

Attachment: add-security-exception-bug.gif

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce:

- Go to a URL that triggers the "Your connection is not secure" (like http://expired.badssl.com)
- click Advanced, then Add Exception
- every time you click the Get Certificate button the Add Security Exception window gets bigger -- and it stays that way even after closing/reopening the browser!


Actual results:

The Add Security Exception window gets bigger every time you click the Get Certificate button.


Expected results:

The Add Security Exception window should not get bigger every time you click the Get Certificate button.

Comment 1

[YF (Yang)]

I cannot reproduce this issue in Fx57, 58 and Nightly 59 on Win10 1709. However, I recently encountered the height of the window was exceeded the screen height and had to remove xulstore.json for fix it.

Comment 2

[gengesa]

I can reproduce this issue by clicking "View" button (opening "Certificate Viewer") before clicking "Get Certificate" button.

Comment 3

[YF (Yang)]

This issue can be reproduced based on comment 2. This issue will eventually make the dialog unusable.

Comment 4

[YF (Yang)]

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=167ed7d3545b6b8ff0c8fee6593a505bd181b08b&tochange=67e5ea975d53131939ef6d399ff1558fd9475c0f

Comment 7

[Ryan VanderMeulen [:RyanVM]]

This has picked up a couple dupes now. Is this something we may want to bump up higher than P5?

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: bug 1424311 - don't persist the certificate exception dialog's size so it doesn't grow unboundedly

window.sizeToContent() apparently interacts poorly with windows that have a
persisted size (see bug 90276, which is a 5-digit bug that hasn't been touched
in over a decade). As a workaround, don't persist the certificate exception
dialog's size. This means we have to call window.sizeToContent() more often and
unfortunately results in the window growing and shrinking again on Windows, but
at least it will always be the "right size" for its content.

Review commit: https://reviewboard.mozilla.org/r/232248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232248/

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler]]

This will make it better. Still not perfect, but at least the dialog will be usable.

Comment 10

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8963346 [details]
bug 1424311 - don't persist the certificate exception dialog's size so it doesn't grow unboundedly

https://reviewboard.mozilla.org/r/232248/#review237710

This seems reasonable to me, but tbh I'm not super familiar with how this part of XUL works. I suppose we can try it out on Nightly though.

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Thanks for the review.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02213fd3a9c8
don't persist the certificate exception dialog's size so it doesn't grow unboundedly r=jcj

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/02213fd3a9c8

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Thanks for fixing this, David! Please nominate this for Beta approval when you get a chance so it makes the next ESR.

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8963346 [details]
bug 1424311 - don't persist the certificate exception dialog's size so it doesn't grow unboundedly

Approval Request Comment
[Feature/Bug causing the regression]: bug 1347859
[User impact if declined]: add certificate exception dialog can become unusable eventually
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]:  yes:

1. visit https://expired.badssl.com
2. click "advanced"
3. click "add exception"
4. click "view"
5. close the dialog that comes up (the certificate viewer)
6. click "get certificate" repeatedly
Step 6 should not cause the dialog to keep growing in size

[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: limited change, doesn't affect core functionality of this dialog, just the display of it
[String changes made/needed]: none

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8963346 [details]
bug 1424311 - don't persist the certificate exception dialog's size so it doesn't grow unboundedly

Approved for 60.0b9, thanks.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/54d48c08e4c9

Comment 18

[Catalin Sasca, Desktop Test Engineering [:csasca]]

I successfully reproduced the issue on Nightly 59.0a1 (2017-12-08) under Windows 10 (x64) using the STR from Comment 0 and Comment 2.

The issue is no longer reproducible on Firefox 60.0b9 and latest Nightly 61.0a1 (2018-04-03) under Windows 10 (x64). I also checked on Ubuntu 16.04 (x64) and macOS 10.12 and everything is working as expected.

Comment 21

[Chris]

The update means that the dialog no longer grows larger, but it does not reset the dialog to the original size and does not allow the user to manually resize. This means that if you are already afflicted with this problem then this update does not fix it. It only prevents it from getting worse or impacting new users. Running 61.01 x64 Win10.

I've followed the instructions on #1389547 which through trial and error I worked out you have to quit Mozilla before you make the change to xulstore.json :

chrome://pippki/content/exceptionDialog.xul":{"exceptiondialog":{"screenX":"132","screenY":"8","width":"955","height":"200"}}

But it would be helpful if a sane exceptiondialog height in newversion(x) or resetting it back to the default if <> default was implemented.

Comment 22

[Neustradamus]

Thanks chris for your comment, one of my very very old bug is now solved with your change (there was xulstore.json removal possibility too).