132334 - [PATCH][PFM]crash if repeat changing display of absolutely positioned form element [@ nsIFrame::GetStyleData]

Status: RESOLVED FIXED

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

Description

[Dylan Schiemann]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.9) Gecko/20020311
BuildID:    2002031104

In the testcase to follow, changing the display back and forth from block to
none of an absolutely positioned div inside a form causes a crash.  Removing the
positioning, or removing the form element prevents the crash.

Reproducible: Always
Steps to Reproduce:
1. Run testcase
2. click several times on hide/show button
3.

Actual Results:  browser crashes

Expected Results:  should hide and show div

Comment 1

[Dylan Schiemann]

Attachment: testcase showing crash

Comment 2

[Boris Zbarsky [:bzbarsky]]

###!!! ASSERTION: frame was not removed from primary frame map before
destruction or was readded to map after being removed:
'!PL_DHASH_ENTRY_IS_BUSY(entry) || entry->frame != aFrame', file
nsFrameManager.cpp, line 1009
###!!! Break: at file nsFrameManager.cpp, line 1009
###!!! ASSERTION: frame was not removed from primary frame map before
destruction or was readded to map after being removed:
'!PL_DHASH_ENTRY_IS_BUSY(entry) || entry->frame != aFrame', file
nsFrameManager.cpp, line 1009
###!!! Break: at file nsFrameManager.cpp, line 1009
###!!! ASSERTION: frame was not removed from primary frame map before
destruction or was readded to map after being removed:
'!PL_DHASH_ENTRY_IS_BUSY(entry) || entry->frame != aFrame', file
nsFrameManager.cpp, line 1009
###!!! Break: at file nsFrameManager.cpp, line 1009
###!!! ASSERTION: No style context found!: 'mStyleContext', file
../../../dist/include/layout/nsIFrame.h, line 556
###!!! Break: at file ../../../dist/include/layout/nsIFrame.h, line 556

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1024 (runnable)]
0x412d66eb in nsIFrame::GetStyleData (this=0x8791770, aSID=eStyleStruct_Display,
aStyleStruct=@0xbfffccac)
    at ../../../dist/include/layout/nsIFrame.h:557
557         aStyleStruct = mStyleContext->GetStyleData(aSID);
(gdb) 
(gdb) p mStyleContext
$1 = (class nsIStyleContext *) 0x0

#0  0x412d66eb in nsIFrame::GetStyleData (this=0x8791770,
aSID=eStyleStruct_Display, aStyleStruct=@0xbfffccac)
    at ../../../dist/include/layout/nsIFrame.h:557
#1  0x410e53a9 in nsComputedDOMStyle::GetStyleData (this=0x86a2968,
aID=eStyleStruct_Display, aStyleStruct=@0xbfffccac, 
    aFrame=0x8791770) at nsComputedDOMStyle.cpp:3623
#2  0x410e1a8e in nsComputedDOMStyle::GetDisplay (this=0x86a2968,
aFrame=0x8791770, aValue=@0xbfffce28)
    at nsComputedDOMStyle.cpp:2322
#3  0x410dbfad in nsComputedDOMStyle::GetPropertyCSSValue (this=0x86a2968,
aPropertyName=@0x8437490, aReturn=0xbfffce78)
    at nsComputedDOMStyle.cpp:515
#4  0x410dba33 in nsComputedDOMStyle::GetPropertyValue (this=0x86a2968,
aPropertyName=@0x8437490, aReturn=@0xbfffd024)
    at nsComputedDOMStyle.cpp:480
#5  0x40268a65 in XPTC_InvokeByIndex (that=0x86a2968, methodIndex=5,
paramCount=2, params=0xbfffd0b8)
    at xptcinvoke_unixish_x86.cpp:153
#6  0x40aef7e6 in XPCWrappedNative::CallMethod (ccx=@0xbfffd160,
mode=CALL_METHOD) at xpcwrappednative.cpp:2025
#7  0x40af8227 in XPC_WN_CallMethod (cx=0x8637300, obj=0x86cbe08, argc=1,
argv=0x8793bd8, vp=0xbfffd284)
    at xpcwrappednativejsops.cpp:1266
#8  0x400d5902 in js_Invoke (cx=0x8637300, argc=1, flags=0) at jsinterp.c:788
#9  0x400ed677 in js_Interpret (cx=0x8637300, result=0xbfffdc34) at
jsinterp.c:2745
#10 0x400d597d in js_Invoke (cx=0x8637300, argc=1, flags=2) at jsinterp.c:805
#11 0x400d5cdc in js_InternalInvoke (cx=0x8637300, obj=0x84fd7c0,
fval=139450336, flags=0, argc=1, argv=0xbfffe0bc, rval=0xbfffddb8)
    at jsinterp.c:880

looks like GetPrimaryFrameFor gives us a bogus frame and then we crash when we
try to GetStyleData() on it.

Comment 3

[Arthur]

OS: All, Crashing on Linux 2002031721. TB4276685X.

Comment 4

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

Marking bugs dealing with re-addition to the primary frame map with [PFM].

Comment 5

[karnaze (gone)]

Taking the bug.

Comment 6

[karnaze (gone)]

Nominating for nsbeta1, since the problem is due to gfx text controls accessing 
the editor during frame destruction which seems like a serious problem in 
general. 

Comment 7

[Kevin McCluskey (gone)]

nsbeta1+. adt2

Comment 8

[karnaze (gone)]

Attachment: patch to fix the bug

The patch adds nsIFrame::RemovedAsPrimaryFrame which is called in
nsCSSFrameConstructor::DoDeletingFrameSubtree and DoCleanupFrameReferences. It
allows nsGfxTextControlFrame to call its new PreDestroy method which does what
was previously being done in Destroy. So, it accesses mEditor before mEditor's
anonymous div gets removed from the map.

Comment 9

[Marc Attinasi]

Comment on attachment 77156 [details] [diff] [review]
patch to fix the bug

sr=attinasi - though I must say I really hate inline virtual methods (yes, I
see there are already some in there...)

Comment 10

[John Keiser (jkeiser)]

Comment on attachment 77156 [details] [diff] [review]
patch to fix the bug

Seems good to me, since we are currently guaranteed that a frame is about to be
destroyed when it is removed from the primary frame map, and that happens
before the sub-frames get messed with.

About that mBits array; if you're going to define  a bitfield why not put the
other bools into it?  Also it's a bitfield, not a bool; it should be PRUint8 or
whatever else you want to declare it as.  In the case of other bools that need
to be set to false, you can use macros (it's easier); I made some in
nsHTMLInputElement.cpp that should apply here just fine.

with one of those, r=jkeiser

Comment 11

[karnaze (gone)]

I'll make the minimal change of turning it into a PRUint8 and adding a comment, 
because I want to reduce the risk. And besides, what you are suggestion should 
have already been done before my change. The best long term solution is to use 
the extra bits from the frame state (there are 12 or so high order bits 
available) and let the existing 3 packed bools and this new use that. This would 
save 4 bytes from the text control frame. However, I was not 100% sure which if 
any of those bits were already being used by ancestors of the text control 
frame. 

Comment 12

[karnaze (gone)]

Attachment: revised patch with reviewer's suggestions

Comment 13

[Asa Dotzler [:asa]]

Comment on attachment 77786 [details] [diff] [review]
revised patch with reviewer's suggestions

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 14

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checkin to 1.0.

Comment 15

[karnaze (gone)]

The patch is in.