Crash [@ nsDocShellEditorData::DetachFromWindow] with spellcheck attribute

VERIFIED FIXED in mozilla1.9

Status

()

defect
P1
critical
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: jruderman, Assigned: cpearce)

Tracking

(Blocks 1 bug, 5 keywords)

Trunk
mozilla1.9
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

11 years ago
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?
Posted patch v1 (obsolete) — Splinter Review
Assignee: nobody → peterv
Status: NEW → ASSIGNED
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.
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 ;)
Reporter

Comment 5

11 years ago
[@ 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
Status: ASSIGNED → NEW
Assignee

Comment 8

11 years ago
Comment on attachment 317549 [details] [diff] [review]
v1

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]
Duplicate of this bug: 431116
Assignee

Updated

11 years ago
Blocks: 430615
(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)
Assignee

Updated

11 years ago
Blocks: 430997
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

a1.9+=damons
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.
Posted patch v2Splinter Review
With review comments taken care of.
Attachment #318752 - Attachment is obsolete: true
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.
Attachment #318832 - Flags: superreview+
Attachment #318832 - Flags: review+
Comment on attachment 318832 [details] [diff] [review]
v2

a1.9+=damons
Attachment #318832 - Flags: approval1.9+

Updated

11 years ago
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.
Status: NEW → RESOLVED
Closed: 11 years ago
OS: Mac OS X → All
Priority: -- → P1
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Blocks: 431157

Updated

11 years ago
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.
Status: RESOLVED → VERIFIED

Comment 22

11 years ago
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.