Closed Bug 412920 Opened 17 years ago Closed 17 years ago

[contenteditable] editable elements cannot be edited anymore after page refresh

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: dpopa, Assigned: cpearce)

References

Details

(Keywords: testcase)

Attachments

(2 files, 4 obsolete files)

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008010205 Minefield/3.0b3pre Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008010205 Minefield/3.0b3pre Editable elements cannot be edited anymore, few seconds after page refresh, Reproducible: Always Steps to Reproduce: 1. Open a page containing one or more elements with the "contenteditable" attribute set to true. Notice that the elements are editable and the spellcheck mechanism highlights the misspelled words. 2. Refresh the page 3. Click inside one of the editable elements and wait for a few seconds (2 to 10). Actual Results: The editable elements cannot be edited anymore. They still have the "contenteditable" attribute set, but they are not editable anymore. The spellcheck stops working and all the misspelled words are not underlined anymore. You can still see the cursor blinking. See the sample page attached.
Reproduced with latest nightly build: 2008011704
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
Another STR: Stephen Donner wrote: > Figured out how to reproduce: > > 1. Load > http://wiki.mozilla.org/index.php?title=QA/Firefox3/TestResults/M11/Release_Notes&action=edit > > 2. Put the cursor at the end of a line > 3. Click the "Back" button in Firefox > 4. Go forward again > > At this point, the caret isn't visible... >
I've also seen this with images in design mode. If you select them, you get resizers, but if you refresh the page with one still selected, the resizers aren't visible after, and you can't resize the images.
Looks like the CC is collecting the editor which is created on page reload shortly after load. Here's the stack which is freeing the editor shortly after reload: > gklayout.dll!nsEditor::~nsEditor() Line 192 C++ gklayout.dll!nsPlaintextEditor::~nsPlaintextEditor() Line 118 + 0x1e bytes C++ gklayout.dll!nsHTMLEditor::~nsHTMLEditor() Line 238 + 0x204 bytes C++ gklayout.dll!nsHTMLEditor::`scalar deleting destructor'() + 0xf bytes C++ gklayout.dll!nsEditor::Release() Line 226 + 0xdc bytes C++ gklayout.dll!nsHTMLEditor::Release() Line 248 + 0xd bytes C++ composer.dll!nsCOMPtr<nsIEditor>::~nsCOMPtr<nsIEditor>() Line 584 C++ composer.dll!nsEditingSession::TearDownEditorOnWindow(nsIDOMWindow * aWindow=0x0622a558) Line 684 + 0x11 bytes C++ gklayout.dll!nsHTMLDocument::TurnEditingOff() Line 3921 + 0x1d bytes C++ gklayout.dll!nsHTMLDocument::EditingStateChanged() Line 3966 + 0xb bytes C++ gklayout.dll!nsHTMLDocument::ChangeContentEditableCount(nsIContent * aElement=0x07fe3860, int aChange=-1) Line 3760 + 0x11 bytes C++ gklayout.dll!nsGenericHTMLElement::UnbindFromTree(int aDeep=1, int aNullParent=0) Line 1139 + 0x1e bytes C++ gklayout.dll!nsGenericElement::UnbindFromTree(int aDeep=1, int aNullParent=0) Line 2161 C++ gklayout.dll!nsGenericHTMLElement::UnbindFromTree(int aDeep=1, int aNullParent=0) Line 1147 C++ gklayout.dll!nsHTMLBodyElement::UnbindFromTree(int aDeep=1, int aNullParent=0) Line 478 C++ gklayout.dll!nsGenericElement::UnbindFromTree(int aDeep=1, int aNullParent=1) Line 2161 C++ gklayout.dll!nsGenericHTMLElement::UnbindFromTree(int aDeep=1, int aNullParent=1) Line 1147 C++ gklayout.dll!nsDocument::cycleCollection::Unlink(void * p=0x05fcb6e8) Line 1021 C++ xpcom_core.dll!nsCycleCollector::CollectWhite() Line 1578 + 0x1a bytes C++ xpcom_core.dll!nsCycleCollector::FinishCollection() Line 2317 + 0x8 bytes C++ xpcom_core.dll!nsCycleCollector_finishCollection() Line 2756 + 0x14 bytes C++ xpc3250.dll!XPCCycleCollectGCCallback(JSContext * cx=0x01347790, JSGCStatus status=JSGC_END) Line 450 + 0x6 bytes C++ js3250.dll!js_GC(JSContext * cx=0x01347790, JSGCInvocationKind gckind=GC_NORMAL) Line 2946 + 0x9 bytes C js3250.dll!JS_GC(JSContext * cx=0x01347790) Line 2394 + 0xb bytes C xpc3250.dll!nsXPConnect::Collect() Line 526 + 0xa bytes C++ xpcom_core.dll!nsCycleCollector::Collect(unsigned int aTryCollections=1) Line 2176 + 0x13 bytes C++ xpcom_core.dll!nsCycleCollector_collect() Line 2744 + 0x16 bytes C++ gklayout.dll!nsJSContext::CC() Line 3309 + 0x6 bytes C++ gklayout.dll!nsJSContext::MaybeCC(int aHigherProbability=1) Line 3332 C++ gklayout.dll!nsJSContext::CCIfUserInactive() Line 3348 + 0x7 bytes C++ gklayout.dll!nsJSContext::LoadEnd() Line 3406 C++ gklayout.dll!DocumentViewerImpl::LoadComplete(unsigned int aStatus=0) Line 1020 C++ docshell.dll!nsDocShell::EndPageLoad(nsIWebProgress * aProgress=0x0639d434, nsIChannel * aChannel=0x07682d40, unsigned int aStatus=0) Line 5032 C++ docshell.dll!nsWebShell::EndPageLoad(nsIWebProgress * aProgress=0x0639d434, nsIChannel * channel=0x07682d40, unsigned int aStatus=0) Line 1016 C++ docshell.dll!nsDocShell::OnStateChange(nsIWebProgress * aProgress=0x0639d434, nsIRequest * aRequest=0x07682d40, unsigned int aStateFlags=131088, unsigned int aStatus=0) Line 4932 C++ docshell.dll!nsDocLoader::FireOnStateChange(nsIWebProgress * aProgress=0x0639d434, nsIRequest * aRequest=0x07682d40, int aStateFlags=131088, unsigned int aStatus=0) Line 1236 C++ docshell.dll!nsDocLoader::doStopDocumentLoad(nsIRequest * request=0x07682d40, unsigned int aStatus=0) Line 869 C++ docshell.dll!nsDocLoader::DocLoaderIsEmpty() Line 765 C++ docshell.dll!nsDocLoader::OnStopRequest(nsIRequest * aRequest=0x07e9a778, nsISupports * aCtxt=0x00000000, unsigned int aStatus=0) Line 682 C++ necko.dll!nsLoadGroup::RemoveRequest(nsIRequest * request=0x07e9a778, nsISupports * ctxt=0x00000000, unsigned int aStatus=0) Line 688 + 0x2e bytes C++ gklayout.dll!nsDocument::DoUnblockOnload() Line 5535 C++ gklayout.dll!nsDocument::UnblockOnload(int aFireSync=1) Line 5483 C++ gklayout.dll!nsDocument::DispatchContentLoadedEvents() Line 2765 C++ gklayout.dll!nsRunnableMethod<nsDocument>::Run() Line 262 C++ xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012f768) Line 511 C++ xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x0134f298, int mayWait=1) Line 227 + 0x16 bytes C++ xpcom_core.dll!nsThread::Shutdown() Line 465 + 0xb bytes C++ xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000006, unsigned int methodIndex=0, unsigned int paramCount=0, nsXPTCVariant * params=0x0012f810) Line 102 C++ xpcom_core.dll!nsProxyObjectCallInfo::Run() Line 181 + 0x2d bytes C++ xpcom_core.dll!nsProxyObjectCallInfo::Run() Line 181 + 0x2d bytes C++ xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012f828) Line 511 C++ xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x0134f298, int mayWait=1) Line 227 + 0x16 bytes C++ gkwidget.dll!nsBaseAppShell::Run() Line 154 + 0xc bytes C++ tkitcmps.dll!nsAppStartup::Run() Line 181 + 0x1c bytes C++ xul.dll!XRE_main(int argc=4, char * * argv=0x01340b80, const nsXREAppData * aAppData=0x01347c68) Line 3145 + 0x25 bytes C++ firefox.exe!NS_internal_main(int argc=4, char * * argv=0x01340b80) Line 158 + 0x12 bytes C++ firefox.exe!wmain(int argc=4, wchar_t * * argv=0x00e3d440) Line 87 + 0xd bytes C++ firefox.exe!__tmainCRTStartup() Line 594 + 0x19 bytes C firefox.exe!wmainCRTStartup() Line 414 C kernel32.dll!760b3833() [Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll] ntdll.dll!773ba9bd()
Is that collecting the elements in the document of the first load? Do we end up in the situation where the document that's being collected still has access to its docshell, which is also the docshell of the new document? The document gets the editor from the docshell.
(In reply to comment #9) > Do we end up in the situation where the document that's being collected > still has access to its docshell, which is also the docshell of the new > document? Yes, this is what it happening. nsGenericHTMLElement::UnbindFromTree() calls ChangeContentEditableCount() when called on an editable element, this calls nsHTMLDocument::EditingStateChanged(), which calls nsHTMLDocument::TurnEditingOff() which gets the editing session from its window and calls nsEditingSession::TearDownEditorOnWindow(), which releases the docShell's reference on the editor, deleting it. So I maybe we should put something in nsGenericHTMLElement::UnbindFromTree() to prevent it from calling ChangeContentEditableCount() on nsHTMLDocuments which have had their editor removed?
Added code so that we don't call nsEditingSession::TearDownEditorOnWindow() more than once for any nsIHTMLDocument's editor created. * Add nsHTMLDocument::mIsEditorAlive field to track whether editor has been destroyed for an html doc. * Add nsIHTMLDocument::NotifyEditorDestroyed(), called by nsEditingSession::TearDownEditorOnWindow(). NotifyEditorDestroyed() sets mIsEditorAlive to PR_FALSE; * nsHTMLDocument::TurnEditingOff() only calls TearDownEditorOnWindow() when mIsEditorAlive is true.
Attachment #302906 - Flags: review?(peterv)
This is the same as previous, but without `diff -w`.
Comment on attachment 302906 [details] [diff] [review] Patch (-w) v1: Track when editor is alive for html document Would it work to split off part of TurnEditingOff into a new function (EditorTornDown(nsIEditor ...)), call that from TearDownEditorOnWindow and then make TurnEditingOff just call TearDownEditorOnWindow. I think it would work.
Attached patch V2 WIP (obsolete) — Splinter Review
(In reply to comment #13) > (From update of attachment 302906 [details] [diff] [review]) > Would it work to split off part of TurnEditingOff into a new function > (EditorTornDown(nsIEditor ...)), call that from TearDownEditorOnWindow and then > make TurnEditingOff just call TearDownEditorOnWindow. I think it would work. > This patch is what I think you mean, it almost works, but we get a bunch of these warnings when the old html doc is collected: WARNING: Context has no global.: file c:/work/src/browser/mozilla/dom/src/base/nsJSEnvironment.cpp, line 2206 Also the caret is repositioned to the start of the editor when the old document is collected. I'll take a closer look on Monday...
Hmm, that doesn't work entirely like I expected. When we tear down the document we end up reoving a style element which calls EndUpdate which tries to turn the editor back on.
Attached patch v2.1 WIP (obsolete) — Splinter Review
This seems to work for me, I wonder what it breaks ;-).
V2.1 WFM. It's a bit funny to change the state to eTornDown in EditorTornDown(), and then set it back to eOff in TurnEditingOff() once EditorTornDown()/TearDownEditorOnWindow() has finished, but if we don't do this, we get problems. Maybe we should call it TearingDownEditor()/eTearingDown to reflect that it's a short-lived state? Additional (independent) bugs: * If you position the caret at the end of the last line, alt+tab to another app, then alt+tab back, the caret appears at the start of the editable field, not the end. * If you position the caret at the end of the last line, it appears to be one space to the right of the last character, but if you type, your inserted characters don't have a space to their left. It's as if the first character inserted overwrites the space. I'll file bugs for these...
Attached patch v2.2Splinter Review
With the name change. Since we both ended up writing this patch together I'll just ask jst to give it a look too.
Attachment #303456 - Attachment is obsolete: true
Attachment #303882 - Attachment is obsolete: true
Attachment #304050 - Flags: superreview?
Attachment #304050 - Flags: review?
Attachment #304050 - Flags: superreview?(jst)
Attachment #304050 - Flags: superreview?
Attachment #304050 - Flags: review?(jst)
Attachment #304050 - Flags: review?
Comment on attachment 304050 [details] [diff] [review] v2.2 r+sr=jst
Attachment #304050 - Flags: superreview?(jst)
Attachment #304050 - Flags: superreview+
Attachment #304050 - Flags: review?(jst)
Attachment #304050 - Flags: review+
Attachment #302906 - Flags: review?(peterv)
Attachment #302907 - Attachment is obsolete: true
Attachment #302906 - Attachment is obsolete: true
Does the IID need to be revved? Checking in content/html/document/src/nsHTMLDocument.cpp; /cvsroot/mozilla/content/html/document/src/nsHTMLDocument.cpp,v <-- nsHTMLDocument.cpp new revision: 3.776; previous revision: 3.775 done Checking in content/html/document/src/nsHTMLDocument.h; /cvsroot/mozilla/content/html/document/src/nsHTMLDocument.h,v <-- nsHTMLDocument.h new revision: 3.228; previous revision: 3.227 done Checking in content/html/document/src/nsIHTMLDocument.h; /cvsroot/mozilla/content/html/document/src/nsIHTMLDocument.h,v <-- nsIHTMLDocument.h new revision: 3.71; previous revision: 3.70 done Checking in editor/composer/src/nsEditingSession.cpp; /cvsroot/mozilla/editor/composer/src/nsEditingSession.cpp,v <-- nsEditingSession.cpp new revision: 1.58; previous revision: 1.57 done
Status: NEW → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
Version: unspecified → Trunk
(In reply to comment #20) > Does the IID need to be revved? Yes, thanks for catching that.
(In reply to comment #21) > (In reply to comment #20) > > Does the IID need to be revved? > > Yes, thanks for catching that. Done. Checking in content/html/document/src/htmldocument.gqi; /cvsroot/mozilla/content/html/document/src/htmldocument.gqi,v <-- htmldocument.gqi new revision: 3.5; previous revision: 3.4 done Checking in content/html/document/src/nsIHTMLDocument.h; /cvsroot/mozilla/content/html/document/src/nsIHTMLDocument.h,v <-- nsIHTMLDocument.h new revision: 3.72; previous revision: 3.71 done
Depends on: 418719
No longer depends on: 418719
Depends on: 428844
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: