1436216 - Too many warnings: '!mParent || mOffset.value() <= mParent->Length()' (EditorDOMPoint.h, line 373))

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1434969 +++

As per bug 1434969 comment #24, we see this 351 times in our Mozmill test suite, for example in our test composition/test-image-display.js (which is a short one).

I changed the warning to MOZ_ASSERT() and here is the stack.
Assertion failure: !mParent || mOffset.value() <= mParent->Length() (The offset is out of bounds), at c:\mozilla-source\comm-central\obj-i686-pc-mingw32\dist\include\mozilla/EditorDOMPoint.h:373
#01: mozilla::EditorDOMPointBase<nsINode *,nsIContent *>::Set (c:\mozilla-source\comm-central\obj-i686-pc-mingw32\dist\include\mozilla\editordompoint.h:373)
#02: mozilla::EditorBase::CreateNode (c:\mozilla-source\comm-central\mozilla\editor\libeditor\editorbase.cpp:1479)
#03: mozilla::EditorBase::DeleteSelectionAndCreateElement (c:\mozilla-source\comm-central\mozilla\editor\libeditor\editorbase.cpp:4558)
#04: mozilla::HTMLEditor::InsertAsCitedQuotation (c:\mozilla-source\comm-central\mozilla\editor\libeditor\htmleditordatatransfer.cpp:1917)
#05: nsMsgCompose::ConvertAndLoadComposeWindow (c:\mozilla-source\comm-central\mailnews\compose\src\nsmsgcompose.cpp:691)
#06: QuotingOutputStreamListener::OnStopRequest (c:\mozilla-source\comm-central\mailnews\compose\src\nsmsgcompose.cpp:3005)

For good measure I also ran composition\test-forwarded-eml-actions.js and it gives the same stack. To round it up I ran composition/test-send-format.js and that gave the same stack as well. So there's something fishy in HTMLEditor::InsertAsCitedQuotation() and below. Surely that doesn't run in Firefox, or does it? There is some setting and then the middle-mouse-button inserts a quote.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This is regression of bug 1425091.
https://searchfox.org/mozilla-central/diff/d438423b799a8be481271ff62341aa8196df4276/editor/libeditor/EditorBase.cpp#1452
> -     pointToInsert.Set(ret);
> +     pointToInsert.Set(ret, offset);

Those lines are not same. That should've been changed as:

> pointToInsert.Set(ret->GetParentNode(), offset);

I'll post a patch with cleaning up the method to avoid unnecessary instance creation of EditorRawDOMPoint.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8ba7261df2cc92d67015f1e72ffde7d56084f80

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=15fa324e81742813939965054c8d41db06217dca

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1436216 - EditorBase::CreateElement() should call RangeUpdater::SelAdjCreateNode() with point of new element

Before bug 1425091, we called RangeUpdater::SelAdjCreateNode() with point of
new node in EditorBase::CreateElement().  However, it was accidentally changed
to point *in* new element.  Therefore, setting such DOM point causes warnings
in debug build and call of RangeUpdater::SelAdjCreateNode() must be failed
since the point stores wrong container node.

Additionally, this patch stops using another EditorRawDOMPoint instance which
is just redundant and renames |ret| to |newElement| for making its meaning
clearer.

Review commit: https://reviewboard.mozilla.org/r/218382/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218382/

Comment 5

[Makoto Kato [:m_kato]]

Comment on attachment 8948959 [details]
Bug 1436216 - EditorBase::CreateElement() should call RangeUpdater::SelAdjCreateNode() with point of new element

https://reviewboard.mozilla.org/r/218382/#review224430

::: editor/libeditor/EditorBase.cpp:1463
(Diff revision 1)
>    //     we need to redesign mRangeUpdater as avoiding using indices.
> -  int32_t offset = static_cast<int32_t>(pointToInsert.Offset());
> +  Unused << aPointToInsert.Offset();
>  
>    AutoRules beginRulesSniffing(this, EditAction::createNode, nsIEditor::eNext);
>  
> -  nsCOMPtr<Element> ret;
> +  nsCOMPtr<Element> newElement;

RefPtr<Element> newElement;

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8948959 [details]
Bug 1436216 - EditorBase::CreateElement() should call RangeUpdater::SelAdjCreateNode() with point of new element

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218382/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46af948408ea
EditorBase::CreateElement() should call RangeUpdater::SelAdjCreateNode() with point of new element r=m_kato

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec2145a457fe783a8bfabb4d3e6c66fb159f2dc7

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for Beta

Approval Request Comment
[Feature/Bug causing the regression]:
New regression of bug 1425091.

[User impact if declined]:
User may see oddd result when editing rich text editor.

[Is this code covered by automated tests?]:
Yes and no. Actual symptom isn't tested since we don't know how the regression actually affects user level.  However, when we hit the case, we get warning of debug build. A lot of automated tests put this warning.

[Has the fix been verified in Nightly?]:
Not yet, but the lot of warnings have gone from automated test's log.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
No.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
This just makes the method behave same as prior to bug 1425091.

[String changes made/needed]:
No.

Comment 10

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/46af948408ea

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8949332 [details] [diff] [review]
Patch for Beta

Fix for a recent regression from 59. Let's uplift this for beta 9.

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8353dff90667