51772 - Crash in nsTextInputListener::Blur if an onchange handler does document.writeln

Status: VERIFIED DUPLICATE of bug 48064

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

Description

[Simon Fraser [no longer active]]

We crash in nsTextInputListener::Blur in the following situation:
1. Page has a form with an onchange handler that:
   a) does a document.writeln
   b) tries to set the value of another text input

See attached test case.

Comment 1

[Simon Fraser [no longer active]]

Attachment: Test case that shows the crasher

Comment 2

[Simon Fraser [no longer active]]

The problem here is that the onchange handler does a document.writeln, which 
blows away all frames. Thus the nsTextInputListener's mFrame member points to 
garbage when it tries to get the editor.

Comment 3

[bsharma]

Updating QA contact.

Comment 4

[Michael La Guardia]

What sort of sites would have this problem?  I'm marking this nsbeta3- as we are
due to complete engineering in two days.  Nominating for rtm

Comment 5

[rubydoo123]

Mike, please include the required information per the rtm checkin rules

Comment 6

[mjudge]

got fix in tree will add patch soon. simple null check after updating 
nsTextListener's frame member variable.

Comment 7

[mjudge]

Attachment: patch to make sure the frame hasnt been removed

Comment 8

[mjudge]

need to find module owner for layout i guess. getting super review from layout 
too.

Comment 9

[mjudge]

adding buster to CC for super-review.

Comment 10

[buster]

a=buster, looks good mike

Comment 11

[rubydoo123]

removing + per pdt sw rules

Comment 12

[kinmoz]

r=[e-mail] with one change ...

>     if (mFrame)
>       mFrame->CallOnChange();
>     if (mFrame)
>       mFrame->SubmitAttempt();


changed to


    if (mFrame)
    {
      mFrame->CallOnChange();
      mFrame->SubmitAttempt();
    }

Comment 13

[Simon Fraser [no longer active]]

kin: you miss the point  ;)
The issue is that
  mFrame->CallOnChange();
can null out mFrame, so the two tests are required.

Comment 14

[kinmoz]

ah ok, my appologies. :-)

Comment 15

[mjudge]

ok r=kin then a= buster please rtm++

Comment 16

[mjudge]

low risk low risk. please rtm++
removing NEED INFO. i think thats what i do now.

Comment 17

[Phil Peterson]

Mike, actually PDT turns [rtm+] into [rtm++]. Making it [rtm+] so PDT will get
to it on our next sweep.

Comment 18

[Jim Roskind]

Cool comment by frazer, explaining why Kin's suggestion wouldn't cut it.  This
had me fooled too...  Clearly a comment is called for (before someone else is
tricked, and "fixes" the code!).
The patch is also really hard to read in this form (I had a terrible time with
the rest of it).  Please use a diff -u patch, and add the comment that there is
a possible side-effect that mframe can be modified to null during the call.
(...yes... pdt folks often review this stuff too at this late point! ;-) ).

Comment 19

[mjudge]

ok then could we make this rtm++ with the caveat that i will add a comment to 
those lines? I will make a new patch with -u.

Comment 20

[rubydoo123]

*** This bug has been marked as a duplicate of 48064 ***

Comment 21

[Vladimir Ermakov]

Verifyed dupe