1478268 - Debug build always crashes at MOZ_ASSERT() in TextEditor::InsertWithQuotationsAsSubAction()

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jorg K (CEST = GMT+2)]

First seen after landing of bug 1476897.

All our MozMill runs, even in opt, now crash, for example here:

https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=a08367c1e5464a339016807c5553067b67e0cb2a&selectedJob=189941971
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=a08367c1e5464a339016807c5553067b67e0cb2a&selectedJob=189942220

You can get a stack from these logs:
https://taskcluster-artifacts.net/egXw6XBkTP-ODPPJBSlqiQ/0/public/logs/live_backing.log
https://taskcluster-artifacts.net/WaepO93dR6Kr4zmTnYiHcA/0/public/logs/live_backing.log
Search for "Application unexpectedly closed"

The test causing this is or MozMill test
cloudfile/test-cloudfile-attachment-urls.js | test_removing_filelinks_removes_root_node

I can run it locally in a minute to get a better stack.

Note: All the debug test failures are due to bug 1478065 / bug 1477904.

Comment 1

[Jorg K (CEST = GMT+2)]

OK, it comes from a MOZ_ASSERT() at TextEditor.cpp#1935:
MOZ_ASSERT(handled, "WillDoAction() shouldn't handle in this case");

Stack is:
xul.dll!mozilla::TextEditor::InsertWithQuotationsAsSubAction(const nsTSubstring<char16_t> & aQuotedText) Line 1935
	at c:\mozilla-source\comm-central\editor\libeditor\texteditor.cpp(1935)
xul.dll!mozilla::HTMLEditor::InsertAsPlaintextQuotation(const nsTSubstring<char16_t> & aQuotedText, bool aAddCites, nsINode * * aNodeInserted) Line 1860
	at c:\mozilla-source\comm-central\editor\libeditor\htmleditordatatransfer.cpp(1860)
xul.dll!mozilla::HTMLEditor::InsertAsQuotation(const nsTSubstring<char16_t> & aQuotedText, nsINode * * aNodeInserted) Line 1779
	at c:\mozilla-source\comm-central\editor\libeditor\htmleditordatatransfer.cpp(1779)
xul.dll!nsMsgCompose::ConvertAndLoadComposeWindow(nsTString<char16_t> & aPrefix, nsTString<char16_t> & aBuf, nsTString<char16_t> & aSignature, bool aQuoted, bool aHTMLEditor) Line 711
	at c:\mozilla-source\comm-central\comm\mailnews\compose\src\nsmsgcompose.cpp(711)
xul.dll!QuotingOutputStreamListener::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult status) Line 3021
	at c:\mozilla-source\comm-central\comm\mailnews\compose\src\nsmsgcompose.cpp(3021)
xul.dll!nsStreamConverter::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult status) Line 1077
	at c:\mozilla-source\comm-central\comm\mailnews\mime\src\nsstreamconverter.cpp(1077)

What happens is that the test starts up a *plaintext* compose window and then tries to insert something into it. 

As you know, plaintext is driven by eEditorPlaintextMask:
https://searchfox.org/mozilla-central/search?q=eEditorPlaintextMask&case=false&regexp=false&path=

Comment 2

[Jorg K (CEST = GMT+2)]

BTW, that's a debug crash caused by a MOZ_ASSERT(). The opt crash will be different and a "real" crash.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1478268-disable-test.patch

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7c93ed296fa1
temporarily disable failing sub-tests of test-cloudfile-attachment-urls.js. rs=bustage-fix

Comment 5

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

Ah, really sorry. This is my mistake. The MOZ_ASSERT should check |!handled|, not |handled| as the comment says. I'll post a patch.

Comment 6

[Takanori MATSUURA]

