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

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect)

Description

[Bob Clary [:bc] (inactive)]

click search folder and crash. 3.0b4pre/mac

http://crash-stats.mozilla.com/report/list?product=Thunderbird&version=Thunderbird%3A3.0b4pre&platform=mac&query_search=signature&query_type=exact&query=&date=&range_value=1&range_unit=weeks&do_query=1&signature=nsMsgSearchValidityTable%3A%3AGetAvailable%28int%2C%20int%2C%20int*%29

regressed since 20090905 nightly.

Comment 1

[Kent James (:rkent)]

See the patch on bug 511131 that is supposed to fix this.

Comment 2

[David :Bienvenu]

fix checked in, as Kent said, see the patch in bug 511131

Comment 3

[Wayne Mery (:wsmwk)]

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

Comment 4

[Bob Clary [:bc] (inactive)]

fwiw, I haven't seen this crash in a while.

Comment 5

[Pablo Greco]

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)

Comment 6

[Pablo Greco]

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

Comment 7

[Wayne Mery (:wsmwk)]

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

Comment 8

[David :Bienvenu]

Is anyone who's seeing this running with extensions?

Comment 9

[Pablo Greco]

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

Comment 10

[Mark Banner (:standard8)]

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.

Comment 11

[Kent James (:rkent)]

See http://getsatisfaction.com/mozilla_messaging/topics/saving_message_filter_fails_then_tb3_crashes for a user thread reporting this crash.

Comment 12

[Kent James (:rkent)]

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.

Comment 13

[Kent James (:rkent)]

Attachment: This test crashes

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.

Comment 14

[Kent James (:rkent)]

Attachment: add checks for proper range

Here's a patch the checks the ranges.

Comment 15

[Kent James (:rkent)]

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.

Comment 16

[Kent James (:rkent)]

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 17

[Kent James (:rkent)]

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.

Comment 18

[Kent James (:rkent)]

Attachment: Don't check getAvailable for matchAll terms

Comment 19

[David :Bienvenu]

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.

Comment 20

[Kent James (:rkent)]

"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.

Comment 21

[David :Bienvenu]

Yes, I'm definitely not asking for a new patch...

Comment 22

[Mark Banner (:standard8)]

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.

Comment 23

[Kent James (:rkent)]

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

Comment 24

[Kent James (:rkent)]

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

Comment 25

[Wayne Mery (:wsmwk)]

v.fixed. no crashes after v3.0.1