Closed Bug 430628 Opened 12 years ago Closed 10 years ago

"ASSERTION: EnsureEditorData() called when detached."

Categories

(Core :: Editor, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: cpearce)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, regression, testcase)

Attachments

(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

[sic]

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
Status: NEW → ASSIGNED
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]
v1.1

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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Crashtest: http://hg.mozilla.org/mozilla-central/rev/f4622261cce8
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.