1407966 - Assertion failure: node->GetChildAt(offset) == *aInOutChildAtOffset, at ... editor/libeditor/EditorBase.cpp:2524 - regression from bug 1406482 detected by Thunderbird Mozmill test

Status: RESOLVED FIXED

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

Description

[Jorg K (CEST = GMT+2)]

Assertion failure: node->GetChildAt(offset) == *aInOutChildAtOffset, at /builds/slave/tb-c-cen-m64-d-000000000000000/build/mozilla/editor/libeditor/EditorBase.cpp:2524

PROCESS-CRASH | cloudfile | application crashed [@ mozilla::EditorBase::InsertTextImpl(nsTSubstring<char16_t> const&, nsCOMPtr<nsINode>*, nsCOMPtr<nsIContent>*, int*, nsIDocument*)] [log…] 

M-C last good: 20d9ad08dd36fe5230ad0ccf6cb3e4865d
M-C first bad: 3d918ff5d63442d7b88e1b7e9cb03b832b

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=20d9ad08dd36fe5230ad0ccf6cb3e4865d&tochange=3d918ff5d63442d7b88e1b7e9cb03b832b

https://hg.mozilla.org/mozilla-central/rev/fdb1abbe808a#l1.131
Bug 1406482.

Ehsan, your new code is asserting in one of our tests:
16:44:22     INFO -  TEST-START | C:\slave\test\build\tests\mozmill\cloudfile\test-cloudfile-attachment-urls.js | test_adding_filelinks_to_empty_forward
...
16:44:28     INFO -  [1852, Main Thread] WARNING: NS_ENSURE_TRUE(sheet) failed: file c:/builds/moz2_slave/tb-c-cen-w32-d-000000000000000/build/mozilla/editor/libeditor/HTMLEditor.cpp, line 2872
16:44:28     INFO -  Assertion failure: node->GetChildAt(offset) == *aInOutChildAtOffset, at c:/builds/moz2_slave/tb-c-cen-w32-d-000000000000000/build/mozilla/editor/libeditor/EditorBase.cpp:2524

The test exercises TB's cloud file attachment function: The user adds a large attachment and chooses to upload it to a cloud file provider. Once uploaded, we insert links into the body of the message. Looks like that calls EditorBase::InsertTextImpl() which you have modified.

Comment 1

[(no longer active)]

Someone needs to debug this to see why the assertion fires, I won't have time to do that, sorry.

Comment 2

[Jorg K (CEST = GMT+2)]

Is this enough information?

=== offset 4
=== node
div@12CA8B80 _moz_dirty="" class="moz-forward-container" state=[100040000000] flags=[00110000] ranges:1 primaryframe=00000000 refcount=29<
  br@12CA8C40 _moz_dirty="" state=[100040000000] flags=[00100000] primaryframe=00000000 refcount=2<>
  br@12CA8CA0 _moz_dirty="" state=[100040000000] flags=[00100000] primaryframe=00000000 refcount=2<>
  Text@2E8306A0 flags=[20000000] primaryframe=00000000 refcount=2<-------- Forwarded Message -------->
  br@12CA8D60 _moz_dirty="" state=[100040000000] flags=[00100000] primaryframe=00000000 refcount=2<>
  br@12CA8BE0 _moz_dirty="" state=[100040000000] flags=[00100000] primaryframe=00000000 refcount=5<>
