335615 - [FIX]Text inputs cause reentry into frame construction ("WARNING: recurring into frame construction: 'mPresContext->mLayoutPhaseCount[eLayoutPhase_FrameC] == 0'")

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P4)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Enable assert mentioned in bug 334460
2)  Load the testcase URL in this bug

What happens is that once we finish setting up the text control frame we do editor init, while still inside frame construction in general.  Editor messes with our anonymous content, causing reentry.  Note that bug 217201 made the current setup suck less than it used to, but ideally we'd defer the editor init to after we're all done with frame construction.

Comment 1

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

*** Bug 339454 has been marked as a duplicate of this bug. ***

Comment 2

[Justin Dolske [:Dolske]]

This bug seems to be a leading cause of console spewage.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Unfortunately, it's kinda hard to fix as things stand.  See bug 221820 for my last attempt.

Comment 5

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

This is not a dup of bug 334450. That bug is about nsTextBoxFrame, this one is 
about nsTextControlFrame

Comment 6

[Damon Sicore (:damons)]

Targeting/blocking M9 per request by dbaron.

Comment 7

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

Not going to make M9, realistically, but we should really try to get it done pretty soon.

Comment 8

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

I'll try taking a look at this and seeing if there's something I can come up with...

Comment 9

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

Attachment: stack showing the cause of the assertion

Comment 10

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

Time to drop this one from the blocker list.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

This basically takes a large chunk of the patch in bug 221820, but NOT the actual lazy init of the editor.  Instead, we set up a bunch of our things (like our text listener, etc) under CreateAnonymousContent, and then put off editor init (a combination of the current InitEditor and the current CreateFrameFor) to a script runner.  That will make sure that it runs as soon as the scriptblocker (which should be in place, since we're in frame construction!) gets removed.

A lot of the patch is just moving code around without changing it, but it could still use a somewhat careful read.  I haven't found any issues with the new ordering, but this is somewhat fragile code.  :(

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 357427 [details] [diff] [review]
Proposed fix

r=mats

> layout/forms/nsTextControlFrame.cpp

You can just remove nsTextControlFrame::CreateFrameFor() since
"return nsnull" is the default behaviour.

> nsTextControlFrame::AttributeChanged
> {
>   if (!mEditor || !mSelCon) 
>-    return NS_ERROR_NOT_INITIALIZED;
>+    return NS_OK;

Why not "return nsStackFrame::AttributeChanged(...)" ?


> layout/forms/nsTextControlFrame.h

s/NSIGFXTEXTCONTROLFRAME2/NSITEXTCONTROLFRAME/g

>+  void DelayedEditorInit();

I'd prefer if it were non-public.


> layout/generic/nsIAnonymousContentCreator.h

You can remove "class nsPresContext;" while you're here.

Also, this comment seems out of date:

> HTML frames like nsFileControlFrame currently use this as well as XUL frames
> like nsScrollbarFrame and nsSliderFrame.

Neither nsScrollbarFrame/nsSliderFrame use it AFAICT.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

> You can just remove nsTextControlFrame::CreateFrameFor()

Good catch.  Done.

> Why not "return nsStackFrame::AttributeChanged(...)" ?

We never call that, actually, and even the call to nsBoxFrame looks weird...  I'll make this call nsBoxFrame, though.  Should be safe enough.

> s/NSIGFXTEXTCONTROLFRAME2/NSITEXTCONTROLFRAME/g

Done.

> I'd prefer if it were non-public.

OK.  Moved to a friend class, I guess.

> Also, this comment seems out of date:

Fixed.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/0c39d79738c4