After talking to David Baron, we decided to use a boolean flag to fix the problem rather than reorder the destruction phase. We considered this less risky, although the original patch may have been good.
Seekin r=dbaron, sr=hyatt http://bugzilla.mozilla.org/show_bug.cgi?id=129628 - Aaron Aaron Leventhal wrote: > I'm getting a crash when the JAWS screen reader asks for some > information about a link in the old page as a new page is loading. > This happens almost every time a new page is loaded by clicking on a > link. > > It turns out nsFrameManager first destroys the frame tree, and then > destroys the primary frame map second. The crash occurs because the > JAWS screen reader is asking about the link that was clicked on in the > old page when the window gets destroyed, but right before > the frame map is destroyed. Essentially, GetPrimaryFrameFor thinks it > still has a > valid hash table, even though none of the frame pointers are valid. A > *Bugscape* bug was filed on this crash (12117). > > The following small patch fixes the crash. It reorders the frame and > primary frame hash table destruction in nsFrameManager::Destroy. > > I need to know if this is an acceptable fix > > Index: nsFrameManager.cpp > =================================================================== > RCS file: /cvsroot/mozilla/layout/html/base/src/nsFrameManager.cpp,v > retrieving revision 1.102 > diff -u -1 -2 -r1.102 nsFrameManager.cpp > --- nsFrameManager.cpp 16 Feb 2002 16:31:49 -0000 1.102 > +++ nsFrameManager.cpp 7 Mar 2002 04:09:09 -0000 > @@ -503,37 +503,42 @@ > NS_IMETHODIMP > FrameManager::Destroy() > { > NS_ASSERTION(mPresShell, "Frame manager already shut down."); > > nsCOMPtr<nsIPresContext> presContext; > > mPresShell->GetPresContext(getter_AddRefs(presContext)); > > // Destroy the frame hierarchy. Don't destroy the property lists > until after > // we've destroyed the frame hierarchy because some frames may expect > to be > // able to retrieve their properties during destruction > mPresShell->SetIgnoreFrameDestruction(PR_TRUE); > - if (mRootFrame) { > - mRootFrame->Destroy(presContext); > - mRootFrame = nsnull; > - } > - > if (mPrimaryFrameMap.ops) { > PL_DHashTableFinish(&mPrimaryFrameMap); > mPrimaryFrameMap.ops = nsnull; > } > if (mPlaceholderMap.ops) { > PL_DHashTableFinish(&mPlaceholderMap); > mPlaceholderMap.ops = nsnull; > } > + > + // Don't destroy the frame hierarchy until the frame map is destroyed. > + // Otherwise something might call into GetPrimaryFrameFor and get a > bogus frame > + // For example, accessibility software in another process may ask for > information > + // about a node in a half-destroyed document > + if (mRootFrame) { > + mRootFrame->Destroy(presContext); > + mRootFrame = nsnull; > + } > + > delete mUndisplayedMap; > DestroyPropertyList(presContext); > > // If we're not going to be used anymore, we should revoke the > // pending |CantRenderReplacedElementEvent|s being sent to us. > nsresult rv = RevokePostedEvents(); > NS_ASSERTION(NS_SUCCEEDED(rv), "RevokePostedEvents failed: might > crash"); > > > > Thanks, > Aaron > >
