Closed Bug 1478268 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Editor, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- unaffected
firefox63 --- fixed

People

(Reporter: jorgk-bmo, Assigned: masayuki)

References

Details

(Keywords: regression)

Attachments

(3 files)

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.
Flags: needinfo?(masayuki)
Flags: needinfo?(m_kato)
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=
BTW, that's a debug crash caused by a MOZ_ASSERT(). The opt crash will be different and a "real" crash.
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7c93ed296fa1
temporarily disable failing sub-tests of test-cloudfile-attachment-urls.js. rs=bustage-fix
Ah, really sorry. This is my mistake. The MOZ_ASSERT should check |!handled|, not |handled| as the comment says. I'll post a patch.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)
Flags: needinfo?(m_kato)
Crash Signature: [@ mozilla::HTMLEditRules::SplitMailCites ]
Crash Signature: [@ mozilla::HTMLEditRules::SplitMailCites ]
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

=============================================================
Crash Signature: [@ mozilla::HTMLEditRules::SplitMailCites ]
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.
Hmm, comment 1 reports about MOZ_ASSERT bug, but the crash is really different bug...
And also I didn't touch HTMLEditRules::WillInsertBreak() in bug 1476897. Are you really sure the crash is caused by the fix?
(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
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.
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?
Flags: needinfo?(t.matsuu)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ffc0f3fcefcc
temporarily disable failing tests test-reply-format-flowed.js. rs=bustage-fix DONTBUILD
(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.
Flags: needinfo?(t.matsuu)
Hmm, bad timing. I filed new clean crash bug at bug 1478605...
Changing the code to this:
  if (previousNodeOfSplitPoint &&
      previousNodeOfSplitPoint->IsHTMLElement(nsGkAtoms::span) &&
      previousNodeOfSplitPoint->GetPrimaryFrame() &&
      previousNodeOfSplitPoint->GetPrimaryFrame()->
                                  IsFrameOfType(nsIFrame::eBlockFrame)) {
fixes the issue.
Attachment #8995140 - Flags: review?(m_kato) → review?(jorgk)
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 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).
Attachment #8995140 - Flags: review?(jorgk) → review+
Comment on attachment 8995140 [details]
Bug 1478268 - Fix condition of MOZ_ASSERT() in TextEditor::InsertWithQuotationsAsSubAction()

https://reviewboard.mozilla.org/r/259618/#review266636
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.
We already lamented the absence of tests, especially for plaintext mode, in bug 1393140 comment #44 and above :-(
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/8cce60bfbdb7
Fix condition of MOZ_ASSERT() in TextEditor::InsertWithQuotationsAsSubAction() r=jorgk+176914
Crash Signature: [@ mozilla::HTMLEditRules::SplitMailCites ]
Component: Testing Infrastructure → Editor
Keywords: leave-openregression
OS: Unspecified → All
Product: Thunderbird → Core
Hardware: Unspecified → All
Summary: PROCESS-CRASH | cloudfile | application crashed [@ mozilla::HTMLEditRules::SplitMailCites(bool*)] → Debug build always crashes at MOZ_ASSERT() in TextEditor::InsertWithQuotationsAsSubAction()
Version: unspecified → Trunk
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.
Attachment #8995140 - Flags: feedback?(m_kato)
https://hg.mozilla.org/mozilla-central/rev/8cce60bfbdb7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b7949186a806
Backed out C-C changesets 7c93ed296fa1 and ffc0f3fcefcc to re-enable tests. a=backout
Attachment #8995140 - Flags: feedback?(m_kato) → feedback+
You need to log in before you can comment on or make changes to this bug.