This bug was filed from the Socorro interface and is
report bp-f0da02b7-f973-459d-97b3-3cf9d0180726.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::HTMLEditRules::SplitMailCites editor/libeditor/HTMLEditRules.cpp:2183
1 xul.dll mozilla::HTMLEditRules::WillInsertBreak editor/libeditor/HTMLEditRules.cpp:1730
2 xul.dll mozilla::HTMLEditRules::WillDoAction editor/libeditor/HTMLEditRules.cpp:695
3 xul.dll mozilla::TextEditor::InsertParagraphSeparatorAsAction editor/libeditor/TextEditor.cpp:1042
4 xul.dll nsMsgCompose::ConvertAndLoadComposeWindow comm/mailnews/compose/src/nsMsgCompose.cpp:730
5 xul.dll QuotingOutputStreamListener::OnStopRequest comm/mailnews/compose/src/nsMsgCompose.cpp:3018
6 xul.dll nsStreamConverter::OnStopRequest comm/mailnews/mime/src/nsStreamConverter.cpp:1070
7 xul.dll nsImapCacheStreamListener::OnStopRequest comm/mailnews/imap/src/nsImapProtocol.cpp:8906
8 xul.dll nsInputStreamPump::OnStateStop netwerk/base/nsInputStreamPump.cpp:706
9 xul.dll nsInputStreamPump::OnInputStreamReady netwerk/base/nsInputStreamPump.cpp:436

=============================================================

Comment 7

[Jorg K (CEST = GMT+2)]

Changing it to
    MOZ_ASSERT(!handled, "WillDoAction() shouldn't handle in this case");
doesn't fix the problem. Instead we now get the crash we see in our opt runs mentioned in comment #2 and reported in comment #6.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1478268-disable-more-tests.patch

Comment 9

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

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

Comment 10

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

Hmm, comment 1 reports about MOZ_ASSERT bug, but the crash is really different bug...

Comment 11

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

And also I didn't touch HTMLEditRules::WillInsertBreak() in bug 1476897. Are you really sure the crash is caused by the fix?

Comment 12

