Bookmark This Page panel hangs Firefox when -moz-border-radius is used

RESOLVED FIXED

Status

()

Core
Widget: Win32
P1
critical
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Alfred Kayser, Assigned: masayuki)

Tracking

({crash, hang, verified1.9.0.4})

Trunk
x86
Windows XP
crash, hang, verified1.9.0.4
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
Bookmark This Page panel hangs Firefox when -moz-border-radius is used.
In my themes I specify a rounded corner for the Bookmark This Page panel:
#editBookmarkPanel{
  padding:4px;
  min-width:24em;
  -moz-border-radius:6px 0 6px 6px}

But when the panel is openend and then closed with 'Cancel' Firefox completely hangs. One can only close it via the taskbar or task manager of Windows.

(My themes that are impacted are: LittleFox, Nautipolis, Walnut and W3v8)

I would say this is blocking FF3.1, as it is a regression and can quite easily hang or even crash FF on themes that are widely used for FF3.0 (and 2.0).

While I can remove this rounding, the overall hang of the complete application because of a CSS style rule is worrying...

This problem is not in FF3.0 (luckily!), but it is really prominent in the 3.1 nightlies.
Flags: blocking1.9.1?
(Reporter)

Comment 1

10 years ago
Could this be caused by: Bug 405421 – Ensure our widget painting is transparent for themes that support it
I can confirm this with my theme. My themes Larry panel also with -moz-border-radius works without problems.
Created attachment 335786 [details]
screenshot

I also reproducible with mozilla-central {Build ID: 20080827132451}/Windows 2000.

1. add the following code to userChrome.css
#editBookmarkPanel{ -moz-border-radius: 5px !important }
2. open Bookmark This Page panel
3. press Cancel button
4. hang
(Reporter)

Comment 5

10 years ago
If bug 433340 is the cause, then bug 451015 has probably the fix.
Alfred, if this bug is a regression from bug 433340 and the fix happens in bug 451015, please nominate that bug for blocking 1.9.0.3.
Flags: blocking1.9.0.3?

Comment 7

