Closed Bug 1360803 Opened 3 years ago Closed Last year

Crash in nsMsgSearchTerm::MatchString / nsMsgSearchTerm::MatchRfc2047String, mimeConverter is null

Categories

(Thunderbird :: Search, defect, critical)

defect
Not set
critical

Tracking

(thunderbird_esr6064+ fixed, thunderbird64 fixed)

RESOLVED FIXED
Thunderbird 64.0
Tracking Status
thunderbird_esr60 64+ fixed
thunderbird64 --- fixed

People

(Reporter: opera.wang, Assigned: mkmelin)

References

()

Details

(Keywords: crash)

Crash Data

Attachments

(4 files, 8 obsolete files)

2.79 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
14.04 KB, patch
jorgk
: feedback-
Details | Diff | Splinter Review
15.52 KB, patch
Details | Diff | Splinter Review
13.80 KB, patch
mkmelin
: review+
benc
: feedback+
Details | Diff | Splinter Review
This bug was filed from the Socorro interface and is 
report bp-6a2b460f-a0f9-42ec-90d9-edc990170429.
=============================================================

There're 384 crashes for this signature in past 6 months and 5 crashes in last 7 days.

I checked a few of them and seems it related to my extension https://addons.mozilla.org/en-US/thunderbird/addon/awesome-auto-archive/

Basically when AAA is doing search and TB crashed.

Seems it caused by bug 1089298, m_value.utf16String was not initialized?

https://bugzilla.mozilla.org/show_bug.cgi?id=1089298#c5 Kent James mentioned that
"1) I believe that you also need to initialize mValue.utf16String in nsMsgSearchValueImpl::nsMsgSearchValueImpl(nsMsgSearchValue *aInitialValue)
"
But the changes does not take that.
Attached patch 1360803-fix-crash.patch (obsolete) — Splinter Review
Assignee: nobody → jorgk
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8863111 - Flags: review?(rkent)
> There're 384 crashes for this signature in past 6 months and 5 crashes in
> last 7 days.
> 
> I checked a few of them and seems it related to my extension
> https://addons.mozilla.org/en-US/thunderbird/addon/awesome-auto-archive/
> 
> Basically when AAA is doing search and TB crashed.
> 
> Seems it caused by bug 1089298, m_value.utf16String was not initialized?

earliest crashes containing nsMsgSearchTerm::MatchString are 45.1.0 like bp-8b6d0f70-baaf-4ac1-ae0d-fac442161109 but this is long after bug 1089298. Perhaps it was crashing in previous versions with a different signature?


> report bp-6a2b460f-a0f9-42ec-90d9-edc990170429.

 0 	xul.dll	nsAString_internal::Assign(nsAString_internal const&, mozilla::fallible_t const&)	xpcom/string/nsTSubstring.cpp:469
