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.
See the patch on bug 511131 that is supposed to fix this.
fix checked in, as Kent said, see the patch in bug 511131
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
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:126.96.36.199) 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
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.
See http://getsatisfaction.com/mozilla_messaging/topics/saving_message_filter_fails_then_tb3_crashes for a user thread reporting this crash.
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.
Created attachment 423820 [details] 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.
Created attachment 423832 [details] [diff] [review] add checks for proper range Here's a patch the checks the ranges.
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.
Created attachment 424271 [details] [diff] [review] Don't check getAvailable for matchAll terms
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.
"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.
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 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