TB was built locally from the C-C tree fetched about a week ago. This is a full debug build with -DDEBUG defined during compilation. When |make mozmill| test suite was run, I notice there are many warnings of the form as follows. [7755, Main Thread] WARNING: 'mOffset.value() >= mParent->Length()', file /NREF-COMM-CENTRAL/objdir-tb3/dist/include/mozilla/EditorDOMPoint.h, line 500 and [7624, Main Thread] WARNING: The offset is out of bounds: '!mParent || mOffset.value() <= mParent->Length()', file /NREF-COMM-CENTRAL/objdir-tb3/dist/include/mozilla/EditorDOMPoint.h, line 373 Actually, the former outnumbers the latter by a factor of approximately 6. The following is the list of WARNING lines sorted based on the # of frequencies. The first PID part in the prefix "[PID, Main Thread]" part is omitted for the sake of comparison. I show only the top 20 entries. The number at the beginning of the line is the # of appearances. I mark the warning in question with a "*" at the beginning of the line. * 2289: Main Thread] WARNING: 'mOffset.value() >= mParent->Length()', file /NREF-COMM-CENTRAL/objdir-tb3/dist/include/mozilla/EditorDOMPoint.h, line 500 1971: Main Thread] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /NREF-COMM-CENTRAL/comm-central/mozilla/xpcom/base/nsTraceRefcnt.cpp, line 171 1376: Main Thread] WARNING: '!mMainThread', file /NREF-COMM-CENTRAL/comm-central/mozilla/xpcom/threads/nsThreadManager.cpp, line 403 1088: Main Thread] WARNING: NS_ENSURE_TRUE(ps) failed: file /NREF-COMM-CENTRAL/comm-central/mozilla/editor/libeditor/HTMLEditor.cpp, line 2872 1048: Main Thread] WARNING: Failed to retarget HTML data delivery to the parser thread.: file /NREF-COMM-CENTRAL/comm-central/mozilla/parser/html/nsHtml5StreamParser.cpp, line 1009 584: Main Thread] WARNING: NS_ENSURE_TRUE(aNode) failed: file /NREF-COMM-CENTRAL/comm-central/mozilla/editor/libeditor/EditorBase.cpp, line 3904 * 350: Main Thread] WARNING: The offset is out of bounds: '!mParent || mOffset.value() <= mParent->Length()', file /NREF-COMM-CENTRAL/objdir-tb3/dist/include/mozilla/EditorDOMPoint.h, line 373 307: Main Thread] WARNING: '!mTextEditor', file /NREF-COMM-CENTRAL/comm-central/mozilla/extensions/spellcheck/src/mozInlineSpellChecker.cpp, line 724 307: Main Thread] WARNING: '!mTextEditor', file /NREF-COMM-CENTRAL/comm-central/mozilla/extensions/spellcheck/src/mozInlineSpellChecker.cpp, line 1730 306: Main Thread] WARNING: '!aPoint.IsSetAndValid()', file /NREF-COMM-CENTRAL/comm-central/mozilla/editor/libeditor/SelectionState.cpp, line 227 289: Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80550006: file /NREF-COMM-CENTRAL/comm-central/mailnews/local/src/nsMailboxService.cpp, line 672 289: Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80550006: file /NREF-COMM-CENTRAL/comm-central/mailnews/base/util/nsMsgDBFolder.cpp, line 5134 272: Main Thread] WARNING: NS_ENSURE_TRUE(sheet) failed: file /NREF-COMM-CENTRAL/comm-central/mozilla/editor/libeditor/HTMLEditor.cpp, line 2866 226: Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /NREF-COMM-CENTRAL/comm-central/mozilla/netwerk/base/nsChannelClassifier.cpp, line 344 170: Main Thread] WARNING: We should have hit the document element...: file /NREF-COMM-CENTRAL/comm-central/mozilla/layout/xul/BoxObject.cpp, line 174 162: Main Thread] WARNING: NS_ENSURE_TRUE(ioService) failed: file /NREF-COMM-CENTRAL/comm-central/mailnews/base/util/nsMsgMailNewsUrl.cpp, line 622 162: Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file /NREF-COMM-CENTRAL/comm-central/mailnews/local/src/nsMailboxUrl.cpp, line 181 152: Main Thread] WARNING: NS_ENSURE_TRUE(msgDocShellItem) failed: file /NREF-COMM-CENTRAL/comm-central/mailnews/base/src/nsMsgWindow.cpp, line 90 148: Main Thread] WARNING: NS_ENSURE_TRUE(name) failed: file /NREF-COMM-CENTRAL/comm-central/mozilla/dom/base/nsDOMAttributeMap.cpp, line 327 67: Main Thread] WARNING: unable to post continuation event: file /NREF-COMM-CENTRAL/comm-central/mozilla/xpcom/io/nsStreamUtils.cpp, line 485 The second and the third most frequent messages, i.e., the following two have been in the log for a long time. 1971: Main Thread] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /NREF-COMM-CENTRAL/comm-central/mozilla/xpcom/base/nsTraceRefcnt.cpp, line 171 1376: Main Thread] WARNING: '!mMainThread', file /NREF-COMM-CENTRAL/comm- The two warnings mentioned in this bugzilla are related to Editor. Coupled with other warnings related to Editor, I suspect there are some serious issues such as the data object expected by Editing environment may not be present after a startup, etc. Anyway, I *think* the warnings were not there last summer and late fall when I tested local build of C-C TB. Busy day time job kept me from tinkering with C-C code until now since then. I am attaching a single test case where the two warnings related to mparent->Length() appear. But the warnings appear in many tests basically. Something is incorrect with EditorDom setup or the assumption does not hold for C-C TB. TIA