[Jorg K (CEST = GMT+2)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #10)
> Hmm, comment 1 reports about MOZ_ASSERT bug, but the crash is really
> different bug...
Indeed, that's why I added comment #2: The opt crash will be different and a "real" crash.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #11)
> And also I didn't touch HTMLEditRules::WillInsertBreak() in bug 1476897. Are
> you really sure the crash is caused by the fix?
I'm only sure about the regression range:
Last good: 54f69f217483467dbd22e02ab4a98d8d8b
First bad: dd386b5b9fa7f5cd6dc4bbbfa0503b3eb2
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=54f69f217483467dbd22e02ab4a98d8d8b&tochange=dd386b5b9fa7f5cd6dc4bbbfa0503b3eb2

Comment 13

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

If the crash line number is correct, null pointer referring crash must be caused here:
https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/editor/libeditor/HTMLEditRules.cpp#2185
I don't know why existing left node does not have primary frame in some cases. layout team stopped to create frames in some cases?

Anyway, I think that we should make this bug treat the MOZ_ASSERT bug since you and I commented about that, and these comment may make other readers confused. And then, perhaps, we should fix the real crash bug in another bug.

Comment 14

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

Matsuura-san, did you meet the crash? If so, do you remember what did you do and you see something odd visible result immediately before the crash?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ffc0f3fcefcc
temporarily disable failing tests test-reply-format-flowed.js. rs=bustage-fix DONTBUILD

Comment 16

[Jorg K (CEST = GMT+2)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #13)
> Anyway, I think that we should make this bug treat the MOZ_ASSERT bug since
> you and I commented about that, and these comment may make other readers
> confused. And then, perhaps, we should fix the real crash bug in another bug.
I don't agree. The bug was filed with this crash signature @ mozilla::HTMLEditRules::SplitMailCites(bool*) and even in comment #0 I mentioned opt crashes which aren't caused be the MOZ_ASSERT().

But if you want to split it, the MOZ_ASSERT() should go to a different bug.

Debugging this locally, it fails around here:
  if (previousNodeOfSplitPoint &&
      previousNodeOfSplitPoint->IsHTMLElement(nsGkAtoms::span) &&
      previousNodeOfSplitPoint->GetPrimaryFrame()->
                                  IsFrameOfType(nsIFrame::eBlockFrame)) {

It must be that previousNodeOfSplitPoint->GetPrimaryFrame() returned null. I don't see any layout changes in the regression range. Maybe your refactoring triggers another bug.

The bug can be triggered, as test-reply-format-flowed.js suggests, by doing a simple plaintext reply, so Shift+Reply if you're usually working in HTML mode.

Comment 17

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

Hmm, bad timing. I filed new clean crash bug at bug 1478605...

Comment 18

[Jorg K (CEST = GMT+2)]

Changing the code to this:
  if (previousNodeOfSplitPoint &&
      previousNodeOfSplitPoint->IsHTMLElement(nsGkAtoms::span) &&
      previousNodeOfSplitPoint->GetPrimaryFrame() &&
      previousNodeOfSplitPoint->GetPrimaryFrame()->
                                  IsFrameOfType(nsIFrame::eBlockFrame)) {
fixes the issue.

Comment 19

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

Attachment: Bug 1478268 - Fix condition of MOZ_ASSERT() in TextEditor::InsertWithQuotationsAsSubAction()

As explained in the comment of TextEditor::InsertWithQuotationsAsSubAction(),
it excepts that TextEditRules::WillDoAction() won't handle it.  So, the
MOZ_ASSERT() should check |!handled|, not |handled|.

Caused this regression means that we do not have test to paste text into
<textarea>.  Therefore, this patch adds the tests.

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

Comment 20

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

Comment on attachment 8995140 [details]
Bug 1478268 - Fix condition of MOZ_ASSERT() in TextEditor::InsertWithQuotationsAsSubAction()

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

Comment 21

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

Japan is now after dinner, so, Jorg K should review the patches for now. I'll mark them "feedback?" after landing them and fix some nits in follow up bug.

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 8995140 [details]
Bug 1478268 - Fix condition of MOZ_ASSERT() in TextEditor::InsertWithQuotationsAsSubAction()

Looks OK to me, not that I have approval rights in Core::Editor ;-)

Should there be a follow-up, you could always add it here, no?

So this exercises the funny "paste as quotation" even available in FF via pref "middlemouse.paste". Ideally we should have some test that exercise HTMLEditRules::SplitMailCites() to avoid further regressions.

I wrote one once here:
https://hg.mozilla.org/mozilla-central/rev/3110a34c9897
... which is also the code now crashing (and was moved to bug 1478605).

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8995140 [details]
Bug 1478268 - Fix condition of MOZ_ASSERT() in TextEditor::InsertWithQuotationsAsSubAction()

https://reviewboard.mozilla.org/r/259618/#review266636

Comment 24

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

Thank you. I'll update the <tltle> of the new test and land it soon.

> Looks OK to me, not that I have approval rights in Core::Editor ;-)

No problem, I have the rights. And enough simple fix.

> Ideally we should have some test that exercise HTMLEditRules::SplitMailCites() to avoid further regressions.

If you can write it as mochitest in mozilla-central, we can realize Thunderbird specific regression. Like what you wrote. So, if some existing test can be ported simply, it's really nice.

Comment 25

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

Comment on attachment 8995140 [details]
Bug 1478268 - Fix condition of MOZ_ASSERT() in TextEditor::InsertWithQuotationsAsSubAction()

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

Comment 26

[Jorg K (CEST = GMT+2)]

We already lamented the absence of tests, especially for plaintext mode, in bug 1393140 comment #44 and above :-(

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8cce60bfbdb7
Fix condition of MOZ_ASSERT() in TextEditor::InsertWithQuotationsAsSubAction() r=jorgk+176914

Comment 28

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

Comment on attachment 8995140 [details]
Bug 1478268 - Fix condition of MOZ_ASSERT() in TextEditor::InsertWithQuotationsAsSubAction()

This is landed for avoiding automated test failure of Tb, without your review.

If you see some points which should be fixed, let me know. I'll file a follow up bug and fix them.

Comment 29

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8cce60bfbdb7

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b7949186a806
Backed out C-C changesets 7c93ed296fa1 and ffc0f3fcefcc to re-enable tests. a=backout