194582 - gklayout.dll causes crash when saveEdit() function is called on the page (DOM node insertion and removal) [@ nsHTMLButtonElement::HandleDOMEvent]

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[leonard]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030221
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030221

When visiting PPK's http://www.xs4all.nl/~ppk/js/cms.html page and clicking on
the button that calls the saveEdit() function, recent builds (did it on one I
had from last week and today's) will always cause a crash.  The data dump points
to gklayout.dll being the culprit.

I made a simplified test case here:
http://randomfoo.net/sandbox/mozilla/saveEdit/crasher.html which surprisingly,
doesn't exhibit the same crashing behavior.

Reproducible: Always

Steps to Reproduce:
1. Visit http://www.xs4all.nl/~ppk/js/cms.html
2. Click on a paragraph
3. Click on the 'Ready' button
4. Crash!




gklayout.dll - I'd give more detail but that would require crashing the browser
that I'm filling the bug in (the Netscape Quality Feedback mechanism doesn't
have a way to get to the information being submitted after you click through and
there's no documentation in the Help on where the data is stored)

Comment 1

[Olivier Cahagne]

confirming crash on Linux with a debug build (CVS 10h ago) with URL
http://www.xs4all.nl/~ppk/js/cms.html :

0x40eec805 in nsHTMLButtonElement::HandleDOMEvent (this=0x8812b08,
    aPresContext=0x854a290, aEvent=0xbfffe590, aDOMEvent=0x0, aFlags=1,
    aEventStatus=0xbfffeac4) at nsHTMLButtonElement.cpp:488
488         mForm->OnSubmitClickEnd();
(gdb) bt
#0  0x40eec805 in nsHTMLButtonElement::HandleDOMEvent (this=0x8812b08,
    aPresContext=0x854a290, aEvent=0xbfffe590, aDOMEvent=0x0, aFlags=1,
    aEventStatus=0xbfffeac4) at nsHTMLButtonElement.cpp:488
#1  0x40d88f16 in PresShell::HandleEventInternal (this=0x883c508,
    aEvent=0xbfffe590, aView=0x0, aFlags=1, aStatus=0xbfffeac4)
    at nsPresShell.cpp:6226
#2  0x40d88dbb in PresShell::HandleEventWithTarget (this=0x883c508,
    aEvent=0xbfffe590, aFrame=0x867e9c0, aContent=0x8812b08, aFlags=1,
    aStatus=0xbfffeac4) at nsPresShell.cpp:6195

Comment 2

[Olivier Cahagne]

Attachment: Stack trace Linux 20030223

Comment 3

[Olivier Cahagne]

using the same build, I don't crash when testing URL
http://randomfoo.net/sandbox/mozilla/saveEdit/crasher.html but I get this
assertion in console:

###!!! ASSERTION: nsFrameManager::GenerateStateKey didn't find content by type!:
'index > -1', file nsFrameManager.cpp, line 2383
Break: at file nsFrameManager.cpp, line 2383

Comment 4

[Boris Zbarsky [:bzbarsky]]

form submission.  Olivier, it really helps if you list the relevant local vars
in addition to the stack.. for example, is mForm null here?

Comment 5

[Olivier Cahagne]

Boris, I'll do it from now on, here's some more data:

(gdb) frame 0
#0  0x40eec805 in nsHTMLButtonElement::HandleDOMEvent (this=0x8932238,
    aPresContext=0x8882ca8, aEvent=0xbfffe590, aDOMEvent=0x0, aFlags=1,
    aEventStatus=0xbfffeac4) at nsHTMLButtonElement.cpp:488
488         mForm->OnSubmitClickEnd();
(gdb) print mForm
No symbol "mForm" in current context.

I guess it's inline, I compiled with enable-optimize=-O2, I'll have to
recompile...

Comment 6

[Hermann Schwab]

CRASH Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.3b) Gecko/20030223
and talkback can´t connect

Comment 7

[Andrew Schultz]

-O exhibits a similar problem with 'No symbol "mForm" in current context.'
(gdb) p *this
shows that mForm is 0x0

Comment 8

[Jan Carpenter]

adding topcrash+ and testcase to keywords.

