Crash in nsFrameManager::Destroy

VERIFIED FIXED in mozilla1.0

Status

()

Core
Layout
P1
critical
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: Aaron Leventhal, Assigned: Aaron Leventhal)

Tracking

(4 keywords)

Trunk
mozilla1.0
x86
Windows 2000
access, crash, sec508, topembed+
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

16 years ago
Sometimes GetPrimaryFrameFor to be called during the destruction phase of the
frame manager. This causes it to return an invalid pointer to a frame.

GetPrimaryFrameFor needs to check a flag, and return early if the frame manager
is in the middle of it's destruction phase.

This is the Bugzilla equivalent of Bugscape 12117
(http://bugscape/show_bug.cgi?id=12117), that had been reported by AOL's
accessibility testers.
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Keywords: access, crash, sec508
Priority: -- → P1
Whiteboard: patch ready, seeking r=
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 1

16 years ago
Created attachment 73142 [details] [diff] [review]
Checks flag to see if destruction phase make cause GPFF to return invalid pointer

Rather than reorder the destruction, this plays it safe and uses a flag
mIsDestroyingFrames. The flag is checked at the start of GetPrimaryFrameFor
Comment on attachment 73142 [details] [diff] [review]
Checks flag to see if destruction phase make cause GPFF to return invalid pointer

The following is unnecessary:

-FrameManager::FrameManager()
+FrameManager::FrameManager() : mIsDestroyingFrames(PR_FALSE)

since FrameManager uses ZEROING_OPERATOR_NEW, but other than that, r=dbaron.
Attachment #73142 - Flags: review+
   PropertyList*            mPropertyList;
   nsCOMPtr<nsIContentList> mHTMLForms;
   nsCOMPtr<nsIContentList> mHTMLFormControls;
+  PRBool mIsDestroyingFrames;

Indentation so the new member name starts in the same column as the existing
ones doesn't hurt.

/be
(Assignee)

Comment 4

16 years ago
Created attachment 73224 [details] [diff] [review]
Contains dbaron's and brendan's tweaks

This should still have dbaron's r= on it.
Attachment #73142 - Attachment is obsolete: true
(Assignee)

Comment 5

16 years ago
Comment on attachment 73224 [details] [diff] [review]
Contains dbaron's and brendan's tweaks

Carrying over r= from dbaron
Attachment #73224 - Flags: review+

Updated

16 years ago
Keywords: topembed

Updated

16 years ago
Keywords: topembed → topembed+
Comment on attachment 73224 [details] [diff] [review]
Contains dbaron's and brendan's tweaks

sr=shaver.
Attachment #73224 - Flags: superreview+

Comment 7

16 years ago
Comment on attachment 73224 [details] [diff] [review]
Contains dbaron's and brendan's tweaks

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #73224 - Flags: approval+

Comment 8

16 years ago
Changing QA contact
QA Contact: petersen → amar
(Assignee)

Comment 9

16 years ago
QA should probaly be dsirnapalli since he was the one who originally repro'd
this bug for me.
(Assignee)

Comment 10

16 years ago
fix checked in
(Assignee)

Comment 11

16 years ago
David Baron wrote (via email):
"This seems a little risky to me for the branch at this point.  I can
imagine things that it could break (they're the type of things (web page
X crashes) that I would think would be caught after a week of being
shaken out on the trunk, especially if you put that printf / assertion
in)."

Updated

16 years ago
QA Contact: amar → dsirnapalli
(Assignee)

Comment 12

16 years ago
fix checked into trunk, not into branch
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

16 years ago
Spoke with dbaron on IRC. He says it should be okay to check this fix into 0.9.9
now.

Updated

16 years ago
Keywords: edt0.9.9+

Comment 14

16 years ago
-- Verified the fix on MfcEmbed of trunk build. Works fine.
Verified it on MfcEmbed nightly_build also. Works fine.
Marking the bug as verified.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 15

16 years ago
John, can you check this one into the trunk for me?

Comment 16

16 years ago
I didn't have a 0.9.9 branch, pulled and am building as I write. I will apply
the patch and check it in asap.

Whiteboard: patch ready, seeking r= → in the trunk, preping to go in 0.9.9 branch

Comment 17

16 years ago
checked in to 0.9.9 branch

I'll let edt remove the edt0.9.9+
Keywords: fixed0.9.9
Whiteboard: in the trunk, preping to go in 0.9.9 branch

Comment 18

16 years ago
actually, "edt0.9.9+" stays. thanks for the checkin!

Comment 19

16 years ago
-- Verified the fix in latest 0.9.9ec (03/27/02). Works fine.
You need to log in before you can comment on or make changes to this bug.