129628 - Crash in nsFrameManager::Destroy

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Aaron Leventhal]

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.

Comment 1

[Aaron Leventhal]

Attachment: 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 2

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

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.

Comment 3

[Brendan Eich [:brendan]]

   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

Comment 4

[Aaron Leventhal]

Attachment: Contains dbaron's and brendan's tweaks

This should still have dbaron's r= on it.

Comment 5

[Aaron Leventhal]

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

Carrying over r= from dbaron

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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

sr=shaver.

Comment 7

[Asa Dotzler [:asa]]

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

Comment 8

[Christine Hoffman]

Changing QA contact

Comment 9

[Aaron Leventhal]

QA should probaly be dsirnapalli since he was the one who originally repro'd
this bug for me.

Comment 10

[Aaron Leventhal]

fix checked in

Comment 11

[Aaron Leventhal]

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)."

Comment 12

[Aaron Leventhal]

fix checked into trunk, not into branch

Comment 13

[Aaron Leventhal]

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

Comment 14

[dsirnapalli]

-- Verified the fix on MfcEmbed of trunk build. Works fine.
Verified it on MfcEmbed nightly_build also. Works fine.
Marking the bug as verified.

Comment 15

[Aaron Leventhal]

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

Comment 16

[John Gaunt (redfive)]

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.

Comment 17

[John Gaunt (redfive)]

checked in to 0.9.9 branch

I'll let edt remove the edt0.9.9+

Comment 18

[Judson Valeski]

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

Comment 19

[dsirnapalli]

-- Verified the fix in latest 0.9.9ec (03/27/02). Works fine.

Comment 20

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381