Closed Bug 430624 Opened 16 years ago Closed 16 years ago

Crash [@ nsDocShellEditorData::DetachFromWindow] with spellcheck attribute


(Core :: DOM: Editor, defect, P1)






(Reporter: jruderman, Assigned: cpearce)



(5 keywords)

Crash Data


(2 files, 2 obsolete files)

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.
Flags: blocking1.9?
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → peterv
Comment on attachment 317549 [details] [diff] [review]

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.
Attachment #317549 - Flags: review?(chris)
Probably wouldn't block on this.  Will take the patch.  Please re-nom if you disagree.
Flags: wanted1.9.0.x+
Flags: blocking1.9?
Flags: blocking1.9+
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.
Keywords: topcrash
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.
Assignee: peterv → chris
Comment on attachment 317549 [details] [diff] [review]

This was rolled into the regression fix patch in bug 386782.
Attachment #317549 - Attachment is obsolete: true
Attachment #317549 - Flags: review?(chris)
Whiteboard: [to be fixed by 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.
Whiteboard: [to be fixed by 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.
Attachment #318752 - Flags: review?(peterv)
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.
Attachment #318752 - Flags: superreview+
Attachment #318752 - Flags: review?(peterv)
Attachment #318752 - Flags: review+
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.
Attachment #318752 - Flags: approval1.9?
Comment on attachment 318752 [details] [diff] [review]
Patch v2 - Bug 386782's regression fix patch

Attachment #318752 - Flags: approval1.9? → approval1.9+
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.
Attached patch v2Splinter Review
With review comments taken care of.
Attachment #318752 - Attachment is obsolete: true
Comment on attachment 318832 [details] [diff] [review]

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.
Attachment #318832 - Flags: superreview+
Attachment #318832 - Flags: review+
Comment on attachment 318832 [details] [diff] [review]

Attachment #318832 - Flags: approval1.9+
Keywords: checkin-needed
Blocks: 430996
Peter just checked this in. Was it meant to be left open?
Was just waiting on some of the unit test boxes to cycle.
Closed: 16 years ago
OS: Mac OS X → All
Priority: -- → P1
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Blocks: 431157
Flags: in-testsuite+
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.
Flags: wanted1.9.0.x+
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.
Crash Signature: [@ nsDocShellEditorData::DetachFromWindow]
Crash Signature: [@ nsDocShellEditorData::DetachFromWindow]
You need to log in before you can comment on or make changes to this bug.