347426 - [fastback] pageshow events fired for documents with null containers

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Aaron Leventhal]

In bug 346936 I dealt with accessibility crashes that occured because a pageshow was fired for a document with a null container. I am concerned that the root cause of the problem is not being dealt with.

The problem occurs on both trunk and branch.

Brian Ryner said via email "The container is unhooked when the document is put into session history, but should be hooked up again before pageshow is fired.  If it's not, that's a bug we should fix for FF2, since it might cause problems for any pageshow event handler."

Comment 1

[Aaron Leventhal]

Need bryner's opinion on severity, but he did say it should be fixed for Firefox 2. The problem did not occur in Firefox 1.5.

Comment 2

[Brian Ryner (not reading)]

This is a regression from the fix for bug 320489, I believe, and only on the branch.  The problem happens because ClearChildShells is called before OpenWithEntry, which wants to use the child shell list to reattach the document containers.

Comment 3

[Brian Ryner (not reading)]

Attachment: branch patch

Move the child shell capturing to after the call to OpenWithEntry, and add a comment to the description of OpenWithEntry.  This shouldn't regress bug 320489, since we're still grabbing the child shell list before calling DestroyChildren.

Comment 4

[Brian Ryner (not reading)]

Attachment: trunk patch (comment only)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 232356 [details] [diff] [review]
branch patch

Is this the right patch?  This looks like the diff from bug 320489, which isn't what we really want here...

Also, bug 320489 landed on the 1.8.0 branch.  Do we need this fix there too?

Comment 6

[Brian Ryner (not reading)]

(In reply to comment #5)
> (From update of attachment 232356 [details] [diff] [review] [edit])
> Is this the right patch?  This looks like the diff from bug 320489, which isn't
> what we really want here...
> 
> Also, bug 320489 landed on the 1.8.0 branch.  Do we need this fix there too?
> 

Yes this is the right patch.  The important part is moving the call to ClearChildShells() to after OpenWithEntry(), but for clarity I moved the entire chunk of code that deals with the child shell list.

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 232356 [details] [diff] [review]
branch patch

re-requesting review per my above comment

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 232356 [details] [diff] [review]
branch patch

Hmm...  I think I ended up looking at the wrong patch somehow...

This looks good. Again, do we need this on the 1.8.0 branch?  Or does the fact that we don't pass mLSHE to Open() there make things ok?

Comment 9

[Brian Ryner (not reading)]

(In reply to comment #8)
> (From update of attachment 232356 [details] [diff] [review] [edit])
> Hmm...  I think I ended up looking at the wrong patch somehow...
> 
> This looks good. Again, do we need this on the 1.8.0 branch?  Or does the fact
> that we don't pass mLSHE to Open() there make things ok?
> 

On the 1.8.0 branch we don't bother detaching or reattaching the containers for subdocuments (ugh), so this particular problem doesn't show up there.

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 232356 [details] [diff] [review]
branch patch

requesting branch approval.  This patch is important because it fixes a regression in fastback stability/correctness from 1.5.

Comment 11

[Mike Schroepfer]

Comment on attachment 232356 [details] [diff] [review]
branch patch

a=schrep for drivers.

Comment 12

[Brian Ryner (not reading)]

checked in, trunk and branch