334450 - nsTextBoxFrame initialization causes incorrect recursion into frame construction

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

When opening the Firefox options dialog in a clean profile, I see the warning added in bug 310985 that there was recursion into frame construction.  The cause is the stack in attachment 218792 [details] (which I incorrectly attached to bug 334430).

Comment 1

[Boris Zbarsky [:bzbarsky]]

I can't reproduce this.  :(  What does |call DumpJSStack()| show when this happesn?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FWIW, it was on a completely clean profile, so it may have something to do with what's inside the default-selected panel (last panel selection is remembered).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yeah, I tried creating a clean profile; still didn't see this.  :(

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  I've reproduced this now.  I needed to actually enable the assert!  ;)

Comment 5

[Boris Zbarsky [:bzbarsky]]

So the issue here is that the "popup" field of the "autocomplete" binding in autocomplete.xml modifies the anonymous DOM.  So we end up doing that from nsXBLBinding::InstallImplementation, which is called from nsElementSH::PostCreate in this case.

This PostCreate is called when we try to get this.labeledControlElement in the "accessKey" property getter of the "label-control" binding in text.xml.

And that, of course, happens because nsTextBoxFrame::UpdateAttributes calls GetAccessKey on its content node.

I don't see an obvious solution here past doing UpdateAttributes off an event (so NOT inside Init or reflow or anything like that; running script under those is a bad idea) or fixing the autocomplete binding to not modify the DOM from inside binding attachment.

We probably need to do the latter anyway, frankly, otherwise just attaching that binding will assert as far as I can tell.  We have constructors for a reason; this binding should use them.

Comment 6

[Boris Zbarsky [:bzbarsky]]

One more note.  Apart from whatever these bindings are doing by accident, a binding could on purpose rip the document apart when we evaluate a field, as things stand.... That's really not good, and probably needs a separate bug. :(

Comment 7

[[e-mail]]

I'd love to see that autocomplete.xml binding fixed :-) However I can't tell whether it should be moved to a constructor or converted to a property.

I'm also interested to know why the popup has to be manually hidden.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

bz, mind attaching a callstack of when this happens. It's hard to keep all the calls of comment 5 in my head at the same time.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment 0 has the C++ stack.   The JS stack (from DumpJSStack()) only shows the first XBL frame, sadly, not the place where the actual call that caused issues happens.

A pseudo-stack would be:

Re-enter frame construction
nsCSSFrameConstructor::ContentAppended
...
nsXULDocument::ContentAppended
...
nsXULElement::AppendChild
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/autocomplete.xml&rev=1.57&mark=121#110
nsJSContext::EvaluateStringWithValue
nsXBLProtoImplField::InstallMember
nsXBLProtoImpl::InstallImplementation
nsXBLPrototypeBinding::InstallImplementation
nsXBLBinding::InstallImplementation
nsXBLService::LoadBindings
nsElementSH::PostCreate
Wrap return value of getElementById in an XPCWrappedNative
Call into C++ (getElementById)
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/text.xml&rev=1.27&mark=255#254
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/text.xml&rev=1.27&mark=228#224
Get the "accesskey" property from mContent; calls into XBL
nsTextBoxFrame::UpdateAttributes
nsTextBoxFrame::Init
nsCSSFrameConstructor::InitAndRestoreFrame
Normal frame construction, etc.

Comment 10

[Mike Schroepfer]

Hey folks - any idea of this is still around and relevant?

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Looks like this depends on bug 372769

Comment 12

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

(In reply to comment #11)
> Looks like this depends on bug 372769
> 

Or more like a dup of Bug 387033. I'll mark this one fixed once the
regression from bug 387033 is fixed.

Comment 13

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

The warnings I still see when opening preferences are because of bug 335615 and bug 372769. nsTextBoxFrame::Init doesn't cause warnings anymore, because
.accessKey is get in a reflowcallback. Marking this fixed (by bug 387033).