1 	xul.dll	nsAString_internal::Assign(nsAString_internal const&)	xpcom/string/nsTSubstring.cpp:435
2 	xul.dll	nsMsgSearchTerm::MatchString(nsAString_internal const&, bool*)	C:/builds/moz2_slave/tb-c-aurora-w32-ntly-000000000/build/mailnews/base/search/src/nsMsgSearchTerm.cpp:1141
3 	xul.dll	nsMsgSearchTerm::MatchRfc2047String(nsACString_internal const&, char const*, bool, bool*)	C:/builds/moz2_slave/tb-c-aurora-w32-ntly-000000000/build/mailnews/base/search/src/nsMsgSearchTerm.cpp:1089
4 	xul.dll	nsMsgSearchTerm::MatchRfc822String(nsACString_internal const&, char const*, bool*)	C:/builds/moz2_slave/tb-c-aurora-w32-ntly-000000000/build/mailnews/base/search/src/nsMsgSearchTerm.cpp:1215
5 	xul.dll	nsMsgSearchOfflineMail::ProcessSearchTerm(nsIMsgDBHdr*, nsIMsgSearchTerm*, char const*, nsIMsgSearchScopeTerm*, nsIMsgDatabase*, char const*, unsigned int, bool, bool*)	C:/builds/moz2_slave/tb-c-aurora-w32-ntly-000000000/build/mailnews/base/search/src/nsMsgLocalSearch.cpp:434
6 	xul.dll	nsMsgSearchBoolExpression::OfflineEvaluate(nsIMsgDBHdr*, char const*, nsIMsgSearchScopeTerm*, nsIMsgDatabase*, char const*, unsigned int, bool)	C:/builds/moz2_slave/tb-c-aurora-w32-ntly-000000000/build/mailnews/base/search/src/nsMsgLocalSearch.cpp:140
7 	xul.dll	nsMsgSearchBoolExpression::OfflineEvaluate(nsIMsgDBHdr*, char const*, nsIMsgSearchScopeTerm*, nsIMsgDatabase*, char const*, unsigned int, bool)	C:/builds/moz2_slave/tb-c-aurora-w32-ntly-000000000/build/mailnews/base/search/src/nsMsgLocalSearch.cpp:151
8 	xul.dll	nsMsgSearchOfflineMail::MatchTerms(nsIMsgDBHdr*, nsIArray*, char const*, nsIMsgSearchScopeTerm*, nsIMsgDatabase*, char const*, unsigned int, bool, nsMsgSearchBoolExpression**, bool*)	C:/builds/moz2_slave/tb-c-aurora-w32-ntly-000000000/build/mailnews/base/search/src/nsMsgLocalSearch.cpp:684
9 	xul.dll	nsMsgSearchOfflineMail::MatchTermsForSearch(nsIMsgDBHdr*, nsIArray*, char const*, nsIMsgSearchScopeTerm*, nsIMsgDatabase*, nsMsgSearchBoolExpression**, bool*)	C:/builds/moz2_slave/tb-c-aurora-w32-ntly-000000000/build/mailnews/base/search/src/nsMsgLocalSearch.cpp:330
10 	xul.dll	nsMsgSearchOfflineMail::Search(bool*)	C:/builds/moz2_slave/tb-c-aurora-w32-ntly-000000000/build/mailnews/base/search/src/nsMsgLocalSearch.cpp:736
Keywords: crash
Comment on attachment 8863111 [details] [diff] [review]
1360803-fix-crash.patch

Review of attachment 8863111 [details] [diff] [review]:
-----------------------------------------------------------------

OK so you are just doing what I suggested years ago, but today I don't understand why I suggested that, and nsString should be initialized to blank when it is created.

Is there any reason for doing this other than the comment that I made? For the crash I would suspect it to be related to some variable going out of scope and getting garbage collected.
I did what your comment suggested, no other reason. It seemed strange to me to initialise one of those string auto-types since the CTOR should initialise them.

