Closed Bug 1447056 Opened 6 years ago Closed 6 years ago

"Removing Cookies and Site Data" dialog buttons are cut off

Categories

(Firefox :: Settings UI, defect, P1)

Product:
Firefox
Component:
Settings UI
Platform:
Unspecified
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 + verified

People

(Reporter: claudijd, Assigned: xidorn)

References

Details

(Keywords: regression, Whiteboard: [storage-v2])

Attachments

(4 files, 1 obsolete file)

Preferences.png
321.58 KB, image/png
Details
Bug 1447056: Don't poorly try to optimize out resizes from OnChromeLoaded.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1447056 part 2 - Invoke Resize in SetSizeConstraints with the current size to apply the new constraints.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1447056 part 1 - Move eWindowType_sheet to be before eWindowType_popup.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Attached image Preferences.png — 
I was trying to view the internal meeting today and it wouldn't work.  I suspected that maybe this was related to bad cookie, so in nightly I went into the cookie manager, attempt to delete the air.mozilla.org cookie, and then upon saving that change, I got into a weird unrecoverable state.

Symptoms:

1.) UI prompt with no ability to accept the change
2.) Cannot close the existing tab
3.) Cannot draw focus of any other other tab

To fix the issue, I needed to quite nightly and restart it.
I am able to reproduce this behavior in the latest nightly version: 61.0a1 (2018-03-19) (64-bit)
Another interesting outcome here, is that the cookie is never actually deleted because you cannot answer the response.  So if a user has a bad cookie, but is unwilling to trash their entire browser cookies whole-hog, this could present a persistent user experience problem.
Yeah, that's pretty bad, I changed that dialog recently but when I last tried it, it was still in pretty good shape. Let's try to find a regression range...
Component: General → Preferences
Keywords: regression, regressionwindow-wanted
Whiteboard: [storage-v2][triage]
Seems like a fresh Nightly regression, Beta is not affected for me.
status-firefox60: --- → unaffected
status-firefox61: --- → affected
Emilio, it seems like your change in bug 1439875 broke this. It's probably a problem with exactly what your patch does, the window doesn't have right size because we're not waiting on the content to layout. There might be a good way to solve this in the front-end.

We can take a look at this together tomorrow if you like. :)

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cad403cafed9b17d1b801837abcd039a0bce82d5&tochange=990a8eb972cd2a734da04ed8d787564752e8c9c7
Blocks: 1439875
Flags: needinfo?(emilio)
Keywords: regressionwindow-wanted
Priority: -- → P1
I can reproduce this on Windows.
Assignee: nobody → emilio
status-firefox59: --- → unaffected
OS: Unspecified → All
Summary: Unrecoverable Firefox UI bug when clearing a specific cookie using the manager UI → stylo: "Removing Cookies and Site Data" dialog buttons are cut off
Hmm, wfm on Linux, I guess this is an OSX/Windows-only issue? But yeah, let's go through it tomorrow... That push was backed out btw, probably should've found the relanding of that bug.

Chances are that the patches for bug 1446264, which affect window restoration, fix this.
I cannot reproduce this on Windows w/ or w/o my patch in bug 1446264... Need to figure out how to reproduce this first :/
Summary: stylo: "Removing Cookies and Site Data" dialog buttons are cut off → "Removing Cookies and Site Data" dialog buttons are cut off
I reproduced this once in my local Nightly install, but cannot reproduce anymore after upgrading (to 2018-03-19), and I cannot reproduce this in my local macOS build either.

Using mozregression, I actually cannot reproduce it on any version from 2018-03-17 to 2018-03-19...

What I did is
Privacy & Security > Cookies and Site Data > Manage Data... > Remove All > Save Changes
and the confirmation window seems to show correctly for me with the two buttons.

