Closed Bug 430628 Opened 14 years ago Closed 12 years ago

"ASSERTION: EnsureEditorData() called when detached."


(Core :: DOM: Editor, defect)

Not set





(Reporter: jruderman, Assigned: cpearce)


(Blocks 1 open bug)


(Keywords: assertion, regression, testcase)


(1 file, 2 obsolete files)

Attached file testcase
Leaving the testcase (e.g. loading another document in the same tab, or closing the tab, or quitting) triggers:

###!!! ASSERTION: EnsureEditorData() called when detached.
: '!HasDetachedEditor()', file /Users/jruderman/trunk/mozilla/docshell/base/nsDocShell.cpp, line 9010


This assertion was added in bug 386782.
Reloading triggers a whole bunch of it.
* InternalLoad calls DetachEditorFromWindow which stores editor data on the shentry
* InternalLoad sets the editor data on the shentry to null, because aLoadType & LOAD_CMD_HISTORY is 0
* the pagehide handler turns off editing, which needs the editing session so we create new editor data
* FirePageHideNotification calls DetachEditorFromWindow which stores editor data on the shentry

We should probably just delay DetachEditorFromWindow/clearing editor data on the shentry until FirePageHideNotification.
Attached patch v1 (obsolete) — Splinter Review
This seems to fix it, don't know if it is enough and whether it breaks anything, so it'll need a bunch of testing (designmode, contenteditable, compose mail, composer). Chris, could you take this one from here?
Assignee: nobody → chris
Breaks when you load a testcase, navigate to a DM page, nav back to testcase, reload testcase, then nav forward to DM page. Investigating...
Yeah, turns out for a reload InteralLoad is called before FirePageHide... Been working on a better fix, but not there yet :-/.
Attached patch v1.1 (obsolete) — Splinter Review
This one seems to behave better.
Attachment #317585 - Attachment is obsolete: true
(In reply to comment #6)
> Created an attachment (id=318017) [details]
> v1.1
> This one seems to behave better.

Still triggers assertions for me when you reload the page. Fixes a couple of other ones, so we're getting closer at least...
(In reply to comment #6)
> Created an attachment (id=318017) [details]
> v1.1
> This one seems to behave better.

Peter, why did you remove StartPageLoad() etc from nsEditingSession in this patch? Would that affect Smaug's patch for bug 430858?
StartPageLoad is unneeded, even with the patch in bug 430858. I don't remember the exact issue with StartDocumentLoad. There was a problem with the ordering, we'd postpone removing the editing session as a progress listener till PageHide and so StartDocumentLoad was called, which messed things up. I've tried your new patch in bug 386782 without the changes to StartDocumentLoad and I can't reproduce the problem, but that could also be because I don't remember the exact steps to reproduce :-/.
Comment on attachment 318017 [details] [diff] [review]

This is handled by bug 430624's patch.
Attachment #318017 - Attachment is obsolete: true
The assertion failure is fixed by 430624. There's still a warning firing when you reload the page though:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004002: file c:/work/src/browser/mozilla/content/html/document/src/nsHTMLDocument.cpp, line 3943

This is happening because we're detaching the editor in nsEditingSession::StartDocumentLoad(), and then the pagehide for the testcase is removing the last CE node, which toggles CE off, calling nsHTMLDocument::TurnEditingOff(), which can't get the editing session from its docshell, and fails an ENSURE_SUCCESS.
cpearce, please file a new bug if you think the warning is a problem.
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.