dynamic insertion of iframe confuses docshell's session history walking

RESOLVED FIXED in mozilla1.8beta4

Status

()

Core
Document Navigation
--
major
RESOLVED FIXED
12 years ago
9 years ago

People

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

Tracking

({crash, verified1.8})

Trunk
mozilla1.8beta4
crash, verified1.8
Points:
---
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [needs review+SR bzbarsky])

Attachments

(2 attachments)

(Assignee)

Description

12 years ago
There's some code in nsDocShell::SetChildHistoryEntry that wasn't ever supposed
to get hit, but it turns out that it can.  If you do a subframe navigation, then
dynamically insert an iframe, then go back, the cloned session history trees
will now have different numbers of child entries.  This leads to a crash in
debug builds because the #ifdef DEBUG code doesn't null-check.  It's not clear
to me right now what the impact would be on release builds.
(Assignee)

Comment 1

12 years ago
Created attachment 193463 [details]
testcase

Steps to reproduce:

1. Load the testcase
2. Click the button
3. Go back
(Assignee)

Comment 2

12 years ago
This should probably block since it causes crashes for developers.
Flags: blocking1.8b4?
Keywords: crash

Updated

12 years ago
Flags: blocking1.8b4? → blocking1.8b4+
Target Milestone: --- → mozilla1.8beta4
(Assignee)

Comment 3

12 years ago
Created attachment 194236 [details] [diff] [review]
possible fix

This certainly isn't an ideal solution but I can't think of a better one right
now, especially if we take the second patch on bug 305181 (I wouldn't want to
take that one without this patch).  Really we just need to fix this once and
for all so that the trees are in sync, or perhaps rethink things so there is no
longer a separate SHEntry tree.
Attachment #194236 - Flags: superreview?(bzbarsky)
Attachment #194236 - Flags: review?(bzbarsky)

Updated

12 years ago
Whiteboard: [needs review+SR bzbarsky]
Comment on attachment 194236 [details] [diff] [review]
possible fix

>Index: nsDocShell.cpp
>+        if (entry && NS_SUCCEEDED(entry->GetID(&id)) && id == targetID) {
>+            destEntry = entry;

Maybe swap() here?  Can't hurt.

>+                if (id == targetID) {
>+                    destEntry = entry;

Same here.

And yes, longer-term (1.9?) I think we should redesign this stuff somehow.
Attachment #194236 - Flags: superreview?(bzbarsky)
Attachment #194236 - Flags: superreview+
Attachment #194236 - Flags: review?(bzbarsky)
Attachment #194236 - Flags: review+
(Assignee)

Comment 5

12 years ago
checked in on the trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Attachment #194236 - Flags: approval1.8b4?

Updated

12 years ago
Attachment #194236 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 6

12 years ago
fixed on branch
Keywords: fixed1.8

Comment 7

12 years ago
no crash firefox 1.5 rc2 winxp/linux
Keywords: fixed1.8 → verified1.8

Updated

9 years ago
Component: History: Session → Document Navigation
QA Contact: history.session → docshell
You need to log in before you can comment on or make changes to this bug.