[fastback] pageshow events fired for documents with null containers

RESOLVED FIXED in mozilla1.8.1beta2

Status

()

Core
DOM: Events
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Aaron Leventhal, Assigned: Brian Ryner (not reading))

Tracking

({fixed1.8.1, regression})

Trunk
mozilla1.8.1beta2
fixed1.8.1, regression
Points:
---
Bug Flags:
blocking1.8.1 ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

12 years ago
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."
(Reporter)

Comment 1

12 years ago
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
(Assignee)

Comment 2

12 years ago
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
(Assignee)

Comment 3

12 years ago
Created attachment 232356 [details] [diff] [review]
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.
Attachment #232356 - Flags: superreview?(bzbarsky)
Attachment #232356 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

12 years ago
Created attachment 232357 [details] [diff] [review]
trunk patch (comment only)
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+
(Assignee)

Comment 6

12 years ago
(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.
(Assignee)

Comment 7

12 years ago
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+
(Assignee)

Comment 9

12 years ago
(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.
(Assignee)

Comment 10

12 years ago
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 11

12 years ago
Comment on attachment 232356 [details] [diff] [review]
branch patch

a=schrep for drivers.
Attachment #232356 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 12

12 years ago
checked in, trunk and branch
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.