400976 - XUL menupopups in HTML document disappear in Firefox 2.0.0.8

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Marcus Better]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.5; Linux) KHTML/3.5.7 (like Gecko) (Debian)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.8) Gecko/20071004 Iceweasel/2.0.0.8 (Debian-2.0.0.8-1)

I have an HTML web app that uses some XUL elements to provide pop-up menus. This worked fine in Firefox before 2.0.0.8. In version 2.0.0.8 the pop-ups  disappear very quickly after being shown. A workaround is to apply a small delay with setTimeout before showPopup.


Reproducible: Always

Comment 1

[Marcus Better]

Attachment: Test case

This shows a popup menu with the text "POPUP!". When run in Firefox 2.0.0.8 you can see that the popup only flashes very quickly and then disappears. If you comment out the call to elt.showPopup() and uncomment the setTimeout line instead, the popup stays on screen.

The test case is not perfect since it doesn't work in earlier versions. On 2.0.0.7 or 2.0.0.6 it gives an error: "elt.showPopup is not a function". However my full webapp does work on earlier versions and creates popups in much the same way.

Comment 2

[Ria Klaassen (not reading all bugmail)]

This was repaired on trunk but it regressed again. Regression range for this is  http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1186421100&maxdate=1186423079 so it is probably caused by Bug 355789.

Comment 3

[Martijn Wargers (dead)]

Marcus, thanks for the testcase. Is there any chance we can test the webapp in question too?

This seems to have regressed on branch between 2007-10-03 and 2007-10-04:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-10-03+03&maxdate=2007-10-04+05&cvsroot=%2Fcvsroot
Regression from bug 398404?

I suspect a relation with bug 400744 too.

Comment 4

[Rob Arnold [:robarnold]]

The patch for bug 355789 wasn't checked in to branch (which 2.0.0.8 was built
from). Does this problem occur on trunk?

Comment 5

[Martijn Wargers (dead)]

It is worksforme, using current trunk build, but I'm using the win2000 theme here.
However, I see a scrollbar appearing in the document, for a brief while, on trunk.

Comment 6

[Ria Klaassen (not reading all bugmail)]

Attachment: Screenshot

This is what I see on trunk before I give another window focus. Then it disappears entirely.

Comment 7

[Martijn Wargers (dead)]

(In reply to comment #6)
> This is what I see on trunk before I give another window focus. Then it
> disappears entirely.

Ok, that's what I can see too. I filed bug 401027 as a trunk bug.

This bug is now meant as the regression that can be seen on branch.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I tried effectively backing out bug 398404, and I still see this problem on branch.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So the menu comes down with the following stack:

#0  nsMenuPopupFrame::HideChain (this=0x8dceaa4)
    at ../../../../../mozilla/layout/xul/base/src/nsMenuPopupFrame.cpp:1855
#1  0xb55b0229 in nsMenuDismissalListener::Rollup (this=0x8dd8528)
    at ../../../../../mozilla/layout/xul/base/src/nsMenuDismissalListener.cpp:110
#2  0xb67ab9ca in nsWindow::Destroy (this=0x8b14d70)
    at ../../../../mozilla/widget/src/gtk2/nsWindow.cpp:391
#3  0xb57d6c3b in ~nsView (this=0x8b14d18) at ../../../mozilla/view/src/nsView.cpp:266
#4  0xb57d6e08 in nsIView::Destroy (this=0x8b14d18)
    at ../../../mozilla/view/src/nsView.cpp:304
...
#12 0xb53747c2 in nsFrameManager::RemoveFrame (this=0x8d75dcc, aParentFrame=0x8dca4f8, 
    aListName=0x0, aOldFrame=0x8dceaa4)
    at ../../../mozilla/layout/base/nsFrameManager.cpp:717
#13 0xb5332597 in nsCSSFrameConstructor::ContentRemoved (this=0x8da49e8, 
    aContainer=0x8c07e90, aChild=0x8b90c28, aIndexInContainer=0, aInReinsertContent=0)
