<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Comment 13

•

17 years ago

(In reply to comment #10) > As I understand it, this is something an extension might do; the core > thunderbird code doesn't do this. What does Thunderbird do differently? We appear to AddFolderListener() from js in several places. Although on second look they appear to be called on mail sessions rather than folders, which does use a nsCOMPtr in there but looks kinda wonky the way it's doing it: it's copying a stack-allocated struct containing the comptr into the observer array -- that's not going to addref the comptr is it? http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/src/nsMsgMailSession.cpp&rev=1.92&mark=89,99,100#89 So this is a booby-trapped API, we need to either fix it to use comptrs in the array or make the unsafe entries unreachable from script. I imagine we can't remove scriptable on the whole API but we could put [noscript] on AddFolderListener.

Whiteboard: [sg:critical?] → [sg:moderate] critical hazard for unwary js code

Nobody; OK to take it and work on it

Comment 14

•

16 years ago

fwiw, nothing on crash-stats since jan 2009

Severity: normal → critical

Summary: Crash when adding listener to local folder [@nsMsgDBFolder::NotifyItemAdded] → Crash when adding listener to local folder [@nsMsgDBFolder::NotifyItemAdded(nsISupports*)]

Updated

•

15 years ago

Crash Signature: [@nsMsgDBFolder::NotifyItemAdded(nsISupports*)]

Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]

Updated

•

14 years ago

Keywords: sec-moderate

Comment 15

•

14 years ago

(In reply to Wayne Mery (:wsmwk) from comment #14) > fwiw, nothing on crash-stats since jan 2009 after multi year hiatus, this crash sig appears again in TB12 - for example bp-e77465c8-8e81-4a38-991d-a38b22120525 - and continues in current releases like bp-c5c99d2c-3634-44d9-ab46-572f32120808 TB15 a common comment is performing UNDO

Crash Signature: [@nsMsgDBFolder::NotifyItemAdded(nsISupports*)] → [@ nsMsgDBFolder::NotifyItemAdded(nsISupports*)] [@ nsMsgDBFolder::NotifyItemAdded ]

OS: Mac OS X → All

Summary: Crash when adding listener to local folder [@nsMsgDBFolder::NotifyItemAdded(nsISupports*)] → Crash when adding listener to local folder [@ nsMsgDBFolder::NotifyItemAdded(nsISupports*)]

Comment 16

•

13 years ago

who is currently operating in the area of comment 14 that could address this? Is addon dev doc/posting needed even if this gets fixed? bug 380167 is related? current crash example: bp-de2d9402-9dbc-40dd-b4ee-cf5922130819 0 XUL nsMsgDBFolder::NotifyItemAdded mailnews/base/util/nsMsgDBFolder.cpp 1 XUL nsMsgThreadedDBView::OnNewHeader mailnews/base/src/nsMsgThreadedDBView.cpp 2 XUL nsMsgDatabase::NotifyHdrAddedAll mailnews/db/msgdb/src/nsMsgDatabase.cpp 3 XUL nsMsgDatabase::AddNewHdrToDB mailnews/db/msgdb/src/nsMsgDatabase.cpp 4 XUL nsMsgLocalMailFolder::EndCopy mailnews/local/src/nsLocalMailFolder.cpp 5 XUL morkAtomSpace::MakeBookAtomCopy db/mork/src/morkAtomSpace.cpp

Crash Signature: [@ nsMsgDBFolder::NotifyItemAdded(nsISupports*)] [@ nsMsgDBFolder::NotifyItemAdded ] → [@ nsMsgDBFolder::NotifyItemAdded(nsISupports*)] [@ nsMsgDBFolder::NotifyItemAdded]

•

13 years ago

Comment on attachment 806517 [details] [diff] [review] Hold a reference to the listener > // This can't be refcounted due to ownsership issues >- nsTObserverArray<nsIFolderListener*> mListeners; >+ nsTObserverArray<nsCOMPtr<nsIFolderListener> > mListeners; Nit: remove out-of-date comment >+ mMessageFolder = do_GetWeakReference(folder); >+ if (!mMessageFolder) >+ return NS_ERROR_FAILURE; >+ > // XXX may want to send messages X seconds after startup if there are any. > > return NS_OK; mMessageFolder = do_GetWeakReference(folder, &rv); return rv; > if (mMessageFolder) > { >- rv = mMessageFolder->RemoveFolderListener(this); >+ nsCOMPtr<nsIMsgFolder> folder = do_QueryReferent(mMessageFolder, &rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = folder->RemoveFolderListener(this); > NS_ENSURE_SUCCESS(rv, rv); > } Lack of a weak reference during shutdown is probably the point, not an error: nsCOMPtr<nsIMsgFolder> folder = do_QueryReferent(mMessageFolder); if (folder) { rv = folder->RemoveFolderListener(this); NS_ENSURE_SUCCESS(rv, rv); } > // XXX This code should be set up for multiple unsent folders, however we > // don't support that at the moment, so for now just assume one folder. > if (!mMessageFolder) > { >+ nsCOMPtr<nsIMsgFolder> folder; > rv = GetUnsentMessagesFolder(nullptr, >+ getter_AddRefs(folder)); > NS_ENSURE_SUCCESS(rv, rv); >+ mMessageFolder = do_GetWeakReference(folder); >+ if (!mMessageFolder) >+ return NS_ERROR_FAILURE; > } > rv = ReparseDBIfNeeded(nullptr); > NS_ENSURE_SUCCESS(rv, rv); > > int32_t totalMessages; >+ nsCOMPtr<nsIMsgFolder> folder = do_QueryReferent(mMessageFolder, &rv); >+ NS_ENSURE_SUCCESS(rv, rv); I am not sure that this is safe; when you drop the strong reference to folder, you might lose the object and your weak reference fails. Or indeed you might have fetched the weak reference earlier and the object has since gone away. Instead I would improve GetUnsentMessagesFolder to try getting the weak reference from mMessageFolder first, and then consistently use GetUnsentMessagesFolder everywhere. Pseudocode: function GetUnsentMessagesFolder(aRetval) { var folder = do_QueryReferent(mMessageFolder); if (!folder) { folder = LocateMessageFolder(GetFolderURIFromUserPrefs()); mMessageFolder = do_GetWeakReference(folder); } folder.forget(aRetval); } r- based on the apparently unsafe weak reference usage. If Mark thinks that the folder object will be held alive until shutdown for other reasons then this may actually be safe in which case r+ with the other three nits fixed.

Attachment #806517 - Flags: review?(neil) → review-

Updated

•

12 years ago

Group: mail-core-security

Ludovic Hirlimann [:Usul]

•

11 years ago

Group: core-security → mail-core-security

Comment 22

•

11 years ago

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Geoff Lankow (:darktrojan) [PTO, back 2 Nov]

Updated

•

9 years ago

Blocks: 380167

Whiteboard: [sg:moderate] critical hazard for unwary js code → [sg:moderate][critical hazard for unwary js code][patchlove]

Magnus Melin [:mkmelin]

Comment 23

•

8 years ago

I imagine this is still alive, at least bug 873426 has recent crashes. If it's the same.

Comment 24

•

8 years ago

Attached patch 409458-folder-listener-1.diff (obsolete) — Details — Splinter Review

Here's an updated patch with the comments addressed. I'm not sure who should review this, so I'll leave that up to you, Magnus.

Flags: needinfo?(mkmelin+mozilla)

Magnus Melin [:mkmelin]

Comment 25

•

8 years ago

Comment on attachment 9005102 [details] [diff] [review] 409458-folder-listener-1.diff Review of attachment 9005102 [details] [diff] [review]: ----------------------------------------------------------------- Hoping Jörg can make sense of it.

Attachment #9005102 - Flags: review?(jorgk)

Magnus Melin [:mkmelin]

Updated

•

8 years ago

Flags: needinfo?(mkmelin+mozilla)

Assignee

Comment 26

•

8 years ago

Sorry about the delay. Something to look at on a relaxed Sunday evening ;-) Looks like there was a previous try run https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=76410d27145a103b2abcc9b9f966e7ea1b66efb7 but let's have a new one since Linux is working again: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=dae737884e0a05d352766fc238df37d1baf10014 Better safe than sorry. The reason for the r- from the end of comment #19 was not addressed. Neil requested something like function GetUnsentMessagesFolder(aRetval) { var folder = do_QueryReferent(mMessageFolder); if (!folder) { folder = LocateMessageFolder(GetFolderURIFromUserPrefs()); mMessageFolder = do_GetWeakReference(folder); } folder.forget(aRetval); } which the comment that the folder object may disappear which was (sort of) confirmed in comment #21. Geoff, did you look into that issue further? Should we give that approach a shot?

