430624 - Crash [@ nsDocShellEditorData::DetachFromWindow] with spellcheck attribute

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when closed)

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 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 2

[Peter Van der Beken [:peterv]]

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.

Comment 3

[Damon Sicore (:damons)]

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

Comment 4

[Martijn Wargers (dead)]

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

Comment 5

[Jesse Ruderman]

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

Comment 6

[Damon Sicore (:damons)]

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

Comment 7

[Peter Van der Beken [:peterv]]

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

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 317549 [details] [diff] [review]
v1

This was rolled into the regression fix patch in bug 386782.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2 - Bug 386782's regression fix patch

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 12

[Peter Van der Beken [: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.

Comment 13

[Peter Van der Beken [:peterv]]

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 14

[Damon Sicore (:damons)]

Comment on attachment 318752 [details] [diff] [review]
Patch v2 - Bug 386782's regression fix patch

a1.9+=damons

Comment 15

[Damon Sicore (: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.

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: v2

With review comments taken care of.

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 18

[Damon Sicore (:damons)]

Comment on attachment 318832 [details] [diff] [review]
v2

a1.9+=damons

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 20

[Peter Van der Beken [:peterv]]

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

Comment 21

[Marcia Knous [:marcia]]

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.

Comment 22

[Grant Gayed]

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.

Comment 23

[Peter Van der Beken [:peterv]]

(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.

Comment 24

[Henri Sivonen (:hsivonen) (temporarily not reading bugmail)]

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.