...
#18 0xb533aa7f in nsCSSFrameConstructor::RestyleEvent::HandleEvent (this=0x8b13748)
    at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:14286

I have no idea what this restyle is and why it's apparently happening later than it used to on branch.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I also backed out bug 393770 and I still see the problem.

Comment 11

[Boris Zbarsky [:bzbarsky]]

At this point I start to wonder about the range...  Someone should really try to do a pull by date and see what happens.

Comment 12

[Martijn Wargers (dead)]

I'm terribly sorry!
I had my Firefox2 build described as 2007-10-04, but it turns out it is from 2007-10-06.
I'll narrow the regression range again from there.
Again, sorry for this.

Comment 13

[Martijn Wargers (dead)]

Ok, the regression on branch is between 2007-10-04 and 2007-10-05:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-10-04+03&maxdate=2007-10-05+05&cvsroot=%2Fcvsroot
which makes bug 384105 to be a likely candidate for the regression.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Ah, yes.  That I would buy.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Backing out bug 384105 fixes this.

Are we perhaps ungenerating after the (following?) open has happened because of the event thing?

Comment 16

[Marcus Better]

(In reply to comment #3)
> Marcus, thanks for the testcase. Is there any chance we can test the webapp
> in question too?

Sure, send me a private mail if you still need it.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

This is not perfect, but I don't know what else could be done.
If we recreate the frame for menu, don't unset menugenerated.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: this one

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 286137 [details] [diff] [review]
this one

Neils, what do you think about this.

Comment 20

[Neil Deakin]

Comment on attachment 286137 [details] [diff] [review]
this one


>+    : mMenu(aMenu), mContent(aContent)

Maybe mContent(aContent) should be mPopup(aPopup) so it is clearer what node it refers to.

>+    if (mMenu) {

Could just null check this before creating the event, or just assert since this shouldn't be null.

Otherwise, looks OK.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

I'm asking extra review in hope to prevent regressions.

Comment 22

[[e-mail]]

Comment on attachment 286141 [details] [diff] [review]
v3

I was trying to work out which frame it was better to check for but I think checking for the menu frame is OK.

As for the null-check, child is (surprise) a child of mContent so it's going to be null when mContent is ;-)

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v4, check also that mPopup hasn't been moved

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 286156 [details] [diff] [review]
v4, check also that mPopup hasn't been moved

>+          new nsASyncUngenerate(menu, child);

s/menu/GetContent()

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v5, fix the "typo" in previous patch

Comment 26

[Mike Schroepfer]

Smaug/Bz any comments on risk level of this patch - we are about ready to start a 2009 and are not sure if we should take this.  Also - does this only affect XUL elements?

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

This is "only" about xul popups/menus behaving badly.
Not sure about bug 400744, though not sure if the patch fixes that either.
Risk should be low, but not minimal. The risk is there in cases where
xul popups/menus are used in some not-so-usual way.

Comment 28

[Boris Zbarsky [:bzbarsky]]

If this patch fixes bug 400744 I think we should take it for 2.0.0.9.

If it doesn't, then I'm a little torn.  If popups in XUL documents are not affected (something that needs to be checked), then I'm not sure XUL is used enough in HTML (or supported enough in that configuration) to put this in the firedrill release.

Comment 29

[Daniel Veditz [:dveditz]]

The testcase in bug 400744 appears to be a XUL dialog, and popups there were affected by this bug and fixed by this patch.

Comment 30

[Mike Schroepfer]

So my $.02 is since there is a workaround, this deals with XUL popups only we should put this in 2.0.0.10 when we have more time to test it...

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 286157 [details] [diff] [review]
v5, fix the "typo" in previous patch

sr=dveditz

approved for 1.8.1.9/1.8.1.10, a=dveditz for release-drivers

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Checked in.

Comment 33

[Daniel Veditz [:dveditz]]

Added appropriate fixed keywords for branch release tracking.

Comment 34

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.10 using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.10pre) Gecko/2007111503 BonEcho/2.0.0.10pre and the testcase from this bug.

Testcase is working fine now - adding verified keyword