Any better steps to reproduce to reliably reproduce this issue?
Flags: needinfo?(jhofmann)
Flags: needinfo?(cpeterson)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #9)
> What I did is
> Privacy & Security > Cookies and Site Data > Manage Data... > Remove All >
> Save Changes
> and the confirmation window seems to show correctly for me with the two
> buttons.
> 
> Any better steps to reproduce to reliably reproduce this issue?

I was only deleting one cookie:

Privacy & Security > Cookies and Site Data > Manage Data... > (select one cookie) > Remove Selected > Save Changes

I can only reproduce the bug if the list of sites in the "Manage Cookies and Site Data" pop-up has at least one site remaining. Since clicking "Remove All" empties the site list, the bug does not reproduce then.
Flags: needinfo?(cpeterson)
Yes, we show a different dialog for deleting all data and deleting individual sites. In the latter case we list the sites in a tree view which seems to be breaking this. Note that removing that tree is tracked in bug 1446368, I'm not sure whether it helps here but I could expedite working on it, just in case...
Flags: needinfo?(jhofmann)
I think I know why this happens... I cannot repro on Linux, but Johann and I can try a patch in a bit.
So this is a window that specifies a width, but not a height. So the code right now relies on the intrinsic height on the window to size the popup, which is set via nsContainerFrame::SetSizeConstraints after layout.

Cocoa (and presumably windows?) are buggy and don't apply the constraints properly in this case unless there's a followup resize, which my patch for bug 1439875 removes. I had a patch written for this already in Cocoa. Not sure if windows will need similar changes.
Flags: needinfo?(emilio)
So, bug 1447292 fixes it on mac, but I think I know why it also fails on windows, and it'd be easier for me to fix it on both.

Here's my theory that I'll confirm tomorrow: https://mozilla.logbot.info/developers/20180322#c14498995
Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Comment on attachment 8961453 [details]
Bug 1447056: Don't poorly try to optimize out resizes from OnChromeLoaded.

https://reviewboard.mozilla.org/r/230206/#review235828

::: commit-message-692c5:3
(Diff revision 1)
> +Bug 1447056: Don't poorly try to optimize out resizes from OnChromeLoaded. r?xidorn
> +
> +The curSize stuff doesn't account for constraints that can be set during layout,

That's not the problem.  The curSize is just the current size, whatever it is.

The problem is that the incoming specified size is not the actual size we want the window to be, but we're comparing it to the actual size the window is.

So I think the commit message should say something like this:

  aSpecWidth and aSpecHeight don't represent the actual width/height we are going to try to size the window to, because they can be affected by size constraints set on mWindow.  So we can't optimize out a resize just because the specified size we have here matches the current size, because we might not be sizing to the given specified size.
  
As a followup, if we feel this optimization is useful (which is not obvious to me), we could apply size constraints to aSpecWidth/Height here before comparing to the current size.  Followup bug for that if you think it's worth doing.
Attachment #8961453 - Flags: review+
Probably related.

In Options - Privacy - Cookies and Site Data - Manage Data...
When having one or more cookies removed, clicking the "Save Changes" button pops up an unclickable dialog.

Screenshot : http://images2.imagebam.com/d9/aa/11/5576ff790738813.jpg
Yeah, probably fixed by this too.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #16)
> As a followup, if we feel this optimization is useful (which is not obvious
> to me), we could apply size constraints to aSpecWidth/Height here before
> comparing to the current size.  Followup bug for that if you think it's
> worth doing.

I don't think it's worth it, all widgets already do that... It saves the persistence call I guess, but it's not clear it's worth optimizing it out.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85bfd88be225
(previously Bug 1446264 part 5) - Avoid ignoring and persisting size/position/sizemode when we are setting them from SizeShell(). r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/1caa948c67f3
Don't poorly try to optimize out resizes from OnChromeLoaded. r=bz
Backout by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ed5c6f25ee
Backed out 2 changesets for bc perma-failures in browser/base/content/test/performance/browser_windowopen_reflows.js on a CLOSED TREE
Attachment #8961453 - Flags: review?(xidorn+moz)
Per discussion with emilio on IRC, the patch in this bug will wallpaper the issue after bug 1446264 lands, and the patch itself can be considered as a harmless optimization in the code path. And I'm going to take this bug to explore the right fix and write a test for it.
Assignee: emilio → xidorn+moz
Comment on attachment 8961620 [details]
Bug 1447056 part 2 - Invoke Resize in SetSizeConstraints with the current size to apply the new constraints.