>
=== child 00000000
=== node->GetChildAt(offset) 12CA8BE0
br@12CA8BE0 _moz_dirty="" state=[100040000000] flags=[00100000] primaryframe=00000000 refcount=5<>
Assertion failure: node->GetChildAt(offset) == *aInOutChildAtOffset, at c:/mozilla-source/comm-central/mozilla/editor/libeditor/EditorBase.cpp:2532
#01: mozilla::EditorBase::InsertTextImpl (c:\mozilla-source\comm-central\mozilla\editor\libeditor\editorbase.cpp:2532)
#02: mozilla::HTMLEditor::InsertTextImpl (c:\mozilla-source\comm-central\mozilla\editor\libeditor\htmleditor.cpp:3130)
#03: mozilla::HTMLEditRules::WillInsertText (c:\mozilla-source\comm-central\mozilla\editor\libeditor\htmleditrules.cpp:1404)
#04: mozilla::HTMLEditRules::WillDoAction (c:\mozilla-source\comm-central\mozilla\editor\libeditor\htmleditrules.cpp:644)
#05: mozilla::TextEditor::InsertText (c:\mozilla-source\comm-central\mozilla\editor\libeditor\texteditor.cpp:669)
#06: mozilla::HTMLEditor::InsertTextWithQuotations (c:\mozilla-source\comm-central\mozilla\editor\libeditor\htmleditordatatransfer.cpp:1791)

This is the code that produced the output:
  printf("=== offset %d\n", offset);
  nsCOMPtr<nsIContent> n(do_QueryInterface(node));
  printf("=== node\n");
  n->List();
  printf("=== child %p\n", (void*)child);
  if (child) child->List();
  printf("=== node->GetChildAt(offset) %p\n", (void*)(node->GetChildAt(offset)));
  if (node->GetChildAt(offset)) node->GetChildAt(offset)->List();
  MOZ_ASSERT(node->GetChildAt(offset) == *aInOutChildAtOffset);

Look like nsCOMPtr<nsIContent> child = *aInOutChildAtOffset; was passed in as null.

If I read your patch correctly, that comes from the selChild set here:
https://hg.mozilla.org/mozilla-central/rev/fdb1abbe808a#l3.12

Other debug examples that don't assert look like this:
=== offset 0
=== node
div@148F67C0 class="anonymous-div" state=[100020000000] flags=[00100670] ranges:1 primaryframe=140C5698 refcount=19<>
=== child 00000000
=== node->GetChildAt(offset) 00000000

or this:

=== offset 2
=== node
body@0E814520 style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;" state=[100040000000] flags=[00100004] ranges:1 primaryframe=17DCB9B8 refcount=86<
  Text@0BA11920 flags=[22000000] primaryframe=17BB0730 refcount=221<This is a line of text>
  br@0C9FECA0 _moz_dirty="" state=[100040000000] flags=[00100000] primaryframe=17BB0838 refcount=9<>
  br@12BB35E0 _moz_dirty="" state=[100040000000] flags=[00100000] primaryframe=17BAFDD8 refcount=10<>
  br@12BB3520 _moz_dirty="" state=[100040000000] flags=[00100000] primaryframe=17BAFD08 refcount=4<>
  div@12BB3820 _moz_dirty="" class="moz-cite-prefix" state=[100040000000] flags=[00100000] primaryframe=17BAF998 refcount=7<
    Text@17BD2DD0 flags=[03000000] primaryframe=17BAFCA8 refcount=2<On 01/02/2000 00:00, Andy Anway wrote:>
    br@12BB3880 _moz_dirty="" state=[100040000000] flags=[00100000] primaryframe=17BAFC58 refcount=2<>
  >
  span@12BB3A00 _moz_dirty="" _moz_quote="true" style="white-space: pre-wrap; display: block; width: 98vw;" state=[100040000000] flags=[00100000] primaryframe=17BAF800 refcount=17<
    Text@12BBB0B0 flags=[22000000] primaryframe=17BAFBF8 refcount=4<&gt; Hello Bob Bell!>
    br@12BB3BE0 _moz_dirty="" state=[100040000000] flags=[00100000] primaryframe=17BAF948 refcount=4<>
    Text@12BBB1A0 flags=[22000000] primaryframe=17BAF8E8 refcount=4<&gt; >
    br@12BB3D00 _moz_dirty="" state=[100040000000] flags=[00100000] primaryframe=17BAF7B0 refcount=3<>
  >
  br@12BB36A0 _moz_dirty="" type="_moz" state=[100040000000] flags=[00100000] primaryframe=17DCCD98 refcount=7<>