I am requesting information from Masayuki san as per Jorg's idea. See: https://groups.google.com/forum/#!topic/mozilla.dev.apps.thunderbird/KpPIu5eRCcY TIA

I'm taking the liberty to adjust the subject and to move the bug to Code::Editor. Perhaps these warnings could be toned down or their root cause addressed.

The warning means that EditorDOMPoint is set, then, the DOM tree is change, and finally, editor tries to rewind the offset. So, editor refers outdated DOM point. I.e., this is obviously an editor's bug. Could you get a stack trace when you hit the warning? Then, I could fix it easy.

I'd have to check which test produces the error and then see what the test does and repeat that action manually to get you a stack, unless Chiaki-san beats me to it.

Perhaps, just inserting MOZ_ASSERT(mOffset.value() < mParent->Length()); into the if block can show stack quickly.

This one |'mOffset.value() >= mParent->Length()' (EditorDOMPoint.h, line 500)| shows up if you start a new message in TB, that is, open a new compose window. Stack: xul.dll!mozilla::EditorDOMPointBase<nsINode *,nsIContent *>::RewindOffset() Line 502 C++ xul.dll!mozilla::HTMLEditRules::PromoteRange(nsRange & aRange, mozilla::EditAction aOperationType) Line 6087 C++ xul.dll!mozilla::HTMLEditRules::AfterEditInner(mozilla::EditAction aAction, short aDirection) Line 478 C++ xul.dll!mozilla::HTMLEditRules::AfterEdit(mozilla::EditAction aAction, short aDirection) Line 416 C++ xul.dll!mozilla::HTMLEditor::EndOperation() Line 3342 C++ xul.dll!mozilla::AutoRules::~AutoRules() Line 397 C++ xul.dll!mozilla::HTMLEditor::DoInsertHTMLWithContext(const nsTSubstring<char16_t> & aInputString, const nsTSubstring<char16_t> & aContextStr, const nsTSubstring<char16_t> & aInfoStr, const nsTSubstring<char16_t> & aFlavor, nsIDOMDocument * aSourceDoc, nsIDOMNode * aDestNode, int aDestOffset, bool aDeleteSelection, bool aTrustedInput, bool aClearStyle) Line 217 C++ xul.dll!mozilla::HTMLEditor::InsertHTMLWithContext(const nsTSubstring<char16_t> & aInputString, const nsTSubstring<char16_t> & aContextStr, const nsTSubstring<char16_t> & aInfoStr, const nsTSubstring<char16_t> & aFlavor, nsIDOMDocument * aSourceDoc, nsIDOMNode * aDestNode, int aDestOffset, bool aDeleteSelection) Line 191 C++ xul.dll!mozilla::HTMLEditor::InsertHTML(const nsTSubstring<char16_t> & aInString) Line 176 C++ xul.dll!nsMsgCompose::ConvertAndLoadComposeWindow(nsTString<char16_t> & aPrefix, nsTString<char16_t> & aBuf, nsTString<char16_t> & aSignature, bool aQuoted, bool aHTMLEditor) Line 761 C++ xul.dll!nsMsgCompose::BuildBodyMessageAndSignature() Line 4676 C++ xul.dll!nsMsgCompose::InitEditor(nsIEditor * aEditor, mozIDOMWindowProxy * aContentWindow) Line 1666 C++ Of course our tests create a few messages ;-) The other one comes soon after: xul.dll!mozilla::EditorDOMPointBase<nsCOMPtr<nsINode>,nsCOMPtr<nsIContent> >::Set(nsINode * aContainer, int aOffset) Line 372 C++ xul.dll!mozilla::HTMLEditor::InsertNodeIntoProperAncestor(nsIContent & aNode, const mozilla::EditorDOMPointBase<nsINode *,nsIContent *> & aPointToInsert, mozilla::SplitAtEdges aSplitAtEdges) Line 1579 C++ xul.dll!mozilla::HTMLEditor::InsertElementAtSelection(nsIDOMElement * aElement, bool aDeleteSelection) Line 1486 C++ xul.dll!_NS_InvokeByIndex() Line 57 Unknown _NS_InvokeByIndex() indicates that something is run from JS, in this case the JS stack is: 0 NotifyComposeBodyReadyNew() ["chrome://messenger/content/messengercompose/MsgComposeCommands.js":437] this = [object Object] 1 NotifyComposeBodyReady() ["chrome://messenger/content/messengercompose/MsgComposeCommands.js":352] this = [object Object] 2 InitEditor() ["chrome://messenger/content/messengercompose/MsgComposeCommands.js":6495] this = [object ChromeWindow] 3 observe(aSubject = [xpconnect wrapped nsICommandManager @ 0x193c99a0 (native @ 0x20feefc0)], aTopic = "obs_documentCreated", aData = "command_status_changed") ["chrome://messenger/content/messengercompose/MsgComposeCommands.js":3004] this = [object Object] MsgComposeCommands.js:437 inserts a paragraph that contains a break: editor.selection.collapse(mailBody, 0); let pElement = editor.createElementWithDefaults("p"); pElement.appendChild(editor.createElementWithDefaults("br")); 437 editor.insertElementAtSelection(pElement, false); Let me know if you want me to do further debugging. Note: I didn't look at the Mozmill logs, I just started a local debug build and created a new message et voilà.