Comment 9

[Olivier Cahagne]

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

Comment 10

[Jan Carpenter]

This crash is no longer showing up in reports, but does still show up (though
not much) in the database in recent trunk and branch builds.  

I'll attach the most recent incident id.  

Comment 11

[Jan Carpenter]

Attachment: Stack trace from WinNT 5.0

Comment 12

[Jan Carpenter]

Marking topcrash-

Comment 13

[Olivier Cahagne]

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

Comment 14

[Olivier Cahagne]

reduced testcase from bug 203026: attachment 121447 [details].

Comment 15

[Owen Marshall (Not reading bugmail)]

All the dupes (plus the stack) seem to point to DOM Events.

Punting there, smack me if I am wrong...

Comment 16

[Olivier Cahagne]

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

Comment 17

[zack-weg]

Attachment: <form><input onchange="this.parentNode.removeChild(this)"></form>

Is this the same bug? It looks very similar.
Type something into the text field and press return to crash.
TB21430177H

Comment 18

[Mats Palmgren (inactive)]

Attachment: Patch (diff -wu)

When a form element removes itself from it's form it assigns NULL to |mForm|
(in nsGenericHTMLContainerFormElement::SetDocument() in this case)

This patch fixes both the URL (and duplicates) and also the testcase in
attachment 126671 [details] which is a separate bug but the same type of problem.

nsHTMLButtonElement.cpp: add a NULL check after HandleDOMEvent() was called.
nsHTMLInputElement.cpp: add a NULL check after CheckFireOnChange() was called.

Comment 19

[Mats Palmgren (inactive)]

Attachment: Patch (diff -u)

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 127140 [details] [diff] [review]
Patch (diff -wu)

I'm really not the best person to review this (I would need to do a lot of code
reading before I could reasonably do this review).

In any case, please add comments in both spots explaining why you are checking
mForm.

Comment 21

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (diff -wu)

Comment 22

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (diff -u)

Comment 23

[John Keiser (jkeiser)]

Comment on attachment 127149 [details] [diff] [review]
Patch rev. 2 (diff -wu)

This will solve the problem, but the new if() in InputElement should cover the
submitControl and numTextControlsFound variables and cover the next section
including GetShell() since if there is no form we're certainly not even going
to try to submit it.

While you're at that, a less-indentful way of doing this would be to *close*
the if (mForm) surrounding CheckFireOnChange and add the new if (mForm) as a
sibling.

Comment 24

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3 (diff -u)

The suggestion to close the previous "if (mForm)" and make a new block was a
good catch, it made the intent more obvious and the code less indentful.

Comment 25

[Bruce]

I've just encountered this bug when searching by "Bug #" on the Bugzilla
submission/search form. Keystrokes were:
Scrolled to bottom of bugzilla form by draging slide bar to bottom
Entered the Bug # in the Bug # box
Hit Enter
Crash

The resulting error was:

MOZILLA caused an invalid page fault in
module GKLAYOUT.DLL at 0167:6027dfaa.
Registers:
EAX=614fde7f CS=0167 EIP=6027dfaa EFLGS=00010246
EBX=021413c0 SS=016f ESP=0065f6b4 EBP=0065f6c4
ECX=01a33784 DS=016f ESI=01a33784 FS=3dbf
EDX=0065f6c0 ES=016f EDI=021413c0 GS=0000
Bytes at CS:EIP:
ff 90 40 01 00 00 8b 45 fc 56 ff 36 8b 08 50 ff 
Stack dump:
00000001 021413c0 01a33784 0214d440 0065f6ec 60288458 01a33784 00000000 01a33784
6028276e 01a33784 021413c0 021413c0 01a33784 0065f728 60289039 
  

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 127231 [details] [diff] [review]
Patch rev. 3 (diff -u)

sr=bzbarsky; drop me a mail if this needs checking in

Comment 27

[Asa Dotzler [:asa]]

Looks like this landed on 7/29. Can we resolve the bug?

Comment 28

[Mats Palmgren (inactive)]

Yes, thanks for noticing.

Comment 29

[Mats Palmgren (inactive)]

-> FIXED (2003-07-29)