Closed Bug 305531 Opened 19 years ago Closed 19 years ago

dynamic insertion of iframe confuses docshell's session history walking

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.8beta4

People

(Reporter: bryner, Assigned: bryner)

Details

(Keywords: crash, verified1.8, Whiteboard: [needs review+SR bzbarsky])

Attachments

(2 files)

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.
Attached file testcase
Steps to reproduce:

1. Load the testcase
2. Click the button
3. Go back
This should probably block since it causes crashes for developers.
Flags: blocking1.8b4?
Keywords: crash
Flags: blocking1.8b4? → blocking1.8b4+
Target Milestone: --- → mozilla1.8beta4
Attached patch possible fixSplinter Review
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)
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+
checked in on the trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Attachment #194236 - Flags: approval1.8b4?
Attachment #194236 - Flags: approval1.8b4? → approval1.8b4+
fixed on branch
Keywords: fixed1.8
no crash firefox 1.5 rc2 winxp/linux
Keywords: fixed1.8verified1.8
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.

Attachment

General

Created:
Updated:
Size: