282359 - Menus fire DOM events from inside frame constructor code

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Place a breakpoint in the early return in PresShell::IsSafeToFlush that
    happens if we're reflowing or inside frame construction.
2)  Open a menu.

EXPECTED RESULTS: breakpoint is not hit

ACTUAL RESULTS:  breakpoint is hit

The reason this happens is that when the menu frame sees the click event it
toggles an attribute (in OpenMenu).  This ends up in
nsMenuFrame::AttributeChanged, which calls to OpenMenuInternal, which calls
OnCreate.  Now OnCreate fires a DOM event, which is handled by the popup.xml
binding.  This binding starts getting boxobject widths (due to the fix for bug
38367), which tries to flush reflow, which hits the breakpoint we added.

I don't think bug 38367 is regressed, but only because there are no pending
reflows on the menu...  In any case, this is a crash waiting to happen if
someone does something like closing the window or removing the menu node from
the DOM in the popupshowing handler.

The right solution, imo, is to put all of OpenMenuInternal off on an event from
inside the AttributeChanged code.  Thoughts on that?

If we do that, I think we may also be able to remove the hacks that handle
menugenerated synchronously (bug 279703).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note, though that even doing OpenMenuInternal off an event will have some
issues, since this fires DOM events and then proceeds to do other code too.  But
at that point the menu frame may have been destroyed....  We can maybe get
around that using a property on the frame, but maybe not.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Put stuff off on an event

Neil, what do you think?

Comment 3

[[e-mail]]

Attachment: My version for comparison

While my tree rebuilds...

Comment 4

[Boris Zbarsky [:bzbarsky]]

That would break opening menus by setting their "open" attribute from script,
no?  We have code in Firefox that relies on that....

Comment 5

[[e-mail]]

(In reply to comment #4)
>That would break opening menus by setting their "open" attribute from script
They could be fixed to call nsIMenuBoxObject::OpenMenu...

Comment 6

[[e-mail]]

Oh, and my version also has the advantage of fixing bug 212625 et. al.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ah, yes.  If we fix the callers, I like your version much more (esp for 1.8b). 
Do you want to hunt down the callers, or should I?  ccing mconnor, since the
only one I recall offhand is in the Firefox personal toolbar code.

Comment 8

[[e-mail]]

Both toolkits have the same code in button.xml so we need to add code to the
menu buttons to use the box object as per menulist.xml - as yet I'm not sure
whether we should add a menubutton base binding or just add an instanceof check.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Neil, do you have time to drive this in? Or should I?  We sort of want this for
1.8, I think, given bug 281709 and bug 260951.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 174396 [details] [diff] [review]
Put stuff off on an event

This is scary and unneeded.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix button bindings to not just set the attr

OK, this patch makes xpfe use the toolkit button.xml (and removes the xpfe
one), and fixes this per Neil's comments to not just set the attribute.  I've
verified that this change does not regress bug 279308 (so setting the "open"
property is working fine to open the menu).

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 174432 [details] [diff] [review]
My version for comparison

>Index: nsMenuFrame.cpp
>@@ -845,16 +837,18 @@
>     // Execute the ondestroy handler, but only if we're actually open
>     if ( !mCreateHandlerSucceeded || !OnDestroy() )
>       return;
> 
>+    mCreateHandlerSucceeded = PR_FALSE;

In the old codeflow, this would have happened after everything else in this
method.  Let's keep that part of the codeflow the same?  So put this at the end
of the else body?

With that change, r+sr=bzbarsky.  Thanks for coming up with the nice simple
idea!

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Same thing, but with jar.mn fixed better

Mano pointed out that button.xml should be preprocessed like it is in toolkit. 
I also added the '+' like in toolkit, just in case.

Comment 14

[Mike Connor [:mconnor]]

Comment on attachment 175028 [details] [diff] [review]
Same thing, but with jar.mn fixed better

hooray for the start of unforking!

Comment 15

[[e-mail]]

Comment on attachment 175028 [details] [diff] [review]
Same thing, but with jar.mn fixed better

As per bug 282177 comment 0 no unforking yet please but sr=me for an interim
parallel xpfe patch.

Indentation nit:
this.boxObject
    .QueryInterface(...)
    .openMenu(val);

Punctuation nit: . after bogus ;-)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Parallel patch

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175162 [details] [diff] [review]
Parallel patch

Checked this in.

Comment 18

[Scott MacGregor]

I believe this patch caused a regression in button.xml for mozilla\toolkit.
After updating my tree to pick up this change to buttons.xml, I now get XBL JS
errors when I try to collapse an open container button in Thunderbird's Options
dialog:

JavaScript error: , line 0: uncaught exception: [Exception... "Component returne
d failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]"  nsres
ult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: chrome://global/conte
nt/bindings/button.xml :: set_open :: line 37"  data: no]

Comment 19

[Boris Zbarsky [:bzbarsky]]

Scott, which of the button bindings does your button actually use?  Do we have
non-menubutton buttons that do something useful with the "open" attribute?

Comment 20

[Scott MacGregor]

Firefox and Thunderbird both use the expander widget:

http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/expander.xml

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix up things for expander and friends...

OK, I guess it's common to mess with the open property either for styling
purposes (menu dropmarkers) or just for the fun of it....

Comment 22

[[e-mail]]

Comment on attachment 175214 [details] [diff] [review]
Fix up things for expander and friends...

I prefer instanceof in case I run Venkman with break on exception ;-)

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175214 [details] [diff] [review]
Fix up things for expander and friends...

Checked this in too.  Neil, wanna land the core menu patch?

Comment 24

[[e-mail]]

OK, core patch is in, so I guess this is fixed.