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

VERIFIED DUPLICATE of bug 48064

Status

()

P1
critical
VERIFIED DUPLICATE of bug 48064
19 years ago
18 years ago

People

(Reporter: sfraser_bugs, Assigned: mjudge)

Tracking

({crash})

Trunk
All
Mac System 8.5
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta3-][rtm need info])

Attachments

(2 attachments)

(Reporter)

Description

19 years ago
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.
(Reporter)

Comment 1

19 years ago
Created attachment 14219 [details]
Test case that shows the crasher
(Reporter)

Comment 2

19 years ago
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.
Keywords: crash

Updated

18 years ago
Severity: normal → critical
Keywords: nsbeta3
Priority: P3 → P1
Whiteboard: [nsbeta3+]
Target Milestone: --- → M19

Comment 3

18 years ago
Updating QA contact.
QA Contact: ckritzer → bsharma

Comment 4

18 years ago
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
Keywords: rtm
Whiteboard: [nsbeta3+] → [nsbeta3-]

Comment 5

18 years ago
Mike, please include the required information per the rtm checkin rules
Whiteboard: [nsbeta3-] → [nsbeta3-][rtm+ NEED INFO]
(Assignee)

Comment 6

18 years ago
got fix in tree will add patch soon. simple null check after updating 
nsTextListener's frame member variable.
Status: NEW → ASSIGNED
(Assignee)

Comment 7

18 years ago
Created attachment 16213 [details] [diff] [review]
patch to make sure the frame hasnt been removed
(Assignee)

Comment 8

18 years ago
need to find module owner for layout i guess. getting super review from layout 
too.
(Assignee)

Comment 9

18 years ago
adding buster to CC for super-review.

Comment 10

18 years ago
a=buster, looks good mike

Comment 11

18 years ago
removing + per pdt sw rules
Whiteboard: [nsbeta3-][rtm+ NEED INFO] → [nsbeta3-][rtm NEED INFO]

Comment 12

18 years ago
r=kin@netscape.com with one change ...

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


changed to


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

Comment 13

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

Comment 14

18 years ago
ah ok, my appologies. :-)
(Assignee)

Comment 15

18 years ago
ok r=kin then a= buster please rtm++
(Assignee)

Comment 16

18 years ago
low risk low risk. please rtm++
removing NEED INFO. i think thats what i do now.
Whiteboard: [nsbeta3-][rtm NEED INFO] → [nsbeta3-][rtm]

Comment 17

18 years ago
Mike, actually PDT turns [rtm+] into [rtm++]. Making it [rtm+] so PDT will get
to it on our next sweep.
Whiteboard: [nsbeta3-][rtm] → [nsbeta3-][rtm+]

Comment 18

18 years ago
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! ;-) ).
Whiteboard: [nsbeta3-][rtm+] → [nsbeta3-][rtm need info]
(Assignee)

Comment 19

18 years ago
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

18 years ago

*** This bug has been marked as a duplicate of 48064 ***
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → DUPLICATE

Comment 21

18 years ago
Verifyed dupe
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.