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

Status: RESOLVED FIXED

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

Description

[Dan POPA]

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.

Comment 1

[Dan POPA]

Attachment: simple page with one contenteditable div. Open, refresh, click inside the editable element and wait 2-10 seconds.

Reproduced with latest nightly build: 2008011704

Comment 2

[Ria Klaassen (not reading all bugmail)]

Regression window is 
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1196416980&maxdate=1196446319
Caused by bug 348156 (the Cycle Collector) or maybe bug 262116 ? I have no access to bug 348156.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Possibly a regression from bug 348156

Comment 5

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

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

Comment 6

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

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.

Comment 8

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

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

Comment 9

[Peter Van der Beken [:peterv]]

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.

Comment 10

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

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

Comment 11

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

Attachment: Patch (-w) v1: Track when editor is alive for html document

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.

Comment 12

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

Attachment: Patch v1 - As previous, but without -w

This is the same as previous, but without `diff -w`.

Comment 13

[Peter Van der Beken [:peterv]]

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.

Comment 14

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

Attachment: V2 WIP

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

Comment 15

[Peter Van der Beken [:peterv]]

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.

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: v2.1 WIP

This seems to work for me, I wonder what it breaks ;-).

Comment 17

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

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

Comment 18

[Peter Van der Beken [:peterv]]

Attachment: v2.2

With the name change. Since we both ended up writing this patch together I'll just ask jst to give it a look too.

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 304050 [details] [diff] [review]
v2.2

r+sr=jst

Comment 20

[Reed Loden [:reed]]

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

Comment 21

[Peter Van der Beken [:peterv]]

(In reply to comment #20)
> Does the IID need to be revved?

Yes, thanks for catching that.

Comment 22

[Reed Loden [:reed]]

(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