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

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Attachment: 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 :(

Comment 1

[Jesse Ruderman]

Attachment: stack traces for the assertions

Comment 2

[Jesse Ruderman]

The assertions appear when I close the window containing the testcase, or navigate away.

Comment 3

[(no longer active)]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP

This is close, but not quite right, since what if there is non-the-same-tree
between mIndex and aStartIndex.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

I pushed this to tryserver.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 11

[Boris Zbarsky [: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.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/edf41ff32f08