However, the whole thing looks fishy, what's the union doing:
https://dxr.mozilla.org/comm-central/rev/cd1e26a0cebec588bcd0076e5726b12f12b413a5/mailnews/base/search/public/nsMsgSearchCore.idl#188
(In reply to Jorg K (GMT+2) from comment #4)
> I did what your comment suggested, no other reason. It seemed strange to me
> to initialise one of those string auto-types since the CTOR should
> initialise them.

Yes I think so as well, so that means that I no longer agree with the approach I suggested. I doubt if that change would fix the crash.
Attachment #8863111 - Flags: review?(rkent)
Perhaps a slightly more common crash signature would help.
nsMsgSearchTerm::MatchRfc2047String bp-cbfd5dfb-e223-49c0-9b1b-6efd00170813
Crash Signature: [@ nsAString_internal::Assign | nsAString_internal::Assign | nsMsgSearchTerm::MatchString] → [@ nsAString_internal::Assign | nsAString_internal::Assign | nsMsgSearchTerm::MatchString] [@ nsMsgSearchTerm::MatchRfc2047String ]
Summary: Crash in nsAString_internal::Assign | nsAString_internal::Assign | nsMsgSearchTerm::MatchString → Crash in nsMsgSearchTerm::MatchString / nsMsgSearchTerm::MatchRfc2047String
I don't have a good idea here and the signature from comment #6 is from TB 45.8 :-(
Assignee: jorgk → nobody
Status: ASSIGNED → NEW
(feeling guilty) mkato, can you get past comment 5?


(In reply to Jorg K (GMT+1) from comment #7)
> I don't have a good idea here and the signature [nsMsgSearchTerm::MatchRfc2047String] from comment #6 is from TB
> 45.8 :-(

That was merely an example.  Examples from current versions:
bp-e9d03c4e-fd62-4b68-8829-86eb90180405 52.7.0 
bp-380c6204-cf00-48bc-a10a-88c4c0180307 60.0a1 (tktoshi)
bp-05806e6f-d784-431a-8828-cab2c0180403 52.6.0 (blaney)
bp-161650d0-9b26-4058-9413-91f650180328   "" ""
bp-2c78d6bf-70cc-4d57-a968-3e7260180222   "" ""


 nsAString_internal::Assign | nsAString_internal::Assign | nsMsgSearchTerm::MatchString  still occurs
bp-d99b63ec-f1d7-4d38-8419-f40140180310 (kevon) claims to be crashing 2-4 times per day on 52.6.0
Flags: needinfo?(m_kato)
> bp-380c6204-cf00-48bc-a10a-88c4c0180307

This is that mimeConverter is nullptr.  So we need add nullptr check for mimeConverter.
Flags: needinfo?(m_kato)
Summary: Crash in nsMsgSearchTerm::MatchString / nsMsgSearchTerm::MatchRfc2047String → Crash in nsMsgSearchTerm::MatchString / nsMsgSearchTerm::MatchRfc2047String, mimeConverter is null
(In reply to Makoto Kato [:m_kato] from comment #9)
> > bp-380c6204-cf00-48bc-a10a-88c4c0180307
> 
> This is that mimeConverter is nullptr.  So we need add nullptr check for
> mimeConverter.

bp-95c31446-b935-4c26-a85c-7533e0180328 "signed an enigmail key; then read an account folder"
Is there any solution in sight because the constant crashes when closing TB seems to corrupt the GLODA and e. g. QuickFilters does not work correctly except only after starting TB.
Duplicate of this bug: 1475885
(In reply to reiner.block from comment #11)
> Is there any solution in sight because the constant crashes when closing TB
> seems to corrupt the GLODA 
Like bp-05247732-6ff5-4d9b-8909-2b17c0180713?  What behavior do you see?

> QuickFilters does not work correctly except only after starting TB.
You mean Thunderbird quick filter?  Or do you mean an addon? (you have 80 add-ons!)
I see that my Bug Report 1475885 has been merged into this one due to the similarity of the crash signature.  Thank you for clarifying that an EXCEPTION_ACCESS_VIOLATION_READ means reading invalid memory location, and is not a
permissions issue. I wish I had a reproducible test case.  I only learn of the crash when I go to log off TB.  Each time, I try to comment what I was up to and give permission to be contacted.  Looking forward to developments and ready to provide any further information.
(In reply to wmblaney from comment #14)
> I wish I had a reproducible test case.  

The most common comments in your crash reports mention using or making filters, and search. You also say you have many filters.
So I suggest
1. shut down once or twice a day (to reduce the time period), and when it crashes try to identify what filter or search actions you took during that period
2. how many filters do you have?
3. how many pop accounts, and how imap accounts do you have?
Flags: needinfo?(wmblaney)
A third party observer's comment:

I have hundreds of filters to wade through many mailing list I subscribe, but come to think of it, several years ago, when TB began crashing too often at shutdown, I began NOT shutting down TB any more, but kept running it for maybe one month or two months before I am forced shutdown the linux image I run on my PC at the office. 
So some kind of crash at shutdown time WAS real back then. Not sure if it is today, though.
Ah right, during the recent tests of local TB build using mid-April source tree, I noticed a strange assertion error regarding certificate not verifiable or something to that effect when a cert is checked (I am not sure if it is a cert of PGP-signed e-mail or cert of a remote mail server?)
Hello Wayne, and thanks for chiming in.  
1. You suggest shutting down once or twice a day, to shorten the session time, and when it crashes to try and identify what filter or search actions I took during that period.  I have the TB Activity Manager to see what actions occurred.  During my session, I see in the lower left window little messages telling me TB is bringing folders up to date, copying messages from one folder to another, and generally following my filter instructions.  
2. You ask how many filters I have:  245.  Also I have 12 saved searches.  
3. You ask how many pop accounts and how imap accounts I have:  3 IMAP accounts, and no POP accounts.  

Since Oct'15 I see I had 5 Unsubmitted Crash Reports, and 50 Submitted Crash Reports.  They all show the same Crash Reason:  "EXCEPTION_ACCESS_VIOLATION_READ".

The signatures appear to be the same:  "nsMsgSearchTerm::MatchRfc2047String".  I see that is the focus of this Bug 1360803.  

I see a number of analytic tools, and each crash report has the following tabs: Details, Metadata, Modules, Raw Dump, Extensions, and (optional) Correlations.  I believe most of these details are also available to Developers.  

If I can provide any further information, please let me know, because I'm very interested in helping.  Thanks.
Flags: needinfo?(wmblaney)
Magnus Is comment 9 just wallpaper?  If yes can you suggest a next step?  And if not can you supply a patch?
Flags: needinfo?(mkmelin+mozilla)
Hard to tell if this will just move the error somewhere else, but it shouldn't hurt.

(Fixed one other similar problem in nsSmtpUrl.cpp while I was at it.
Assignee: nobody → mkmelin+mozilla
Attachment #8863111 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
Attachment #9011494 - Flags: review?(jorgk)
Comment on attachment 9011494 [details] [diff] [review]
bug1360803_mimeconverter_null.patch

I doubt that this will fix anything since do_GetService(NS_MIME_CONVERTER_CONTRACTID) shouldn't fail. No harm in added error checking.

I have a good feeling now of what *will* fix the crash, it's the silly copying in the CTOR. Patch coming.
Attachment #9011494 - Flags: review?(jorgk) → review+
I really don't know what damage
  mValue = *aInitialValue;
can do then you're copying a structure that contains a nsString.

Like this, the structure members are copied individually and the auto-initialised utf16String isn't clobbered by having something copied on top.
Attachment #9011513 - Flags: review?(mkmelin+mozilla)
Attachment #9011513 - Flags: feedback?(m_kato)
Attachment #9011513 - Flags: feedback?(benc)
Comment on attachment 9011513 [details] [diff] [review]
1360803-fix-nsMsgSearchValueImpl.patch

My reading is that `mValue = *aInitialValue;` would just cause the utf16String copy operator to be invoked, which just calls Assign().
Which should be fine, albeit redundant if IS_STRING_ATTRIBUTE is set.
I think your change makes it a little clearer.

That said, I'd have thought that if aInitialValue is valid when it's passed in, then its `utf16String` member should already be set up to be a utf16 version of `string` anyway, so the whole if/else part shouldn't be needed... (but I've not looked into the wider context - that just a naive first impression reading of the code)
Attachment #9011513 - Flags: feedback?(benc) → feedback+
(In reply to Ben Campbell from comment #22)
> My reading is that `mValue = *aInitialValue;` would just cause the utf16String
> copy operator to be invoked, which just calls Assign().
I have no idea how the initial value is generated, since it's a structure, and whether utf16String is even properly initialised. Maybe someone just allocated memory for it.

> That said, I'd have thought that if aInitialValue is valid when it's passed
> in, then its `utf16String` member should already be set up to be a utf16
> version of `string` anyway, so the whole if/else part shouldn't be needed...
Well, as least the strdup() is necessary, since this new structure wants to own its own string.
No effective change to the previous patch, only renaming the union since 'u' isn't a great name. It's a matter of taste.
Attachment #9011724 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9011724 [details] [diff] [review]
1360803-fix-nsMsgSearchValueImpl.patch - with renaming the 'u' union

Review of attachment 9011724 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/search/src/nsMsgSearchValue.cpp
@@ +21,5 @@
> +  }
> +  else
> +  {
> +    mValue.string = 0;
> +    // mValue.utf16String is an auto-string that needs no further intialization.

the whole duplication of having a string an also a utf16string member stored on the struct looks very goofy to me. couldn't we remove one of them and not use char* for the one member?
Attachment #9011724 - Flags: feedback?(mkmelin+mozilla) → feedback+
Comment on attachment 9011513 [details] [diff] [review]
1360803-fix-nsMsgSearchValueImpl.patch

Review of attachment 9011513 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/search/src/nsMsgSearchValue.cpp
@@ +10,5 @@
>  #include "nsString.h"
>  
>  nsMsgSearchValueImpl::nsMsgSearchValueImpl(nsMsgSearchValue *aInitialValue)
>  {
> +  // Copy the structrure members one by one.

typo

seems a little against the intent though, that if, say, you change the implementation you'd have to go around adding/removing/modifying individual members
So where does no review on one patch and f+ for the renaming of the union leave me? :-( - We really came here for a crash fix instead of a complete refactoring, but here goes ...

Here's a version that removes the char* member and switches processing to the nsString. That comes at a cost since you need to convert to nsCString/UTF-8 on various occasions.

It doesn't compile yet, I still need to replace to .string call sites in nsMsgSearchTerm.cpp. I just wanted to show you want's involved.

We could go the other way and switch everything to nsCString. However, the match code nsMsgSearchTerm::MatchString() works on the UFT-16 strings, so converting a possible nsCString member up there (in a tight inner loop) would be an even bigger performance hit.
Attachment #9011770 - Flags: feedback?(mkmelin+mozilla)
Attachment #9011770 - Flags: feedback?(acelists)
Actually, I removed renaming of the union again, so you can see the string changes better. I can always do that in a second patch.
Attachment #9011770 - Attachment is obsolete: true
Attachment #9011770 - Flags: feedback?(mkmelin+mozilla)
Attachment #9011770 - Flags: feedback?(acelists)
Attachment #9011772 - Flags: feedback?(mkmelin+mozilla)
Attachment #9011772 - Flags: feedback?(acelists)
Comment on attachment 9011772 [details] [diff] [review]
1360803-fix-nsMsgSearchValueImpl.patch - remove char* member - WIP (doesn't compile fully yet)

Review of attachment 9011772 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/search/public/nsMsgSearchCore.idl
@@ +167,5 @@
>  [ptr] native nsMsgSearchValue(nsMsgSearchValue);
>  
>  %{C++
>  #include "nsString.h"
> +// If you changes this structure, make sure you update

"change" ?

@@ -189,1 @@
>      nsString utf16String;

Yes I like this. Now the variable could also be renamed to something more descriptive, like you do with the 'u' member.
Attachment #9011772 - Flags: feedback?(acelists) → feedback+
Comment on attachment 9011724 [details] [diff] [review]
1360803-fix-nsMsgSearchValueImpl.patch - with renaming the 'u' union

Review of attachment 9011724 [details] [diff] [review]:
-----------------------------------------------------------------

I like the renaming of the union to something more descriptive.
Attachment #9011724 - Flags: feedback+
I think keeping both strings to avoid conversion is best.
Attachment #9011513 - Attachment is obsolete: true
Attachment #9011724 - Attachment is obsolete: true
Attachment #9011772 - Attachment is obsolete: true
Attachment #9011513 - Flags: review?(mkmelin+mozilla)
Attachment #9011513 - Flags: feedback?(m_kato)
Attachment #9011772 - Flags: feedback?(mkmelin+mozilla)
Attachment #9011788 - Flags: review?(mkmelin+mozilla)
Oops, missed one.
Attachment #9011788 - Attachment is obsolete: true
Attachment #9011788 - Flags: review?(mkmelin+mozilla)
Attachment #9011790 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9011790 [details] [diff] [review]
1360803-fix-nsMsgSearchValueImpl.patch (v5b)

I missed the earlier feedback. Renaming of the union can happen in a separate patch so it doesn't obscure the string changes. I noted the typo "changes".
Attachment #9011790 - Flags: feedback?(acelists)
Please note: The current suggestion is to change the char* to an nsCString but leave two versions of the string since you can see that there are a few conversions otherwise, see attachment 9011772 [details] [diff] [review]. That patch wasn't finished, so even more conversions would have been necessary to complete it. That's a performance hit which we can't allow to happen in search.
Same patch, I've only changed two comments and the commit message.

I've checked the allocation of nsMsgSearchValue and nsMsgSearchValueImpl. They are done via 'new' so the CTORs.

In nsMsgSearchValueImpl::nsMsgSearchValueImpl() we would most likely get away with replacing the entire body with
  mValue = *aInitialValue;
assuming that all the C++ bits will do the right thing.
Attachment #9011790 - Attachment is obsolete: true
Attachment #9011790 - Flags: review?(mkmelin+mozilla)
Attachment #9011790 - Flags: feedback?(acelists)
Attachment #9011882 - Flags: review?(mkmelin+mozilla)
Attachment #9011882 - Flags: feedback?(acelists)
Let's complete the sentence:
They are done via 'new' so the CTORs ... will initialise the strings.
(In reply to Jorg K (GMT+2) from comment #34)
> Please note: The current suggestion is to change the char* to an nsCString
> but leave two versions of the string since you can see that there are a few
> conversions otherwise, see attachment 9011772 [details] [diff] [review].
> That patch wasn't finished, so even more conversions would have been
> necessary to complete it. That's a performance hit which we can't allow to
> happen in search.

Looks to me that with the exception of matching a custom header, it's used only during the setup phase (i.e. once per complete search session), which is not a measurable performance hit.
(In reply to Magnus Melin [:mkmelin] from comment #37)
> (In reply to Jorg K (GMT+2) from comment #34)
> > Please note: The current suggestion is to change the char* to an nsCString
> > but leave two versions of the string since you can see that there are a few
> > conversions otherwise, see attachment 9011772 [details] [diff] [review].
> > That patch wasn't finished, so even more conversions would have been
> > necessary to complete it. That's a performance hit which we can't allow to
> > happen in search.
> 
> Looks to me that with the exception of matching a custom header, 

But for users with several custom headers?
OK, here is the "one string" solution which fully compiles now.

There are nine conversions, these for *NOT* in the initialisation:
- nsMsgSearchTerm::MatchBody() (OK, obscure case, but anyway)
- nsMsgSearchTerm::MatchInAddressBook()
- nsMsgSearchTerm::MatchKeyword()
- nsMsgSearchTerm::MatchCustom()

DON'T DO IT!

What are you gaining? You save a few bytes of memory?
Attachment #9011941 - Flags: feedback-
Sorry: There are nine conversions NS_ConvertUTF16toUTF8(), these four *NOT* in the initialisation ...
Same one string solution with added prints so you can see when the conversions happens. But we knew that since comment #39.
referring to attachment 9011941 [details] [diff] [review] (and attachment 9011956 [details] [diff] [review]):

> nsresult nsMsgSearchTerm::OutputValue(nsCString &outputStr)
> {
>-  if (IS_STRING_ATTRIBUTE(m_attribute) && m_value.string)
>+  if (IS_STRING_ATTRIBUTE(m_attribute) && !m_value.utf16String.IsEmpty())
>   {

Is the IsEmpty() needed? If the term is a string you'd always want it returned in outputStr, even if empty, right?
(of course, you'd never be searching for an empty string anyway, so it's a bit mot anyway, but hey ;-)
OK, here's the version with |mValue = *aInitialValue;|. Interdiff with (v5c).
Attachment #9011967 - Flags: review?(mkmelin+mozilla)
Attachment #9011967 - Flags: feedback?(benc)
Comment on attachment 9011967 [details] [diff] [review]
1360803-fix-nsMsgSearchValueImpl.patch (v6)

looks good to me - I think replacing the raw char* is the Right Thing (tm).
Attachment #9011967 - Flags: feedback?(benc) → feedback+
Comment on attachment 9011967 [details] [diff] [review]
1360803-fix-nsMsgSearchValueImpl.patch (v6)

Review of attachment 9011967 [details] [diff] [review]:
-----------------------------------------------------------------

Great, r=mkmelin

FWIW, I was never out to save a few bytes. But there is easily the case that they would get out of sync. Saving the same thing two times is fundamentally wrong, but I guess we have a reason for it here.
Attachment #9011967 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9011882 - Attachment is obsolete: true
Attachment #9011882 - Flags: review?(mkmelin+mozilla)
Attachment #9011882 - Flags: feedback?(acelists)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/34eaca00ed36
Add error checking to getting nsIMimeConverter service to avoid crash. r=jorgk
https://hg.mozilla.org/comm-central/rev/9cbdc6469caf
switch nsMsgSearchValue to using nsCString and fix CTOR of nsMsgSearchValueImpl. r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
Note, beta has only ~3 per month so we likely won't know how effective this is until it hits esr 60.4.0 - which is just fine
Attachment #9011967 - Flags: approval-comm-esr60?
Attachment #9011967 - Flags: approval-comm-beta+
Personally, I would prefer this one ride the train and not be uplifted
There seems to be a contradiction.

(In reply to Wayne Mery (:wsmwk) from comment #47)
> ... until it hits esr 60.4.0 - which is just fine
                    ^^^^^^^^^^
(In reply to Wayne Mery (:wsmwk) from comment #48)
> Personally, I would prefer this one ride the train and not be uplifted

How do you expect it to get into esr 60.4.0 if not via uplift? The train doesn't go there. Whether it goes into 63 beta or 64 is really irrelevant. If you prefer, I can wait for 60.4, as I already indicated by the ? since everything I plan for 60.3 already has the +.
I mean no uplift to beta 63.
Comment on attachment 9011967 [details] [diff] [review]
1360803-fix-nsMsgSearchValueImpl.patch (v6)

As you please.
Attachment #9011967 - Flags: approval-comm-beta+
Comment on attachment 9011967 [details] [diff] [review]
1360803-fix-nsMsgSearchValueImpl.patch (v6)

Good for 60.4.
Attachment #9011967 - Flags: approval-comm-esr60? → approval-comm-esr60+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/dfeb481219e9
Follow-up: Align trunk with ESR code by replacing MakeSpan(). r=me
You need to log in before you can comment on or make changes to this bug.