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)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla1.8beta4
People
(Reporter: bryner, Assigned: bryner)
Details
(Keywords: crash, verified1.8, Whiteboard: [needs review+SR bzbarsky])
Attachments
(2 files)
377 bytes,
text/html
|
Details | |
2.11 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
cbeard
:
approval1.8b4+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
Steps to reproduce: 1. Load the testcase 2. Click the button 3. Go back
Assignee | ||
Comment 2•19 years ago
|
||
This should probably block since it causes crashes for developers.
Flags: blocking1.8b4?
Keywords: crash
Updated•19 years ago
|
Flags: blocking1.8b4? → blocking1.8b4+
Target Milestone: --- → mozilla1.8beta4
Assignee | ||
Comment 3•19 years ago
|
||
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•19 years ago
|
Whiteboard: [needs review+SR bzbarsky]
Comment 4•19 years ago
|
||
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•19 years ago
|
||
checked in on the trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•19 years ago
|
Attachment #194236 -
Flags: approval1.8b4?
Updated•19 years ago
|
Attachment #194236 -
Flags: approval1.8b4? → approval1.8b4+
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.
Description
•