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

RESOLVED FIXED in Firefox 59

Status

()

defect
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jorgk, Assigned: masayuki)

Tracking

({regression})

Trunk
mozilla60
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox58 unaffected, firefox59 fixed, firefox60 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

a year ago
+++ 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.
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.
Assignee: nobody → masayuki
Blocks: 1425091
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All
Comment hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
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;
Attachment #8948959 - Flags: review?(m_kato) → review+
Comment hidden (mozreview-request)

Comment 7

a year ago
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/46af948408ea
EditorBase::CreateElement() should call RangeUpdater::SelAdjCreateNode() with point of new element r=m_kato
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.
Attachment #8949332 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/46af948408ea
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment on attachment 8949332 [details] [diff] [review]
Patch for Beta

Fix for a recent regression from 59. Let's uplift this for beta 9.
Attachment #8949332 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.