Closed Bug 514945 Opened 11 years ago Closed 11 years ago

crash [@ nsMsgSearchValidityTable::GetAvailable(int, int, int*)] in matchAll filters using saved search or updating filters

Categories

(MailNews Core :: Filters, defect)

defect
Not set
critical

Tracking

(thunderbird3.1 beta1-fixed, thunderbird3.0 .2-fixed)

VERIFIED FIXED
Thunderbird 3.1b1
Tracking Status
thunderbird3.1 --- beta1-fixed
thunderbird3.0 --- .2-fixed

People

(Reporter: bc, Assigned: rkent)

References

()

Details

(Keywords: crash, fixed-seamonkey2.0.4, regression)

Crash Data

Attachments

(1 file, 2 obsolete files)

See the patch on bug 511131 that is supposed to fix this.
fix checked in, as Kent said, see the patch in bug 511131
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
crash sig still exists in 3.0b4, and 3.0pre so bug 511131 didn't get all (or any??) of this crasher.

bp-eb3d0edc-5872-4565-8a09-ff6cb2090905 3.0b4pre Mac 20090905045154 (as reported in comment 0)
0  	thunderbird-bin  	nsMsgSearchValidityTable::GetAvailable  	 apter.h:173
1 	libxpcom_core.dylib 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:179
2 	thunderbird-bin 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2454
3 	thunderbird-bin 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1590
4 	libmozjs.dylib 	js_Invoke 	js/src/jsinterp.cpp:1386
5 	libmozjs.dylib 	js_Interpret 	js/src/jsinterp.cpp:5179