https://reviewboard.mozilla.org/r/230490/#review235972

::: widget/nsBaseWidget.cpp:1765
(Diff revision 1)
> -  // probably in the middle of a reflow.
> +  // Popups are constrained during layout, and we don't want to synchronously
> +  // paint from reflow, so bail out... This is not great, but it's no worse than
> +  // what we used to do.
> +  //
> +  // The right fix here is probably making constraint changes go through the
> +  // view manager and such.
> +  if (mWindowType == eWindowType_popup) {
> +    return;
> +  }

The logic here is stolen from emilio's patch in bug 1447292.
Comment on attachment 8961620 [details]
Bug 1447056 part 2 - Invoke Resize in SetSizeConstraints with the current size to apply the new constraints.

https://reviewboard.mozilla.org/r/230490/#review235998

::: widget/nsBaseWidget.cpp:1765
(Diff revision 1)
>  
>  void nsBaseWidget::SetSizeConstraints(const SizeConstraints& aConstraints)
>  {
>    mSizeConstraints = aConstraints;
> -  // We can't ensure that the size is honored at this point because we're
> -  // probably in the middle of a reflow.
> +
> +  // Popups are constrained during layout, and we don't want to synchronously

I'm a little confused.  Don't we constrain _everything_ during layout?  The non-popup case is called from DoReflow while we're still working with frames, for example.  And from ConstructRootFrame...

Can we just do the Resize() call off a scriptrunner and avoid all these problems?
Xidorn answered my question on IRC: the non-popup cases don't actually call SetSizeConstraints during reflow; they just schedule it to be called later when safe.
Attachment #8961620 - Flags: review?(bzbarsky)
Attachment #8961619 - Attachment is obsolete: true
Attachment #8961619 - Flags: review?(karlt)
Wait... SizeConstraints and mBounds use the same unit but they are actually different unit?!
Comment on attachment 8961620 [details]
Bug 1447056 part 2 - Invoke Resize in SetSizeConstraints with the current size to apply the new constraints.

https://reviewboard.mozilla.org/r/230490/#review236054

::: widget/nsBaseWidget.cpp:1781
(Diff revision 4)
> +  LayoutDeviceIntSize clampedSize =
> +    Max(aConstraints.mMinSize, Min(aConstraints.mMaxSize, curSize));

We should reuse `ConstrainSize` here... but that's worse on typing, so let's just use Min and Max for now. We may convert that method at the same time in bug 1448283.
Comment on attachment 8961620 [details]
Bug 1447056 part 2 - Invoke Resize in SetSizeConstraints with the current size to apply the new constraints.

https://reviewboard.mozilla.org/r/230490/#review236200

r=me
Attachment #8961620 - Flags: review?(bzbarsky) → review+
Comment on attachment 8961693 [details]
Bug 1447056 part 1 - Move eWindowType_sheet to be before eWindowType_popup.

https://reviewboard.mozilla.org/r/230568/#review236202

::: commit-message-83793:4
(Diff revision 1)
> +Bug 1447056 part 1 - Move eWindowType_sheet to be before eWindowType_popup. r?mstange
> +
> +nsBaseWidget::BoundsUseDesktopPixels() states that window types before
> +eWindowType_sheet take desktop pixels rather than device pixels for

s/eWindowType_sheet/eWindowType_popup/
Attachment #8961693 - Flags: review?(mstange) → review+
MozReview is broken on open issue number again, and I cannot land it to autoland... And pushing to inbound doesn't seem to work on my MBP somehow... :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/52b1ebf43592cfb59916f9fbebdbb5fbbf83cb10
Bug 1447056 part 1 - Move eWindowType_sheet to be before eWindowType_popup. r=mstange

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc2c96c7b4703708738335fd0fdd41980af3913d
Bug 1447056 part 2 - Invoke Resize in SetSizeConstraints with the current size to apply the new constraints. r=bz
Backed out for devtools failures on browser_toolbox_hosts_size.js

backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/e7e45b59cc49a0680de62a5a21d5aef193f91dfc

push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fc2c96c7b4703708738335fd0fdd41980af3913d&selectedJob=170019461

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=170019461&repo=mozilla-inbound&lineNumber=2818

20:48:13     INFO - TEST-START | devtools/client/framework/test/browser_toolbox_hosts_size.js
20:48:14     INFO - TEST-INFO | started process screencapture
20:48:14     INFO - TEST-INFO | screencapture: exit 0
20:48:14     INFO - Buffered messages logged at 20:48:13
20:48:14     INFO - Entering test bound 
20:48:14     INFO - Adding a new tab with URL: data:text/html;charset=utf8,test for host sizes
20:48:14     INFO - Tab added and finished loading
20:48:14     INFO - Console message: [JavaScript Warning: "Unknown property ‘user-select’.  Declaration dropped." {file: "resource://devtools/client/shared/components/reps/reps.css" line: 290}]
20:48:14     INFO - Console message: [JavaScript Warning: "Unknown property ‘user-select’.  Declaration dropped." {file: "resource://devtools/client/shared/components/reps/reps.css" line: 290}]
20:48:14     INFO - Buffered messages finished
20:48:14     INFO - TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_toolbox_hosts_size.js | Opening the toolbox hasn't changed the height of the nbox - Got 957, expected 935
20:48:14     INFO - Stack trace:
20:48:14     INFO - chrome://mochikit/content/browser-test.js:test_is:1280
20:48:14     INFO - chrome://mochitests/content/browser/devtools/client/framework/test/browser_toolbox_hosts_size.js:null:24
20:48:14     INFO - TEST-PASS | devtools/client/framework/test/browser_toolbox_hosts_size.js | Opening the toolbox hasn't changed the width of the nbox - 
20:48:14     INFO - TEST-PASS | devtools/client/framework/test/browser_toolbox_hosts_size.js | The iframe fits within the available space - 
20:48:14     INFO - TEST-PASS | devtools/client/framework/test/browser_toolbox_hosts_size.js | The iframe fits within the available space - 
20:48:14     INFO - Leaving test bound 
20:48:14     INFO - Entering test bound 
20:48:14     INFO - Adding a new tab with URL: data:text/html;charset=utf8,test for host sizes
20:48:15     INFO - Tab added and finished loading
20:48:15     INFO - Console message: [JavaScript Warning: "Unknown property ‘user-select’.  Declaration dropped." {file: "resource://devtools/client/shared/components/reps/reps.css" line: 290}]
20:48:15     INFO - Console message: [JavaScript Warning: "Unknown property ‘user-select’.  Declaration dropped." {file: "resource://devtools/client/shared/components/reps/reps.css" line: 290}]
20:48:15     INFO - TEST-PASS | devtools/client/framework/test/browser_toolbox_hosts_size.js | Opening the toolbox hasn't changed the height of the nbox - 
20:48:15     INFO - TEST-PASS | devtools/client/framework/test/browser_toolbox_hosts_size.js | Opening the toolbox hasn't changed the width of the nbox - 
20:48:15     INFO - TEST-PASS | devtools/client/framework/test/browser_toolbox_hosts_size.js | The iframe is resized properly - 
20:48:15     INFO - TEST-PASS | devtools/client/framework/test/browser_toolbox_hosts_size.js | The iframe is resized properly - 
20:48:16     INFO - Leaving test bound 
20:48:16     INFO - GECKO(794) | MEMORY STAT | vsize 4674MB | residentFast 782MB | heapAllocated 164MB
20:48:16     INFO - TEST-OK | devtools/client/framework/test/browser_toolbox_hosts_size.js | took 2745ms
Flags: needinfo?(xidorn+moz)
After a bunch of debugging... I eventually realize what's happening here.

So the problem this time is in ClientToWindowSize (which is again why it fails on Windows and macOS but not Linux). The problem is that ClientToWindowSize doesn't take drawInTitlebar into consider, as mstange correctly pointed out in bug 1445519 comment 7.

What happens here is that:
* the test tries to set a extremely large size for the toolbox[1], and
* in that case, we clamp the height to height of <notificationbox> around the browser area - min-height of <browser>[2], and set it to height of <iframe> it just creates.

Since that <iframe> is not flex, its preferred size, i.e. its height attribute, is used as its min-height when calculating its XUL min size[3]. So, at this point, the whole window has its size being the minimal intrinsic size, and this intrinsic size certainly includes the part we draw in the titlebar.

Then, we apply this intrinsic size to the window as the size constraints. So far so good.

However, nsContainerFrame::SetSizeConstraints calls ClientToWindowSize to calculate the non-client size, and adds them to the constraints. This makes the min size of the window be taller than before by a titlebar height. And the patch here makes us aggressively apply the new constraints to the window, which uncovers the issue of the size constraints calculation.

This theory can be further confirm via that, when setting browser.tabs.drawInTitlebar to false, the test passes with this patch.


I can see several possible approaches forward:

(1) Fix ClientToWindowSize to make it correctly take drawInTitlebar into consider, but this is effectively undoing bug 1445519 on macOS, so we may need to investigate what's the right thing to do at the callsites of this function.

(2) The current toolbox has its own issue that when you size it to take the maximum area (the browser area hits its min-height), the whole window can no longer be shrunk anymore. This feels quite wrong itself. Maybe we can make it flex so that we don't take hard height on it?

(3) We can probably workaround it in toolbox-hosts.js[2] via further cut the max height further down, so that we don't stretch the window.


In long term, we probably should fix both (1) and (2), but (3) might require the least work at the moment.


[1] https://searchfox.org/mozilla-central/rev/56274d0a016a6833e5da7770ce70580b6f5abb21/devtools/client/framework/test/browser_toolbox_hosts_size.js#16-17
[2] https://searchfox.org/mozilla-central/rev/56274d0a016a6833e5da7770ce70580b6f5abb21/devtools/client/framework/toolbox-hosts.js#71-74
[3] https://searchfox.org/mozilla-central/rev/56274d0a016a6833e5da7770ce70580b6f5abb21/layout/xul/nsSprocketLayout.cpp#1387-1393
Depends on: 1445738
Flags: needinfo?(xidorn+moz)
Depends on: 1448555
(Not meant to cancel the ni?)
Flags: needinfo?(xidorn+moz)
(I'll land it once bug 1448555 lands.)
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(emilio)
Whiteboard: [storage-v2][triage] → [storage-v2]
We're getting more and more dupes of this as people can't clear their cookies in Nightly anymore. It would be great to expedite the fix...
Once bug 1448555 gets fixed, I can land the patches here immediately.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/829f5e202398
part 1 - Move eWindowType_sheet to be before eWindowType_popup. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/150ed033741d
part 2 - Invoke Resize in SetSizeConstraints with the current size to apply the new constraints. r=bz
https://hg.mozilla.org/mozilla-central/rev/829f5e202398
https://hg.mozilla.org/mozilla-central/rev/150ed033741d
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Flags: qe-verify+
Reproduced this issue on Windows 10x64 and MacOS 10.11.5 using nightly version: 61.0a1(2018-03-19)(64-bit).
Verified as fixed with build 61.0b7 accross la platforms.
Status: RESOLVED → VERIFIED
status-firefox61: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: