Leaving the testcase (window close, quit, or simply navigating to another document) triggers: ###!!! ASSERTION: Can't detach when we don't have a session to detach!: 'mEditingSession', file /Users/jruderman/trunk/mozilla/docshell/base/nsDocShellEditorData.cpp, line 231 ###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file ../../dist/include/xpcom/nsCOMPtr.h, line 868 Crash [@ nsDocShellEditorData::DetachFromWindow] This is all part of code added in bug 386782.

Comment on attachment 317549 [details] [diff] [review] v1 I also see the assertion from bug 430591 when loading this testcase, so included a fix for that. It doesn't make sense to create a new nsDocShellEditorData just to check if there's an editor.

Probably wouldn't block on this. Will take the patch. Please re-nom if you disagree.

Well, you did mark it blocking-1.9+, which I don't disagree with ;)

[@ nsDocShellEditorData::DetachFromWindow] accounts for 6% of crashes in the the last nightly.

Well, heck. I meant to minus this, but looks like more info indicates this is a blocker. :(

Chris rolled this into his patch in bug 386782, so reassigning to him.

Comment on attachment 317549 [details] [diff] [review] v1 This was rolled into the regression fix patch in bug 386782.

(In reply to comment #8) > (From update of attachment 317549 [details] [diff] [review]) > This was rolled into the regression fix patch in bug 386782. > I'm going to unroll this fix back into this bug, so that we don't have multiple checkins in bug 386782.

This patch is the continuation of the work on "Regression fix patch v1" from bug 386782. See bug 386782 comment 36 and onwards for details of the evolution of this patch. This is based on the earlier patch in this bug by PeterV. Changes to patch: * Change IID in nsISHEntry and nsIDocShell - I'd forgotten to do this in the original 386782 patch. * Moved ReattachEditorToWindow() call from nsDocshell::RestoreFromHistory() to nsDocShell::FinishRestore(). * Added crashtest. * Added mochitest to ensure that a reloaded iframe is still editable (basically a test for bug 431157, which is fixed by this). * In previous versions of this patch, I asserted openDocHasDetachedEditor in nsDocShell::EnsureEditorData(), but this was being triggered when you clicked on a live-bookmark from a popup, in nsXULPopupManager::FirePopupHidingEvent(). FirePopupHidingEvent() was reaching down into nsDocAccessible which was checking if something is readonly by asking the docshell for its editordata, after the editor had been detached. I think it's ok for EnsureEditorData() to just return failure in this case. I saved a callstack if anyone's interested. Anyone who wants to build/test this should also apply bug 430723's patch.

Comment on attachment 318752 [details] [diff] [review] Patch v2 - Bug 386782's regression fix patch >Index: docshell/shistory/public/nsISHEntry.idl >=================================================================== > %{ C++ >-// {BFD1A791-AD9F-11d3-BDC7-0050040A9B44} >+// {720570e7-838a-4d65-8a6e-b51709966f1d} > #define NS_SHENTRY_CID \ >-{0xbfd1a791, 0xad9f, 0x11d3, {0xbd, 0xc7, 0x0, 0x50, 0x4, 0xa, 0x9b, 0x44}} >+{ 0x720570e7, 0x838a, 0x4d65, { 0x8a, 0x6e, 0xb5, 0x17, 0x09, 0x96, 0x6f, 0x1d } } Don't change the CID. >Index: docshell/test/navigation/test_bug430624.html >=================================================================== >+function onLoad() { >+ window.frames[0].frameElement.onload = onReload; >+ window.frames[0].location = window.frames[0].location; >+} >+ >+function onReload() { >+ var bodyElement = window.frames[0].frameElement.contentDocument.body; >+ sendChar('S', bodyElement); >+ sendChar('t', bodyElement); >+ sendChar('i', bodyElement); >+ sendChar('l', bodyElement); >+ sendChar('l', bodyElement); >+ sendChar(' ', bodyElement); >+ >+ is(bodyElement.innerHTML, "Still contentEditable", "Check we're contentEditable after reload"); >+} >+ >+ I think you need to call SimpleTest.waitForExplicitFinish() here and SimpleTest.finish() at the end of onReload? Previous version also got sr=jst in bug 386782.

Comment on attachment 318752 [details] [diff] [review] Patch v2 - Bug 386782's regression fix patch This fixes the regressions from bug 386782. It's not without risk, but multiple people have tested the patch (see also bug 386782, comment 43 and more), and all issues so far have been taken care of. Includes mochitest and crashtest.

Comment on attachment 318752 [details] [diff] [review] Patch v2 - Bug 386782's regression fix patch a1.9+=damons

My understanding is that peterv has a holiday today, and chris can't check this in, so I'm going to ask Jonas to do it.

With review comments taken care of.

Comment on attachment 318832 [details] [diff] [review] v2 This v2 patch is r+ peterv sr+ jst, a1.9+damons. I'm not cool enough to carry the a1.9 flag over. ;) This patch the one needing checkin.

Comment on attachment 318832 [details] [diff] [review] v2 a1.9+=damons

Peter just checked this in. Was it meant to be left open?

Was just waiting on some of the unit test boxes to cycle.

verified fixed using : Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre. I verified using the test case in the bug.

This may be a dumb question, but I'll ask it: was it necessary for nsIDocShell's uuid to change as part of this? I ask because I thought that it only had to change if its idl changed, but I think the only idl change here is to change the uuid.

(In reply to comment #22) See comment #11: > * Change IID in nsISHEntry and nsIDocShell - I'd forgotten to do this in the > original 386782 patch.

What mechanism ensures that the editing state state is preserved when a data URL is navigated to itself using .location? How might the HTML5 parser be disrupting that mechanism? The test case for this bug fails with the HTML5 parser; bug 541078.