bp-c0b1734c-902d-44e4-b9af-030ea2091009 3.0pre windows 20091009032155
0	thunderbird.exe	nsMsgSearchValidityTable::GetAvailable	 objdir-tb/mozilla/dist/include/msgbase/nsMsgSearchAdapter.h:173
1	xpcom_core.dll	NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
2	thunderbird.exe	XPCWrappedNative::CallMethod	js/src/xpconnect/src/xpcwrappednative.cpp:2297
3	thunderbird.exe	XPC_WN_CallMethod	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1590
4	js3250.dll	js_Invoke	js/src/jsinterp.cpp:1386
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
fwiw, I haven't seen this crash in a while.
I've got this today on my wife's computer
bp-bc09bb61-04d5-4e8a-b0dc-7cde42091118 
She was editing a filter using Mozilla/5.0 (Windows; U; Windows NT 5.1; es-AR; rv:1.9.1.5) Gecko/20091117 Thunderbird/3.0 (tb3rc1-build2)
I'm nominating for blocking TB3, just to keep it seen.
If I'm correct, this number 44 topcrash in beta4 and number 7 in RC builds
Flags: blocking-thunderbird3?
hmm, this probably should still be sev=blocker. (arguing with myself, maybe I shouldn't have even reopened?)

and unlikely to be worthy of blocking because it's not a true topcrash:
* for point releases we typically nominate blocking for crashes from the top 10-20
* there's not a critical mass of usage or cross section of users doing RC1+related nightlies to believe those rankings. Until there is critical mass on board for a period of 5-7 days, the rankings aren't much more than hints of what might emerge.  FWIW, nightly stats (3.01pre) would be slightly more believable than RC stats. however, we normally find that stats of the point releases don't match up well with nightlies. For example, nsMsgSearchValidityTable::GetAvailable isn't in the top 100 for 3.0pre
Is anyone who's seeing this running with extensions?
In my wife's case, three extensions
Minimize to tray plus 1.0.6
Allow Empty subject (changed by me to be compatible with 3.0, not just bumped)
Compact headers 0.99.6
Not going to block on this, as it doesn't seem to be a top crasher.

If steps to reproduce are found or this becomes a confirmed top crasher please feel free to re-nominate.
Flags: blocking-thunderbird3? → blocking-thunderbird3-
The description of the current crash is frequently associated with filter editing. The GetAvailable function has no protection around its calls, it just blindly accesses an array. That means that js callers can easily crash us by simply giving invalid input data. And there seems to be unexplained cases out there of issues with filter editing or filter files that give unpredictable results.

I think that we need to at least protect the calls to GetAvailable so that invalid parameters do not cause a crash.
Attached file This test crashes (obsolete) —
This test will crash on my system. Not that anyone should be calling GetAvailable in this way, but still js calls should not crash this easily.
Assignee: nobody → kent
Status: REOPENED → ASSIGNED
Attachment #423820 - Attachment mime type: text/x-c → text/plain
Attached patch add checks for proper range (obsolete) — Splinter Review
Here's a patch the checks the ranges.
Attachment #423820 - Attachment is obsolete: true
Attachment #423832 - Flags: superreview?(bienvenu)
Attachment #423832 - Flags: review?(bienvenu)
Attachment #423832 - Flags: approval-thunderbird3.0.2?
I can generate this crash in some cases with filters that seem in other cases to be bug 517040, so these are definitely related. Applying the patch to this bug turns a crash into a filter edit dialog where "OK" appears to do nothing, but errors appear in the error console.

So this bug will stop the crash, and I will deal with the root cause in bug 517040.
So after a day playing with bug 517040, along with this patch applied, I see some issues to consider.

In some cases we were checking for validity of search terms when we didn't need to, for example when the term was a matchAll term. Without this patch, and with getAvailable returning a random but non-crashing value, this is interpreted usually as TRUE and the garbage getAvailable is ignored, life goes on, and everyone is happy. But once we add these checks, then in some cases, like the test filter I was working with in bug 517040, we get the new error from this patch, and so (in that one case) the filter save fails. The OK button just sits there and the filter editor dialog does not go away. I will fix that in bug 517040, but it brings up the question for this bug, would it be better to return safe values (like true for getAvailable) when we give garbage input, rather than generating an XPCOM failure as the current patch does? I need to think about that some.
Comment on attachment 423832 [details] [diff] [review]
add checks for proper range

So I think what I will do is to fold the patch for bug 517040 into this one, because if the existing patch is applied before the issues in bug 517040 are fixed, then in some cases the program will fail to save edited filters that were successfully saved without this patch. So they need to be fixed together.
Attachment #423832 - Flags: superreview?(bienvenu)
Attachment #423832 - Flags: review?(bienvenu)
Attachment #423832 - Flags: approval-thunderbird3.0.2?
Attachment #423832 - Attachment is obsolete: true
Attachment #424271 - Flags: superreview?(bienvenu)
Attachment #424271 - Flags: review?(bienvenu)
Attachment #424271 - Flags: approval-thunderbird3.0.2?
Severity: blocker → critical
Component: Folder and Message Lists → Filters
OS: Mac OS X → All
Product: Thunderbird → MailNews Core
QA Contact: folders-message-lists → filters
Hardware: x86 → All
Version: 3.0 → Trunk
Summary: crash [@ nsMsgSearchValidityTable::GetAvailable(int, int, int*)] clicking search folder → crash [@ nsMsgSearchValidityTable::GetAvailable(int, int, int*)] in matchAll filters
Summary: crash [@ nsMsgSearchValidityTable::GetAvailable(int, int, int*)] in matchAll filters → crash [@ nsMsgSearchValidityTable::GetAvailable(int, int, int*)] in matchAll filters using saved search or updating filters
Attachment #424271 - Flags: superreview?(bienvenu)
Attachment #424271 - Flags: superreview+
Attachment #424271 - Flags: review?(bienvenu)
Attachment #424271 - Flags: review+
Comment on attachment 424271 [details] [diff] [review]
Don't check getAvailable for matchAll terms

thx for the patch. nsMsgSearchOp.Contains is a somewhat arbitrary choice (though a reasonable one). An alternative would be to treat using a search term with an undefined op as an error, and have a special undefined value.
Attachment #424271 - Flags: approval-thunderbird3.0.2? → approval-thunderbird3.0.2+
"nsMsgSearchOp.Contains" may be arbitrary, but choosing it as the default was made long ago, as the default search term that shows up now in the filter editor is "Subject" "Contains". So this is consistent with current usage - but in theory the value that I set is not actually used anywhere, this was just set for safety.

Though I could see the possible value of your suggestion to use a default value, I don't think this is terribly important, nor worth respinning the patch and reviews.
Yes, I'm definitely not asking for a new patch...
Comment on attachment 424271 [details] [diff] [review]
Don't check getAvailable for matchAll terms

So somehow, I missed the fact that this hasn't been checked in on trunk. As we've still got a few days to go before we get to 3.0.2, retracting my a+ for now.
Attachment #424271 - Flags: approval-thunderbird3.0.2+ → approval-thunderbird3.0.2?
Comment on attachment 424271 [details] [diff] [review]
Don't check getAvailable for matchAll terms

Checked in as http://hg.mozilla.org/comm-central/rev/0c27228b8400
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Attachment #424271 - Flags: approval-thunderbird3.0.2? → approval-thunderbird3.0.2+
Target Milestone: --- → Thunderbird 3.1b1
Comment on attachment 424271 [details] [diff] [review]
Don't check getAvailable for matchAll terms

Checked in http://hg.mozilla.org/releases/comm-1.9.1/rev/83b4effeb792
v.fixed. no crashes after v3.0.1
Status: RESOLVED → VERIFIED
Crash Signature: [@ nsMsgSearchValidityTable::GetAvailable(int, int, int*)]
You need to log in before you can comment on or make changes to this bug.