10 years ago
(In reply to comment #5)
> If bug 433340 is the cause, then bug 451015 has probably the fix.

This will not be fixed by bug 451015 because Bookmark This Page panel is *not* already topmost window.

When this bug occurs, browser window gets WS_EX_LAYERED wrongly.
Maybe hWnd is wrong at http://hg.mozilla.org/mozilla-central/index.cgi/annotate/5e208ee3e1cc/widget/src/windows/nsWindow.cpp#l7992


looks like that if I cannot fix this soon, we should back out the patch of bug 43340 from 1.9.0 branch.
oops, I meant bug 433340.
Created attachment 335906 [details] [diff] [review]
Patch v1.0

O.k. This fixes this bug.

The our TopLevelWindow definition doesn't include popup which has parent (then, the panel is a non-topmost window). We should add new param to |GetTopLevelWindow|, |GetTopLevelHWND| and |GetParent(PRBool)| which name is "aStopOnFirstPopup". And GetParent(PRBool) is too unreadable. The name is used in nsIWidget and Win32 API. Therefore, I rename it to "GetPraentInternal".
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #335906 - Flags: review?(emaijala)
Er, I'm not sure whether this change is correct.

>  NS_IMETHODIMP nsWindow::HideWindowChrome(PRBool aShouldHide)
>  {
> -  HWND hwnd = GetTopLevelHWND(mWnd);
> +  HWND hwnd = GetTopLevelHWND(mWnd, PR_TRUE);

By this change, if the XUL element is in popuped panel, the hwnd is the popup window. Otherwise, if the second param is PR_FALSE, the hwnd is the owner window of the popup window. I'm not sure nsIWidget::HideWindowChrome should affect to which window. Deakin, do you know the correct behavior of it?
(Assignee)

Updated

10 years ago
Blocks: 433340, 451015
I think that we have 2 ways for 1.9.0 branch.

1. Back out the patch of bug 433340 from 1.9.0.2.
2. Land this fix to 1.9.0.2.
Flags: blocking1.9.0.2?
We need a test for this as well so it doesn't happen again.
Flags: in-testsuite?
Masayuki, before we block on this and have to respin, can you confirm it actually happens on 1.9.0? Comment 0 implies that it doesn't.
(In reply to comment #14)
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.3pre) Gecko/2008082806 GranParadiso/3.0.3pre

Reproducible on 1.9.0
Backed out the patch for bug 433340 on CVS HEAD and GECKO190_20080827_RELBRANCH.

mozilla/browser/base/content/browser.xul 	1.467.2.1
mozilla/browser/base/content/browser.xul 	1.468 
Keywords: fixed1.9.0.2, fixed1.9.0.3
Gonna be pretty aggressive and not mark this as blocking even though I think it's basically right on the edge of falling onto blocking status (shouldn't be able to hang the browser with this type of css)... but given that it's chrome only it shouldn't block.  Regardless, we should fix this for 1.9.1.
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Priority: -- → P1
(Assignee)

Updated

10 years ago
Attachment #335906 - Flags: review?(emaijala) → review-
Comment on attachment 335906 [details] [diff] [review]
Patch v1.0

Canceling this patch. It should be simpler than this... I try to think the better patch.
Flags: blocking1.9.0.3?
Flags: blocking1.9.0.3+
Flags: blocking1.9.0.2?
Flags: blocking1.9.0.2+
Created attachment 336080 [details] [diff] [review]
Patch v2.0

This patch is clearer than the previous one. I changed some method and param names.

And I also change the result of ChildWindow::WindowStyle(). If WS_POPUP is in the result, we must not return WS_CHILD.
Attachment #335906 - Attachment is obsolete: true
Attachment #336080 - Flags: review?(emaijala)
(In reply to comment #11)
> Er, I'm not sure whether this change is correct.
> 
> >  NS_IMETHODIMP nsWindow::HideWindowChrome(PRBool aShouldHide)
> >  {
> > -  HWND hwnd = GetTopLevelHWND(mWnd);
> > +  HWND hwnd = GetTopLevelHWND(mWnd, PR_TRUE);
> 
> By this change, if the XUL element is in popuped panel, the hwnd is the popup
> window. Otherwise, if the second param is PR_FALSE, the hwnd is the owner
> window of the popup window. I'm not sure nsIWidget::HideWindowChrome should
> affect to which window.

This should be ok. Becasue HideWindowChrome is called only from window elements of XUL. Therefore, the hwnd never becomes the handle of popup window.

Updated

10 years ago
Attachment #336080 - Flags: review?(emaijala) → review-

Comment 21

10 years ago
Comment on attachment 336080 [details] [diff] [review]
Patch v2.0

-HWND nsWindow::GetTopLevelHWND(HWND aWnd, PRBool aStopOnFirstTopLevel)
+HWND nsWindow::GetTopLevelHWND(HWND aWnd, PRBool aStopOnNonChildWindow)

In my opinion aStopOnFirstTopLevel was clearer than aStopOnNonChildWindow.

+      // Note that WS_CHILD and WS_POPUP should not be in same window.
+      // But actually, it's possible.

Is it anymore as the patch has a fix for that too? If there are still such cases, they need to be fixed rather than this code be made more complicated.


+nsWindow* nsWindow::GetParentInTopLevelWindow(PRBool aStopOnFloatingWindow)

These names and the call hierarchy is getting a bit confusing. How about naming it just GetParentWindow and moving the check for dialog or popup into GetTopLevelWindow? That seems to be the only place where it's actually necessary.

+  DWORD style = WS_CHILD | WS_CLIPCHILDREN | nsWindow::WindowStyle();
+  if (style & WS_POPUP)
+    style &= ~WS_CHILD; // WS_POPUP and WS_CHILD should be in same window.

How about doing it the other way? This would be clearer (not sure if the comment is necessary, but it was saying the opposite):

+  DWORD style = WS_CLIPCHILDREN | nsWindow::WindowStyle();
+  if (!(style & WS_POPUP))
+    style &= WS_CHILD; // WS_POPUP and WS_CHILD are mutually exclusive.

+  // even if the current focused content is in popuped window.

Make that "even if the content of the popup window has focus." or such.

+nsWindow* nsWindow::GetTopLevelWindow(PRBool aStopOnFloatingWindow)

Make the parameter name e.g. aStopOnDialogOrPopup.
Thank you, Ere.

(In reply to comment #21)
> (From update of attachment 336080 [details] [diff] [review])
> -HWND nsWindow::GetTopLevelHWND(HWND aWnd, PRBool aStopOnFirstTopLevel)
> +HWND nsWindow::GetTopLevelHWND(HWND aWnd, PRBool aStopOnNonChildWindow)
> 
> In my opinion aStopOnFirstTopLevel was clearer than aStopOnNonChildWindow.

When the second parameter is true, this may return popup window that is not a top level window. Is |aStopOnDialogOrPopup| a better name, like |nsWindow::GetTopLevelWindow|?

Comment 23

10 years ago
(In reply to comment #22)
> When the second parameter is true, this may return popup window that is not a
> top level window. Is |aStopOnDialogOrPopup| a better name, like
> |nsWindow::GetTopLevelWindow|?

The problem here is that the definition of a top level window is somewhat blurry, so a popup could be considered a top level window, but a child window couldn't. So saying "asking for a top level window that's not a child window" doesn't make sense. I understand you meant WS_CHILD, but that's still easily confusing. I guess aStopOnDialogOrPopup would be ok.
Created attachment 336663 [details] [diff] [review]
Patch v3.0
Attachment #336080 - Attachment is obsolete: true
Attachment #336663 - Flags: review?(emaijala)
(Assignee)

Updated

10 years ago
Duplicate of this bug: 453306

Comment 26

10 years ago
When "-moz-appearance" is "none", and sets "background" to translucent color(for example rgba(68,68,68,0.8)), panel background isn't translucent. When user click "Cancel" button, Firefox 3.1 also hangs.

#editBookmarkPanel {
  -moz-appearance: none;
  background: rgba(68,68,68,0.8);
  border: 1px solid rgba(255,255,255,0.15);
  padding: 10px 8px 6px 8px;
  margin-top: 4px;
  color: #ffffff;
}
(In reply to comment #21)
> +      // Note that WS_CHILD and WS_POPUP should not be in same window.
> +      // But actually, it's possible.
> Is it anymore as the patch has a fix for that too? If there are still such
> cases, they need to be fixed rather than this code be made more complicated.
Some broken embedders may have invalid window styles which are out of our contorl.

Comment 28

10 years ago
(In reply to comment #27)
> Some broken embedders may have invalid window styles which are out of our
> contorl.

How likely is it that there is a broken embedder that calls our GetTopLevelHWND() for such window? And should we even try to cater for that? I'd say no unless there are compelling reasons to do that.

Updated

10 years ago
Attachment #336663 - Flags: review?(emaijala) → review+
(Assignee)

Updated

10 years ago
Attachment #336663 - Flags: superreview?(vladimir)
Vlad:

Would you sr the patch?
Attachment #336663 - Flags: superreview?(vladimir) → superreview+
checked-in to trunk, thank you.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Duplicate of this bug: 454404
Comment on attachment 336663 [details] [diff] [review]
Patch v3.0

let's land this patch to 1.9.0.3.
Attachment #336663 - Flags: approval1.9.0.3?
Flags: blocking1.9.0.4+ → blocking1.9.0.3+
Keywords: fixed1.9.0.4 → fixed1.9.0.3
Sorry for the noise. fixed1.9.0.4 was correct after all since the point is to get main-branch verification separate from relbranch verification. 1.9.0.3 is still the same relbranch as 1.9.0.2

This bug has gotten confusing in that it was "fixed" in comment 16 by a backout of another bug, and another 16 comments later there's a patch to check in to fix it again? Shouldn't we have reopened bug 433340 instead and then fixed this there?
Flags: blocking1.9.0.3+ → blocking1.9.0.4+
Keywords: fixed1.9.0.3 → fixed1.9.0.4
(In reply to comment #33)
> This bug has gotten confusing in that it was "fixed" in comment 16 by a backout
> of another bug, and another 16 comments later there's a patch to check in to
> fix it again? Shouldn't we have reopened bug 433340 instead and then fixed this
> there?

I don't think so, the backing out didn't fix the actual bug. It only hides the bug from our eyes :-(
I remove the keywords from this bug.

And this is not a regression bug.
Severity: blocker → critical
Keywords: fixed1.9.0.2, fixed1.9.0.4, regression
(Reporter)

Comment 35

10 years ago
It is a regression as FF3.0 (and older) don't have this behavior of locking the browser when radius is used.
Flags: blocking1.9.0.4+
Flags: blocking1.9.0.2+
Comment on attachment 336663 [details] [diff] [review]
Patch v3.0

Approved for 1.9.0.4, a=dveditz for release-drivers
Attachment #336663 - Flags: approval1.9.0.4? → approval1.9.0.4+
checked-in to 1.9.0 branch.
Keywords: fixed1.9.0.4
Created attachment 341928 [details] [diff] [review]
Patch for ci to 1.9.0 branch
Verified for 1.9.0.4 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.4pre) Gecko/2008102306 GranParadiso/3.0.4pre.
Keywords: fixed1.9.0.4 → verified1.9.0.4

Updated

10 years ago
Duplicate of this bug: 446491
You need to log in before you can comment on or make changes to this bug.