Closed Bug 347426 Opened 14 years ago Closed 14 years ago

[fastback] pageshow events fired for documents with null containers

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8.1beta2

People

(Reporter: aaronlev, Assigned: bryner)

References

Details

(Keywords: fixed1.8.1, regression)

Attachments

(2 files)

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."
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.
Flags: blocking1.8.1?
Keywords: regression
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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8.1beta2
Attached patch branch patchSplinter Review
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.
Attachment #232356 - Flags: superreview?(bzbarsky)
Attachment #232356 - Flags: review?(bzbarsky)
Attachment #232357 - Flags: superreview?(bzbarsky)
Attachment #232357 - Flags: review?(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?
Attachment #232356 - Flags: superreview?(bzbarsky)
Attachment #232356 - Flags: superreview-
Attachment #232356 - Flags: review?(bzbarsky)
Attachment #232356 - Flags: review-
Attachment #232357 - Flags: superreview?(bzbarsky)
Attachment #232357 - Flags: superreview+
Attachment #232357 - Flags: review?(bzbarsky)
Attachment #232357 - Flags: review+
(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 on attachment 232356 [details] [diff] [review]
branch patch

re-requesting review per my above comment
Attachment #232356 - Flags: superreview?(bzbarsky)
Attachment #232356 - Flags: superreview-
Attachment #232356 - Flags: review?(bzbarsky)
Attachment #232356 - Flags: review-
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?
Attachment #232356 - Flags: superreview?(bzbarsky)
Attachment #232356 - Flags: superreview+
Attachment #232356 - Flags: review?(bzbarsky)
Attachment #232356 - Flags: review+
(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 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.
Attachment #232356 - Flags: approval1.8.1?
Comment on attachment 232356 [details] [diff] [review]
branch patch

a=schrep for drivers.
Attachment #232356 - Flags: approval1.8.1? → approval1.8.1+
checked in, trunk and branch
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Blocks: 345687
You need to log in before you can comment on or make changes to this bug.