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

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P1)

Description

[Alfred Kayser]

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.

Comment 1

[Alfred Kayser]

Could this be caused by: Bug 405421 – Ensure our widget painting is transparent for themes that support it

Comment 2

[Richard Marti (:Paenglab)]

I can confirm this with my theme. My themes Larry panel also with -moz-border-radius works without problems.

Comment 3

[Ria Klaassen (not reading all bugmail)]

Maybe bug 433340? 

Comment 4

[AKIHIRO Misaki (a.k.a Kuden)]

Attachment: 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

Comment 5

[Alfred Kayser]

If bug 433340 is the cause, then bug 451015 has probably the fix.

Comment 6

[Samuel Sidler (old account; do not CC)]

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.

Comment 7

[Atsushi Sakai]

(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

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

looks like that if I cannot fix this soon, we should back out the patch of bug 43340 from 1.9.0 branch.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

oops, I meant bug 433340.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: 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".

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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?

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 13

[Samuel Sidler (old account; do not CC)]

We need a test for this as well so it doesn't happen again.

Comment 14

[Samuel Sidler (old account; do not CC)]

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.

Comment 15

[AKIHIRO Misaki (a.k.a Kuden)]

(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

Comment 16

[:Gavin Sharp [email: [e-mail]]]

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 

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: 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.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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.

Comment 21

[Ere Maijala (slow)]

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.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

[Ere Maijala (slow)]

(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.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.0

Comment 26

[Ji Shiping]

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;
}

Comment 27

[Masatoshi Kimura [:emk]]

(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

[Ere Maijala (slow)]

(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.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Vlad:

Would you sr the patch?

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in to trunk, thank you.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 336663 [details] [diff] [review]
Patch v3.0

let's land this patch to 1.9.0.3.

Comment 33

[Daniel Veditz [:dveditz]]

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?

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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.

Comment 35

[Alfred Kayser]

It is a regression as FF3.0 (and older) don't have this behavior of locking the browser when radius is used.

Comment 36

[Daniel Veditz [:dveditz]]

Comment on attachment 336663 [details] [diff] [review]
Patch v3.0

Approved for 1.9.0.4, a=dveditz for release-drivers

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in to 1.9.0 branch.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for ci to 1.9.0 branch

Comment 39

[Al Billings [:abillings - ex-MoCo]]

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.