Assignee

Comment 27

•

8 years ago

Aceman has been involved in folder caching and clean-up, so maybe he has an opinion here. Since this bug is pretty ancient (11 y/o), another couple of days won't make a great difference now.

Flags: needinfo?(acelists)

Assignee

Comment 28

•

8 years ago

Comment on attachment 9005102 [details] [diff] [review] 409458-folder-listener-1.diff Review of attachment 9005102 [details] [diff] [review]: ----------------------------------------------------------------- Terribly sorry, ignore comment #26. Neil's comment #19 was fully, well, almost fully, implemented. Just one more question. ::: mailnews/compose/src/nsMsgSendLater.cpp @@ +127,3 @@ > NS_ENSURE_SUCCESS(rv, rv); > > + mMessageFolder = do_GetWeakReference(folder, &rv); Move this into GetUnsentMessagesFolder()? @@ +671,5 @@ > + nsCOMPtr<nsIMsgFolder> folder = do_QueryReferent(mMessageFolder); > + if (!folder) { > + nsCString uri; > + GetFolderURIFromUserPrefs(nsIMsgSend::nsMsgQueueForLater, aIdentity, uri); > + rv = LocateMessageFolder(aIdentity, nsIMsgSend::nsMsgQueueForLater, uri.get(), getter_AddRefs(folder)); Add mMessageFolder = do_GetWeakReference(folder); here as Neil suggested? @@ +710,3 @@ > return NS_OK; > + > + mMessageFolder = do_GetWeakReference(folder); Move this into GetUnsentMessagesFolder()?