>
=== child 12BB35E0
br@12BB35E0 _moz_dirty="" state=[100040000000] flags=[00100000] primaryframe=17BAFDD8 refcount=10<>
=== node->GetChildAt(offset) 12BB35E0
br@12BB35E0 _moz_dirty="" state=[100040000000] flags=[00100000] primaryframe=17BAFDD8 refcount=10<>

Comment 3

[(no longer active)]

The question is why |selChild| there is null, that's unexpected.  My guess is that InsertTextImpl() is getting called in a loop and our variable isn't properly updated somehow.

Like I said, I won't have time to look at this (I will be away for a few months starting from tomorrow).  Please don't needinfo me.  Thanks!

Comment 4

[Jorg K (CEST = GMT+2)]

I wanted to avoid this discussion, but if the author has no time to take of regressions, then it should simply backed out.

I don't want to get into another misunderstanding/friction/etc. over this, but it looks like you caused a regression in the Core::Editor. I guess it's OK that the debugging left to others because no FF test picked it up but some other test in an Gecko-based product. But typically the patch author should address the regression, especially if they were notified within less then 24 hours of the patch landing.

So can we back this out until you return? IMHO we can't rule out that this effects typical editing situations. Sorry about the last NI that appears necessary here.

Comment 5

[Jorg K (CEST = GMT+2)]

Masayuki, you're the editor module owner, can we back this out? Ehsan has blocked NI requests.

Comment 6

[(no longer active)]

We don't back out patches for causing regressions, we instead _fix_ those regressions.  I don't know what has given you the impression that a backout is the right response to a regression, but whatever it is, it's time for you to correct your expectation.  Backouts are only the solution for changes that break something in a serious way for one of our Tier 1 platforms.

Again, I am not going to be around after tomorrow, so I will not have time to look into this, but I think this should definitely be fixed.  Please work with Masayuki to find another engineer to work on it.  Thanks!

Comment 7

[(no longer active)]

Having a test case that reproduces this against Firefox would be helpful.  If you can get one by tomorrow I will try to have a look at this before I leave.  :-)

Comment 8

[Jorg K (CEST = GMT+2)]

Thanks for the offer, but it won't be easy to reproduce this in FF. A debug version of TB asserts when you use the function that does a plaintext forward. That's one things the test does and where it fails. Debug is the same as at the start of comment #2. So I guess another plaintext editor problem not applicable to your Tier 1 platforms.

Comment 9

[(no longer active)]

Based on reading the code, I have a guess as to what may have caused this...  Look at the way we're updating selChild here: <https://searchfox.org/mozilla-central/rev/ed1d5223adcdc07e9a2589ee20f4e5ee91b4df10/editor/libeditor/HTMLEditRules.cpp#1453>

But look at what InsertBreak() which calls CreateBRImpl() does: <https://searchfox.org/mozilla-central/rev/ed1d5223adcdc07e9a2589ee20f4e5ee91b4df10/editor/libeditor/TextEditor.cpp#468>

So offset may be increased more than one but selChild will be moved only one time per loop iteration.  I'll write a patch to fix this.  Not sure if that'll help this assertion failure or not.

Comment 10

[(no longer active)]

Jorg, can you please try the patch in bug 1408227, and see if that changes the behavior of this test in any way by any chance?

Comment 11

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

Canceling ni. Please re-request if I still need to comment something here.

Comment 12

[Jorg K (CEST = GMT+2)]

Thanks, I tried the patch from bug 1408227 and sadly see no improvement, I get the same assert:
Assertion failure: node->GetChildAt(offset) == *aInOutChildAtOffset.

