Closed Bug 596870 Opened 10 years ago Closed 9 years ago

"ASSERTION: Detaching editor when it's already detached" with iframes and contenteditable

Categories

(Core :: DOM: Editor, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: jruderman, Assigned: smaug)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(5 files)

Attached file testcase
###!!! 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 :(
The assertions appear when I close the window containing the testcase, or navigate away.
blocking2.0: --- → ?
Assignee: nobody → ehsan
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).
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.
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.
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?
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
Attached patch WIPSplinter Review
This is close, but not quite right, since what if there is non-the-same-tree
between mIndex and aStartIndex.
Attached patch possible patchSplinter Review
I pushed this to tryserver.
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 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+
Attached patch patchSplinter Review
http://hg.mozilla.org/mozilla-central/rev/edf41ff32f08
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.