Alfred Kayser Reporter
	Description • 16 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.

	Alfred Kayser Reporter
	Comment 1 • 16 years ago

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

	Richard Marti (:Paenglab)
	Comment 2 • 16 years ago

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

	Ria Klaassen (not reading all bugmail)
	Comment 3 • 16 years ago

Maybe bug 433340? 

	AKIHIRO Misaki (a.k.a Kuden)
	Comment 4 • 16 years ago

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

	Alfred Kayser Reporter
	Comment 5 • 16 years ago

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

	Samuel Sidler (old account; do not CC)
	Comment 6 • 16 years ago

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.

	Atsushi Sakai
	Comment 7 • 16 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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 8 • 16 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 9 • 16 years ago

oops, I meant bug 433340.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 10 • 16 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 11 • 16 years ago

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?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 12 • 16 years ago

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.

	Samuel Sidler (old account; do not CC)
	Comment 13 • 16 years ago

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

	Samuel Sidler (old account; do not CC)
	Comment 14 • 16 years ago

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.

	AKIHIRO Misaki (a.k.a Kuden)
	Comment 15 • 16 years ago

(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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 16 • 16 years ago

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 

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 17 • 16 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 18 • 16 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 19 • 16 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 20 • 16 years ago

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

	Ere Maijala (slow)
	Comment 21 • 16 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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 22 • 16 years ago

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|?

	Ere Maijala (slow)
	Comment 23 • 16 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.

	Ji Shiping
	Comment 26 • 16 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;
}

	Masatoshi Kimura [:emk]
	Comment 27 • 16 years ago

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

	Ere Maijala (slow)
	Comment 28 • 16 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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 29 • 16 years ago

Vlad:

Would you sr the patch?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 30 • 16 years ago

checked-in to trunk, thank you.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 32 • 16 years ago

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

let's land this patch to 1.9.0.3.

	Daniel Veditz [:dveditz]
	Comment 33 • 16 years ago

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?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 34 • 16 years ago

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

	Alfred Kayser Reporter
	Comment 35 • 16 years ago

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

	Daniel Veditz [:dveditz]
	Comment 36 • 16 years ago

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

Approved for 1.9.0.4, a=dveditz for release-drivers

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 37 • 16 years ago

checked-in to 1.9.0 branch.

	Al Billings [:abillings - ex-MoCo]
	Comment 39 • 16 years ago

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.