(In reply to Jorg K (GMT+1) from comment #6) > xul.dll!mozilla::EditorDOMPointBase<nsINode *,nsIContent *>::RewindOffset() > Line 502 C++ > xul.dll!mozilla::HTMLEditRules::PromoteRange(nsRange & aRange, > mozilla::EditAction aOperationType) Line 6087 C++ > xul.dll!mozilla::HTMLEditRules::AfterEditInner(mozilla::EditAction aAction, > short aDirection) Line 478 C++ This might be bug of EditorDOMPointBase<>::RewindOffset(). Looks like that offset of it must be fine according to PromoteRange() and GetPromotedPoint(). mOffset.value() should be fine even if it equals length of its container because such point means after the last child. > The other one comes soon after: > > xul.dll!mozilla::EditorDOMPointBase<nsCOMPtr<nsINode>,nsCOMPtr<nsIContent> > >::Set(nsINode * aContainer, int aOffset) Line 372 C++ > xul.dll!mozilla::HTMLEditor::InsertNodeIntoProperAncestor(nsIContent & > aNode, const mozilla::EditorDOMPointBase<nsINode *,nsIContent *> & > aPointToInsert, mozilla::SplitAtEdges aSplitAtEdges) Line 1579 C++ Hmm, I don't understand this stack yet... #1579 of HTMLEditor.cpp is the end of IsertNodeIntoProperAncestor() but I don't understand why Set() is called from here. > xul.dll!mozilla::HTMLEditor::InsertElementAtSelection(nsIDOMElement * > aElement, bool aDeleteSelection) Line 1486 C++ > xul.dll!_NS_InvokeByIndex() Line 57 Unknown > > _NS_InvokeByIndex() indicates that something is run from JS, in this case > the JS stack is: > > 0 NotifyComposeBodyReadyNew() > ["chrome://messenger/content/messengercompose/MsgComposeCommands.js":437] > this = [object Object] I have a question. Do you know whether Thunderbird's composer uses mutation observer or mutation events? I mean that if Thunderbird observes DOM tree change of editor and it modifies the result of edit action, can cause this kind of warnings. Anyway, I should fix the first point.

[Tracking Requested - why for this release]: This is regression of bug 1419745. And this may break something of editor (if the point is set at end of a container element, moving the point to before the last child fails). We should uplift it.

Thanks for the quick action. Please not the spelling errors "gratner" and "lengh" in your patch. As for your question, yes, we use mutation observers, but only for the lang attribute, and we don't modify the DOM in response: https://dxr.mozilla.org/comm-central/rev/e62f73758e138743a47f2682f721862f09cd89c4/mail/components/compose/content/MsgComposeCommands.js#2607 For the second point I can understand better what's going on than for the first. When you start a new e-mail you get a new windows and a new editor inside it. We put all sorts of things into the editor in C++ code, like a signature, and when done, the C++ code calls NotifyComposeBodyReadyNew() from here: https://dxr.mozilla.org/comm-central/rev/e62f73758e138743a47f2682f721862f09cd89c4/mailnews/compose/src/nsMsgCompose.cpp#1666 That comes into the JS code in MsgComposeCommands.js and inserts a paragraph. As you know, TB now defaults into "paragraph mode". https://dxr.mozilla.org/comm-central/rev/e62f73758e138743a47f2682f721862f09cd89c4/mail/components/compose/content/MsgComposeCommands.js#437 So something triggers the warning when inserting the paragraph via InsertElementAtSelection().

(In reply to Jorg K (GMT+1) from comment #13) > Thanks for the quick action. Please not the spelling errors "gratner" and > "lengh" in your patch. Thanks! I used an environment which I don't use usually. So, I wrote it without spellchecker. > As for your question, yes, we use mutation observers, but only for the lang > attribute, and we don't modify the DOM in response: > https://dxr.mozilla.org/comm-central/rev/ > e62f73758e138743a47f2682f721862f09cd89c4/mail/components/compose/content/ > MsgComposeCommands.js#2607 Thank you. That's great information. Currently, editor does not check the DOM tree after modifying DOM tree by itself unless there were some crash bugs. So, basically, we still don't care mutation observer/events in editor. > For the second point I can understand better what's going on than for the > first. > > When you start a new e-mail you get a new windows and a new editor inside > it. We put all sorts of things into the editor in C++ code, like a > signature, and when done, the C++ code calls NotifyComposeBodyReadyNew() > from here: > https://dxr.mozilla.org/comm-central/rev/ > e62f73758e138743a47f2682f721862f09cd89c4/mailnews/compose/src/nsMsgCompose. > cpp#1666 > > That comes into the JS code in MsgComposeCommands.js and inserts a > paragraph. As you know, TB now defaults into "paragraph mode". > https://dxr.mozilla.org/comm-central/rev/ > e62f73758e138743a47f2682f721862f09cd89c4/mail/components/compose/content/ > MsgComposeCommands.js#437 > > So something triggers the warning when inserting the paragraph via > InsertElementAtSelection(). Thank you for the investigation. This file is really interesting to me since it actually uses editor's XPCOM APIs. I'm always afraid to breaks something of Thunderbird when we work on improving editor performance. So, this file helps me to understand what's going on in Thunderbird.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #15) > This file is really interesting to me since > it actually uses editor's XPCOM APIs. MsgComposeCommands.js is the code that drives the Thunderbird compose window. But there is much more. In Netscape, all editor code lived in editor/. Once Firefox and Thunderbird separated, editor/ got split into M-C: editor/composer editor/libeditor and more. C-C: editor/ui, also used by the SeaMonkey HTML editor. Thunderbird/SeaMonkey code is the biggest user of Core::Editor. Further reading in: https://dxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.js and a lot in: https://dxr.mozilla.org/comm-central/source/editor/ui/dialogs/content, just picking an example at random: https://dxr.mozilla.org/comm-central/rev/03203045182581bdc9e8909165c671809aed044a/editor/ui/dialogs/content/EdLinkProps.js#317

Comment on attachment 8948164 [details] Bug 1434969 - EditorDOMPointBase::RewindOffset() shouldn't treat it as error if offset is same as length of the container https://reviewboard.mozilla.org/r/217740/#review223548

Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/b5b67bfa069b EditorDOMPointBase::RewindOffset() shouldn't treat it as error if offset is same as length of the container r=m_kato

(In reply to Jorg K (GMT+1) from comment #6) > This one |'mOffset.value() >= mParent->Length()' (EditorDOMPoint.h, line > 500)| shows up if you start a new message in TB, that is, open a new compose > window. Stack: Thank you for collecting the stack information. I was away on a trip and then was down with a cold-like symptom. I may want to figure out if '!mParent || mOffset.value() <= mParent->Length()' (EditorDOMPoint.h, line 373)) still persists in |make mozmill| after applying the above patch. The discussion so far suggests mParent may have been null in the following cases: > * 350: Main Thread] WARNING: The offset is out of bounds: '!mParent || mOffset.value() <= mParent->Length()', file /NREF-COMM-CENTRAL/objdir-tb3/dist/include/mozilla/EditorDOMPoint.h, line 373 > > _NS_InvokeByIndex() indicates that something is run from JS, in this case > the JS stack is: How do you obtain JS stack back trace? > 0 NotifyComposeBodyReadyNew() > ["chrome://messenger/content/messengercompose/MsgComposeCommands.js":437] > this = [object Object] > 1 NotifyComposeBodyReady() > ["chrome://messenger/content/messengercompose/MsgComposeCommands.js":352] > this = [object Object] > 2 InitEditor() > ["chrome://messenger/content/messengercompose/MsgComposeCommands.js":6495] > this = [object ChromeWindow] > 3 observe(aSubject = [xpconnect wrapped nsICommandManager @ 0x193c99a0 > (native @ > 0x20feefc0)], aTopic = "obs_documentCreated", aData = > "command_status_changed") > ["chrome://messenger/content/messengercompose/MsgComposeCommands.js":3004] > this = [object Object] > Do you call a function from a debugger? Which function exactly? (Oh well I would insert MOZ_ASSERT() as crude measure). TIA > MsgComposeCommands.js:437 inserts a paragraph that contains a break: > editor.selection.collapse(mailBody, 0); > let pElement = editor.createElementWithDefaults("p"); > pElement.appendChild(editor.createElementWithDefaults("br")); > 437 editor.insertElementAtSelection(pElement, false); > > Let me know if you want me to do further debugging. Note: I didn't look at > the Mozmill logs, I just started a local debug build and created a new > message et voilà.

(In reply to ISHIKAWA, Chiaki from comment #19) > How do you obtain JS stack back trace? You get the debugger to evaluate/execute |DumpJSStack()|. With MS VS you can do that in a QuickWatch window.

(In reply to ISHIKAWA, Chiaki from comment #19) > I may want to figure out if '!mParent || mOffset.value() <= > mParent->Length()' (EditorDOMPoint.h, line 373)) > still persists in |make mozmill| after applying the above patch. > The discussion so far suggests mParent may have been null in the following > cases: > > * 350: Main Thread] WARNING: The offset is out of bounds: '!mParent || mOffset.value() <= mParent->Length()', file /NREF-COMM-CENTRAL/objdir-tb3/dist/include/mozilla/EditorDOMPoint.h, line 373 Yeah, it's really different bug. It must detect actual bug. Please file a follow up bug with stack trace when you hit it.

Comment on attachment 8948164 [details] Bug 1434969 - EditorDOMPointBase::RewindOffset() shouldn't treat it as error if offset is same as length of the container Approval Request Comment [Feature/Bug causing the regression]: Regression of bug 1419745 (starting from 59). [User impact if declined]: User may see odd result when they modify contenteditable or designMode editor since HTMLEditRules::PromoteRange() which is used to prepare of some edit actions may hits this bug. [Is this code covered by automated tests?]: No. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: No since we don't have a good STR to reproduce actual symptom. [List of other uplifts needed for the feature/fix]: No. [Is the change risky?]: No. [Why is the change risky/not risky?]: This patch just fixes wrong offset range check in a DOM node. Max possible offset of DOM point is length of the container node. However, old code decided offset == container.length is illegal. And adjusting (decrementing) offset is canceled in such case. [String changes made/needed]: No.

OK, this landed this morning on C-C and I'm not looking at the Mozmill debug run which includes this patch: In this log https://public-artifacts.taskcluster.net/F9XHhuioSjq-0-cjoVk8uw/0/public/logs/live_backing.log I see WARNING: The offset is out of bounds: '!mParent || mOffset.value() <= mParent->Length()', file z:\build\build\src\obj-thunderbird\dist\include\mozilla/EditorDOMPoint.h, line 373 351 times. I also see WARNING: '!mOffset.value()', file z:\build\build\src\obj-thunderbird\dist\include\mozilla/EditorDOMPoint.h, line 499 eight times. I'll file a new bug at least for the message that shows up 351 times. BTW, I can't reproduce it right now by just creating a new compose windows in TB.

I filed follow-up bug 1436216.

Comment on attachment 8948164 [details] Bug 1434969 - EditorDOMPointBase::RewindOffset() shouldn't treat it as error if offset is same as length of the container Sounds like we can assume this recent regression may affect Firefox users as well as TB. Let's uplift for beta 9.