2052101 - Mork memory safety issues/crash reports (cumulative fix for remaining known Mork issues)

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Max Emig [:maxe]]

I am attempting a more massive at every currently reachable crash-signature of Mork.

My assumption is, that any changes there need a massive validation for correctness anyway, so I might as well include everything in one draft attempt. This is deliberately set to WIP because at least the comments make no sense in the final code.

This is heavily machine-assisted, both frontier LLM and non-LLM. However, it still took multiple days and plenty of headaches.

Whether we can merge it at some point in one go or split it up into smaller pieces remains to be seen.

---

Otherwise, this is just a standard meta-bug that related bugs can be linked to.

Comment 1

[Max Emig [:maxe]]

Attachment: Bug 2052101 - Improve Mork memory safety to prevent at least 13 distinct crash signatures and add some narration. r=#thunderbird-back-end-reviewers

Comment 2

[Francesco (no longer active, use PM for contact)]

Nice job, but you mixed up the dependencies.

Comment 3

[Max Emig [:maxe]]

(In reply to Francesco from comment #2)

Nice job, but you mixed up the dependencies.

Heyo,

I do not think so.

These and some more would be solved if D309986 lands, not your usual meta-bug.

My rationale is that any risk, testing (and worst-case back-out) is much easier to manage with one large fix than several smaller ones. If the landing order is bad, this could lead to rebasing nightmares.

Let's wait on our first code review ^^.

Cheers

Max

Comment 4

[Francesco (no longer active, use PM for contact)]

(In reply to Max Emig [:maxe] from comment #3)

I do not think so.

Oh, boy, yes, you're right, BUT then this is not a Meta bug.

Comment 5

[Max Emig [:maxe]]

Oh, boy, yes, you're right, BUT then this is not a Meta bug.

Is there any precedent for what this is, how should we call it?

Comment 6

[Wayne Mery (:wsmwk)]

(In reply to Max Emig [:maxe] from comment #5)

Oh, boy, yes, you're right, BUT then this is not a Meta bug.

Correct. Not all bugs that have many blockers or dependencies are meta bugs. https://bugzilla.mozilla.org/describekeywords.cgi#meta

Is there any precedent for what this is, how should we call it?

A bug that fixes other bugs :)
There is no keyword for such bugs.

Comment 7

[Max Emig [:maxe]]

A bug that fixes other bugs :)

By design to keep it more manageable and safe.

Comment 8

[Max Emig [:maxe]]

Now we will need to account for fixed crashes vs those that remained open.

Comment 9

[Max Emig [:maxe]]

The crash signatures I just added are not tracked in any bug, often very infrequent but are supposed to be fixed here, so I track them here instead of opening 6 new bugs.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/51fc2b250dec
Improve Mork memory safety to prevent at least 13 distinct crash signatures and add some narration. r=BenC

Comment 11

[Wayne Mery (:wsmwk)]

morkBookAtom::HashAid long term rate is 0.8 crashes per day.

Most of the crashes sampled go off the rails in js. A rare stack that does not is Crash report: https://crash-stats.mozilla.org/report/index/6cd52c57-8969-4e61-b237-7e74b0260721 v152.0.1

Reason:

EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames:

0  xul.dll  morkBookAtom::HashAid() const  mailnews/db/mork/morkAtom.h:210
0  xul.dll  morkAtomAidMap::Hash(morkEnv*, void const*) const  mailnews/db/mork/morkAtomMap.cpp:146
1  xul.dll  morkMap::grow(morkEnv*)  mailnews/db/mork/morkMap.cpp:471
2  xul.dll  morkMap::Put(morkEnv*, void const*, void const*, void*, void*, unsigned char**)  mailnews/db/mork/morkMap.cpp:501
3  xul.dll  morkAtomAidMap::AddAtom(morkEnv*, morkBookAtom*)  mailnews/db/mork/morkAtomMap.cpp:157
3  xul.dll  morkAtomSpace::MakeBookAtomCopyWithAid(morkEnv*, morkFarBookAtom const&, unsi...  mailnews/db/mork/morkAtomSpace.cpp:157
4  xul.dll  morkBuilder::OnAlias(morkEnv*, morkSpan const&, morkMid const&)  mailnews/db/mork/morkBuilder.cpp:546
5  xul.dll  morkParser::ReadAlias(morkEnv*)  mailnews/db/mork/morkParser.cpp:855
6  xul.dll  morkParser::ReadDict(morkEnv*)  mailnews/db/mork/morkParser.cpp:1103
7  xul.dll  morkParser::ReadContent(morkEnv*, unsigned char)  mailnews/db/mork/morkParser.cpp:1196

Comment 12

[Wayne Mery (:wsmwk)]

morkMap::find long term rate is 0.6 crashes per day.

Crash report: https://crash-stats.mozilla.org/report/index/d0cb2a83-37d8-4ccf-92ff-5c4c90260805 v140.13.0esr

Reason:

EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames:

0  xul.dll  morkMap::find(morkEnv*, void const*, unsigned int) const  mailnews/db/mork/morkMap.cpp:222
0  xul.dll  morkMap::Get(morkEnv*, void const*, void*, void*, unsigned char**)  mailnews/db/mork/morkMap.cpp:606
1  xul.dll  morkRowMap::GetOid(morkEnv*, mdbOid const*)  mailnews/db/mork/morkRowMap.cpp:120
2  xul.dll  morkTable::MapHasOid(morkEnv*, mdbOid const*)  mailnews/db/mork/morkTable.cpp:1098
2  xul.dll  morkTable::HasOid(nsIMdbEnv*, mdbOid const*, unsigned char*)  mailnews/db/mork/morkTable.cpp:506
3  xul.dll  nsMsgDatabase::ContainsKey(unsigned int, bool*)  mailnews/db/msgdb/src/nsMsgDatabase.cpp:1820
4  xul.dll  nsImapMailFolder::SyncFlags(nsIImapFlagAndUidState*)  mailnews/imap/src/nsImapMailFolder.cpp:4636
5  xul.dll  nsImapMailFolder::UpdateImapMailboxInfo(nsIImapProtocol*, nsIMailboxSpec*)  mailnews/imap/src/nsImapMailFolder.cpp:2687
6  xul.dll  (anonymous namespace)::SyncRunnable2<nsIImapProtocolSink, nsIMsgMailNewsUrl*,...  mailnews/imap/src/nsSyncRunnableHelpers.cpp:123
7  xul.dll  mozilla::RunnableTask::Run()  xpcom/threads/TaskController.cpp:703

Comment 13

[Wayne Mery (:wsmwk)]

morkMap::new_assocs long term rate is 0.6 crashes per day.

Crash report: https://crash-stats.mozilla.org/report/index/e0a08081-2a35-4294-a11c-05ef80260803 v152.0.1

Reason:

EXCEPTION_ACCESS_VIOLATION_WRITE

Top 10 frames:

0  xul.dll  morkMap::new_assocs(morkEnv*, unsigned int)  mailnews/db/mork/morkMap.cpp:350
0  xul.dll  morkMap::new_arrays(morkEnv*, morkHashArrays*, unsigned int)  mailnews/db/mork/morkMap.cpp:363
1  xul.dll  morkMap::grow(morkEnv*)  mailnews/db/mork/morkMap.cpp:446
2  xul.dll  morkMap::Put(morkEnv*, void const*, void const*, void*, void*, unsigned char**)  mailnews/db/mork/morkMap.cpp:501
3  xul.dll  morkAtomAidMap::AddAtom(morkEnv*, morkBookAtom*)  mailnews/db/mork/morkAtomMap.cpp:157
3  xul.dll  morkAtomSpace::MakeBookAtomCopy(morkEnv*, morkFarBookAtom const&)  mailnews/db/mork/morkAtomSpace.cpp:189
4  xul.dll  morkStore::YarnToAtom(morkEnv*, mdbYarn const*, bool)  mailnews/db/mork/morkStore.cpp:598
5  xul.dll  morkRow::AddColumn(morkEnv*, unsigned int, mdbYarn const*, morkStore*)  mailnews/db/mork/morkRow.cpp:707
6  xul.dll  morkRowObject::AddColumn(nsIMdbEnv*, unsigned int, mdbYarn const*)  mailnews/db/mork/morkRowObject.cpp:205
7  xul.dll  nsMsgDatabase::UInt64ToRowCellColumn(nsIMdbRow*, unsigned int, unsigned long ...  mailnews/db/msgdb/src/nsMsgDatabase.cpp:3299

Comment 14

[Wayne Mery (:wsmwk)]

The crash rate of the other crash signatures currently documented here are so low as to not be worth cataloging.

Comment 15

[Wayne Mery (:wsmwk)]

In bug 1981802 comment 9 you write "In bug 2052101 I attempted to guard and so far it's working." - I'm curious how you measure "working", given the patch is only active in daily builds, and there is no history bug 1981802's nor this bug's crash signatures in daily? Even in beta channel, only morkMap::new_assocs has any history of crashes.

Comment 16

[Max Emig [:maxe]]

(In reply to Wayne Mery (:wsmwk) from comment #15)

In bug 1981802 comment 9 you write "In bug 2052101 I attempted to guard and so far it's working." - I'm curious how you measure "working", given the patch is only active in daily builds, and there is no history bug 1981802's nor this bug's crash signatures in daily? Even in beta channel, only morkMap::new_assocs has any history of crashes.

No longer reproducible with some absurd induced errors. And yeah, you're right I didn't notice that there previously also weren't any on Socorro.