Assignee

Updated

•

8 years ago

Flags: needinfo?(acelists)

Assignee

Comment 29

•

8 years ago

Documentation isn't great, but reading the IDL helps: https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsIWeakReference.idl

Assignee

Comment 30

•

8 years ago

Comment on attachment 9005102 [details] [diff] [review] 409458-folder-listener-1.diff Haha, try failed since it's MailServices.jsm now. Also, we use ChromeUtils.import now. I'll attach my suggestion with a new try in a moment.

Attachment #9005102 - Flags: review?(jorgk)

Geoff Lankow (:darktrojan) [PTO, back 2 Nov]

Assignee

Comment 31

•

8 years ago

Attached patch 409458-folder-listener-1.diff (JK) (obsolete) — Details — Splinter Review

I moved the lines in question. Any objection to this? Maybe the try has: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cf924dfa66cb4054a38083183c17963d79dc5f07

Assignee: nobody → jorgk

Attachment #9007627 - Flags: feedback?(geoff)

Comment 32

•

8 years ago

Comment on attachment 9007627 [details] [diff] [review] 409458-folder-listener-1.diff (JK) Review of attachment 9007627 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, apart from the comment below. ::: mailnews/compose/src/nsMsgSendLater.cpp @@ +792,1 @@ > NS_ENSURE_SUCCESS(rv, rv); We checked for NS_FAILED(rv) || !folder in the other places, does that need to happen here?

Attachment #9007627 - Flags: feedback?(geoff) → feedback+

Assignee

Comment 33

•

8 years ago

Comment on attachment 9007627 [details] [diff] [review] 409458-folder-listener-1.diff (JK) Review of attachment 9007627 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/compose/src/nsMsgSendLater.cpp @@ +792,1 @@ > NS_ENSURE_SUCCESS(rv, rv); The other two callsites have the comment: // There doesn't have to be a nsMsgQueueForLater flagged folder. So they explicitly ignore the fact that it doesn't exist. This code here is in InternalSendMessages(), so we're already sending messages. But I can see your point, I'll add it.

Geoff Lankow (:darktrojan) [PTO, back 2 Nov]

Assignee

Comment 34

•

8 years ago

Attached patch 409458-folder-listener-1.diff (JK, v2) — Details — Splinter Review

Please use interdiff.

Attachment #806517 - Attachment is obsolete: true

Attachment #9005102 - Attachment is obsolete: true

Attachment #9007627 - Attachment is obsolete: true

Attachment #9007703 - Flags: review+

Attachment #9007703 - Flags: feedback?(geoff)

Updated

•

8 years ago

Attachment #9007703 - Flags: feedback?(geoff) → feedback+

Assignee

Comment 35

•

8 years ago

https://hg.mozilla.org/comm-central/rev/2cfa24cfd1ac6984457e9bd952fb2e347670faba nsIMsgFolder.AddFolderListener should hold a reference to the listener. r=standard8,neil,darktrojan,jorgk Fixed after 11 years, four reviewers.

Status: NEW → RESOLVED

Closed: 8 years ago

Resolution: --- → FIXED

Target Milestone: --- → Thunderbird 64.0

Updated

•

8 years ago

Group: mail-core-security → core-security-release

Comment 37

•

8 years ago

Jorg, thanks for the patch. An observation - the crash rate so far for version 60 (only 1.6% of crashes) is much lower than 52.x [1]. Perhaps it is because of fewer addons running on v60. The only user we have is Adam from bug 873426 (if he's even still around), We'll need to check in 3-4 weeks after automatic updates have moved more users, and also check beta stats because there are a few crashes per week for beta. A little troubling, is the crash data shows *many* users not on the most recent versions of 52.x. So will also need to dive into what is causing that. More generally, perhaps a high percentage are on OS not supported in version 60. Thunderbird 52.9.1 529 39.0% 477 Thunderbird 52.8.0 303 22.3% 578 Thunderbird 52.7.0 283 20.9% 191 Thunderbird 45.8.0 67 4.9% 56 Thunderbird 52.9.0 30 2.2% 34 Thunderbird 52.4.0 26 1.9% 28 Thunderbird 60.0 22 1.6% 16 Thunderbird 52.6.0 19 1.4% 20 [1] two weeks data - https://crash-stats.mozilla.com/signature/?signature=nsMsgDBFolder%3A%3ANotifyItemAdded&date=%3E%3D2018-04-14T04%3A21%3A56.000Z&date=%3C2018-10-14T04%3A21%3A56.000Z#summary

Crash Signature: [@ nsMsgDBFolder::NotifyItemAdded(nsISupports*)] [@ nsMsgDBFolder::NotifyItemAdded] → [@ nsMsgDBFolder::NotifyItemAdded ]

Summary: Crash when adding listener to local folder [@ nsMsgDBFolder::NotifyItemAdded(nsISupports*)] → Crash when adding listener to local folder [@ nsMsgDBFolder::NotifyItemAdded]

Whiteboard: [sg:moderate][critical hazard for unwary js code][patchlove] → [sg:moderate][critical hazard for unwary js code]