Closed Bug 282359 Opened 15 years ago Closed 15 years ago

Menus fire DOM events from inside frame constructor code

Categories

(Core :: XUL, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

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).
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.
Blocks: 281709
Attached patch Put stuff off on an event (obsolete) — Splinter Review
Neil, what do you think?
Attachment #174396 - Flags: review?(neil.parkwaycc.co.uk)
Blocks: 282265
While my tree rebuilds...
That would break opening menus by setting their "open" attribute from script,
no?  We have code in Firefox that relies on that....
(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...
Oh, and my version also has the advantage of fixing bug 212625 et. al.
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.
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.
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.
Flags: blocking1.8b2?
Comment on attachment 174396 [details] [diff] [review]
Put stuff off on an event

This is scary and unneeded.
Attachment #174396 - Attachment is obsolete: true
Attachment #174396 - Flags: review?(neil.parkwaycc.co.uk) → review-
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).
Attachment #175026 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #175026 - Flags: review?(mconnor)
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!
Attachment #174432 - Flags: superreview+
Attachment #174432 - Flags: review+
Attachment #175026 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #175026 - Flags: review?(mconnor)
Attachment #175026 - Flags: review-
Mano pointed out that button.xml should be preprocessed like it is in toolkit. 
I also added the '+' like in toolkit, just in case.
Attachment #175026 - Attachment is obsolete: true
Attachment #175028 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #175028 - Flags: review?(mconnor)
Blocks: 282180
Comment on attachment 175028 [details] [diff] [review]
Same thing, but with jar.mn fixed better

hooray for the start of unforking!
Attachment #175028 - Flags: review?(mconnor) → review+
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 ;-)
Attachment #175028 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Attached patch Parallel patchSplinter Review
Attachment #175028 - Attachment is obsolete: true
Comment on attachment 175162 [details] [diff] [review]
Parallel patch

Checked this in.
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]
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?
Firefox and Thunderbird both use the expander widget:

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

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....
Attachment #175214 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #175214 - Flags: review?(mconnor)
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 ;-)
Attachment #175214 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Attachment #175214 - Flags: review?(mconnor) → review+
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?
OK, core patch is in, so I guess this is fixed.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
No longer blocks: 260951
Blocks: 260951
Flags: blocking1.8b2?
Blocks: 285476
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.widgets
Blocks: 252618
You need to log in before you can comment on or make changes to this bug.