I understand that it's of course better to fix a regression than to backout the bug which caused it. However, given the problem here and bug 1408227 (and maybe bug 1408125) which would have to land today and then would not have the original author available to address problems, I still think backing out bug 1406482 and then addressing the entire area when you return would be the safer option. Rushing changes through in the somewhat brittle editor and then being absent doesn't appear to be the most prudent process. GetChildAt() won't change to slow linear processing while you're away, or will it?

Comment 13

[(no longer active)]

No, still the right thing to do is to fix the regression.

This bug needs an owner...

Comment 14

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

Looking the code briefly, there are two problems. One is a part of the patch for bug 1408227. HTMLEditRules::WillInsertText() doesn't adjust selChild at creating a <br> node. The other is, EditorBase::InsertTextImpl() shouldn't assume the child node is always non-nullptr. E.g., when the container node is a text node or the offset is same as length of the container node. In these cases, nsRange::GetChildAtStartOffset() can return nullptr.

I have written two test patches, but tryserver is closed now :-(

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(offline: 10/17 and 10/23) from comment #14)
> I have written two test patches, but tryserver is closed now :-(
Only closed for Windows, you can run your test on another platform.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

FWIW, we have a fuzzer testcase hitting this now too - bug 1409520.

Comment 17

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

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

Comment 18

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

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

Comment 19

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/try/rev/c7c4c690b19e0fa06a4290c6306e46f06a7752b5
fixes the problem we observe in TB, \o/ !!

One nit:
https://hg.mozilla.org/try/rev/3ef0f32dd61be96c8c843fb732d399bd392a2a2c#l1.16
"The Child must be a child node at |offset| in |node| unless it's a text or some other data node" or
"The Child must be a child node at |offset| in |node| unless it's a text or some data node"

Comment 20

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

(In reply to Jorg K (GMT+2) from comment #19)
> https://hg.mozilla.org/try/rev/c7c4c690b19e0fa06a4290c6306e46f06a7752b5
> fixes the problem we observe in TB, \o/ !!
> 
> One nit:
> https://hg.mozilla.org/try/rev/3ef0f32dd61be96c8c843fb732d399bd392a2a2c#l1.16
> "The Child must be a child node at |offset| in |node| unless it's a text or
> some other data node" or
> "The Child must be a child node at |offset| in |node| unless it's a text or
> some data node"

Really thank you for your confirmation and really sorry for the long delay. I'll request review soon.

Comment 21

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

Attachment: Bug 1407966 - part 1: Update |selChild| when HTMLEditRules::WillInsertText() updates |curNode| and |curOffset|

|selChild| should be a node which is at |curOffset| in |curNode|, however,
it's not updated when |curOffset| is updated by a call of CreateBRImpl().  So,
it should be next sibling of the created br node.

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

Comment 22

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

Attachment: Bug 1407966 - part 2: EditorBase::InsertTextImpl() shouldn't assume that aInOutChildAtOffset is never nullptr

Since if the container is not a container node like a text node or the offset
is same as the length, child at the offset can be nullptr.

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

Comment 23

[Makoto Kato [:m_kato]]

Comment on attachment 8919822 [details]
Bug 1407966 - part 1: Update |selChild| when HTMLEditRules::WillInsertText() updates |curNode| and |curOffset|

https://reviewboard.mozilla.org/r/190754/#review196148

Comment 24

[Makoto Kato [:m_kato]]

Comment on attachment 8919823 [details]
Bug 1407966 - part 2: EditorBase::InsertTextImpl() shouldn't assume that aInOutChildAtOffset is never nullptr

https://reviewboard.mozilla.org/r/190756/#review196150

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad96b27c86c5
part 1: Update |selChild| when HTMLEditRules::WillInsertText() updates |curNode| and |curOffset| r=m_kato
https://hg.mozilla.org/integration/autoland/rev/760469149d9d
part 2: EditorBase::InsertTextImpl() shouldn't assume that aInOutChildAtOffset is never nullptr r=m_kato

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ad96b27c86c5
https://hg.mozilla.org/mozilla-central/rev/760469149d9d