Closed
Bug 596870
Opened 14 years ago
Closed 14 years ago
"ASSERTION: Detaching editor when it's already detached" with iframes and contenteditable
Categories
(Core :: DOM: Editor, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: jruderman, Assigned: smaug)
References
Details
(Keywords: assertion, testcase)
Attachments
(5 files)
681 bytes,
text/html
|
Details | |
3.05 KB,
text/plain
|
Details | |
1.50 KB,
patch
|
Details | Diff | Splinter Review | |
4.27 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
4.40 KB,
patch
|
Details | Diff | Splinter Review |
###!!! ASSERTION: Detaching editor when it's already detached.: '!mOSHE || !mOSHE->HasDetachedEditor()', file docshell/base/nsDocShell.cpp, line 6590 ###!!! ASSERTION: We're going to overwrite an owning ref!: '!(aData && mEditorData)', file docshell/shistory/src/nsSHEntry.cpp, line 922 I was hoping smaug's fix for bug 591433 would take care of this, but no dice :(
Reporter | ||
Comment 1•14 years ago
|
||
Reporter | ||
Comment 2•14 years ago
|
||
The assertions appear when I close the window containing the testcase, or navigate away.
Updated•14 years ago
|
blocking2.0: --- → ?
Updated•14 years ago
|
Assignee: nobody → ehsan
blocking2.0: ? → final+
Comment 3•14 years ago
|
||
This is probably the most useless bug comment ever, but I just wanted to point out that I looked into this for a few hours but I couldn't figure it out. From what I can tell, this doesn't look directly related to the editor. I think there's a problem with the way that we manage our session history entries. Boris, I was wondering if you can take a look at this? This is where the first assertion happens: <http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#6553> (the second one is just a fallout of the first one).
Comment 4•14 years ago
|
||
Uh... yeah. This about sums it up: Breakpoint 2, nsSHEntry::SetEditorData (this=0x129c49590, aData=0x1235dfc70) at /Users/bzbarsky/mozilla/vanilla/mozilla/docshell/shistory/src/nsSHEntry.cpp:921 921 NS_ASSERTION(!(aData && mEditorData), (gdb) frame 1 #1 0x0000000100fad6c7 in nsDocShell::DetachEditorFromWindow (this=0x1235e2c20) at /Users/bzbarsky/mozilla/vanilla/mozilla/docshell/base/nsDocShell.cpp:6562 6562 mOSHE->SetEditorData(mEditorData.forget()); (gdb) p mOSHE $12 = { mRawPtr = 0x129c49590 } (gdb) frame 2 #2 0x0000000100fa8e35 in nsDocShell::FirePageHideNotification (this=0x1235e2c20, aIsUnload=1) at /Users/bzbarsky/mozilla/vanilla/mozilla/docshell/base/nsDocShell.cpp:1524 1524 DetachEditorFromWindow(); (gdb) frame 3 #3 0x0000000100fa8e0e in nsDocShell::FirePageHideNotification (this=0x1235f8800, aIsUnload=1) at /Users/bzbarsky/mozilla/vanilla/mozilla/docshell/base/nsDocShell.cpp:1519 1519 kids[i]->FirePageHideNotification(aIsUnload); (gdb) p mOSHE $13 = { mRawPtr = 0x129c49590 } So the parent docshell and the child docshell have the _same_ mOSHE. That's _so_ broken. For what it's worth, the parent (0x1235f8800) docshell's mCurrentURI is "https://bug596870.bugzilla.mozilla.org/attachment.cgi?id=475753" while the child (0x1235e2c20) docshell's mCurrentURI is "data:text/html,<body%20contenteditable=true>4". The latter is what matches mOSHE's URI.
Comment 5•14 years ago
|
||
OK, so I see mOSHE change twice on that parent docshell. The first time, it changes from one SHEntry for the bugzilla URI to another SHRntry for the bugzilla URI. This is the "data:text/html,2" load cloning the SHentry tree and the mOSHE set happens in nsDocShell::CloneAndReplaceChild; all good so far. The second time is the broken one. It happens with this stack: #3 0x0000000100f8d675 in nsDocShell::SwapHistoryEntries (this=0x12a1b4fd0, aOldEntry=0x125f68d50, aNewEntry=0x11a246a70) at /Users/bzbarsky/mozilla/vanilla/mozilla/docshell/base/nsDocShell.cpp:10118 #4 0x0000000100f9061a in nsDocShell::SetChildHistoryEntry (aEntry=0x125f68d50, aShell=0x12a1b4fd0, aEntryIndex=0, aData=0x7fff5fbfba00) at /Users/bzbarsky/mozilla/vanilla/mozilla/docshell/base/nsDocShell.cpp:10183 #5 0x0000000100f9082f in nsDocShell::SetHistoryEntry (this=0x1233848f0, aPtr=0x123384b70, aEntry=0x11a246a70) at /Users/bzbarsky/mozilla/vanilla/mozilla/docshell/base/nsDocShell.cpp:10239 #6 0x0000000100faf67d in nsDocShell::Embed (this=0x1233848f0, aContentViewer=0x1233dd820, aCommand=0x1019f1420 "", aExtraInfo=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/docshell/base/nsDocShell.cpp:5706 The issue is that in nsDocShell::SetHistoryEntry aEntry is the entry for the "data:text/html,<body contenteditable=true>4" url and has a null parent. So we end up setting newRootEntry to aEntry, and oldRootEntry to the actual old toplevel SHEntry, then call SetChildHistoryEntry which does: aShell->SwapHistoryEntries(aEntry, destEntry); In this case destEntry == destTreeRoot; it _should_ be an entry for the toplevel docshell, but because of the null parent above is not.
Comment 6•14 years ago
|
||
OK, so we create that session history entry in nsDocShell::AddToSessionHistory, then end up calling nsDocShell::AddChildSHEntry on our parent (that is, on the root docshell). AddChildSHEntry calls GetIndex on the session history and gets back 0. It then calls GetEntryAtIndex and gets back a session history entry (assigned to currentEntry) that does NOT match mOSHE on that root docshell. In particular, currentEntry has 0 children; mOSHE has one child. And that child is the aCloneRef that was passed to AddChildSHEntry. So the upshot is that we call CloneAndReplace, end up in CloneAndReplaceChild, never hit the |srcID == cloneID| case, so never hook the new SHEntry into the tree properly. And then we lose. Smaug, it sounds like when we removed that first iframe from the DOM we trimmed back the session history without syncing it up to the docshell, so the session history only contains the very first SHEntry for the root docshell, while the root docshell's mOSHE is the _second_ SHEntry it's had (and the one that has the child SHEntry corresponding to the second inserted subframe)... Should I keep trying to page in the SHistory trimming code, or do you want to take over?
Assignee | ||
Comment 7•14 years ago
|
||
I can take this. On 1.9.2 we get different assertions. ###!!! ASSERTION: Adding child where we already have a child? This will likely misbehave: 'Error', file /home/smaug/mozilla/hg/192/docshell/shistory/src/nsSHEntry.cpp, line 598 (In reply to comment #6) > Smaug, it sounds like when we removed that first iframe from the DOM we trimmed > back the session history without syncing it up to the docshell, so the session > history only contains the very first SHEntry for the root docshell, while the > root docshell's mOSHE is the _second_ SHEntry it's had (and the one that has > the child SHEntry corresponding to the second inserted subframe)... Ah, indeed, session history doesn't update docshell's shentry, or in other way, perhaps session history shouldn't remove entries which are currently in use.
Assignee: ehsan → Olli.Pettay
Assignee | ||
Comment 8•14 years ago
|
||
This is close, but not quite right, since what if there is non-the-same-tree between mIndex and aStartIndex.
Assignee | ||
Comment 9•14 years ago
|
||
I pushed this to tryserver.
Assignee | ||
Comment 10•14 years ago
|
||
Comment on attachment 489594 [details] [diff] [review] possible patch I verified the existing tests test for example the code path when a new mListRoot is set. Need to take Jesse's testcase as a crash test.
Attachment #489594 -
Flags: review?(bzbarsky)
Comment 11•14 years ago
|
||
Comment on attachment 489594 [details] [diff] [review] possible patch >+ NS_ASSERTION(aIndex >= 0, "aIndex must be > 0!"); Fix the assert text? >+ if (aKeepNext) { >+ txToKeep->SetPrev(txPrev); >+ if (txPrev) { >+ txPrev->SetNext(txToKeep); >+ } >+ } else { >+ txToKeep->SetNext(txNext); >+ if (txNext) { >+ txNext->SetPrev(txToKeep); >+ } >+ } SetNext does a SetPrev. So this could be: if (aKeepNext) { if (txPrev) { txPrev->SetNext(txToKeep); } else { txToKeep->SetPrev(nsnull); } } else { txToKeep->SetNext(txNext); } Document the new argument, and r=me with the nits.
Attachment #489594 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 12•14 years ago
|
||
Assignee | ||
Comment 13•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/edf41ff32f08
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•