The following are all returning 500 Internal Server Errors: https://crash-stats.stage.mozilla.com/topcrasher https://crash-stats.stage.mozilla.com/topcrasher/ https://crash-stats.stage.mozilla.com/report/ https://crash-stats.stage.mozilla.com/correlation/bulk_ajax/cpu/Firefox/ https://crash-stats.stage.mozilla.com/correlation/bulk_ajax/cpu/ https://crash-stats.stage.mozilla.com/correlation/bulk_ajax/addon/ Expected: I'm not sure any of these are valid, but we should at least be returning a 404, if they're not including the right parameters.
[12:50:08.840] GET https://crash-stats.stage.mozilla.com/correlation/ajax/cpu/Firefox/3.6.13/Windows%20NT// [HTTP/1.1 500 Internal Server Error 139ms] [12:50:08.882] GET https://crash-stats.stage.mozilla.com/correlation/ajax/addon/Firefox/3.6.13/Windows%20NT// [HTTP/1.1 500 Internal Server Error 181ms] [12:50:08.904] GET https://crash-stats.stage.mozilla.com/correlation/ajax/module/Firefox/3.6.13/Windows%20NT// [HTTP/1.1 500 Internal Server Error 196ms] So, even with valid parameters, these are failing; got the above from loading https://crash-stats.stage.mozilla.com/report/index/a27904d2-34aa-4634-aa6e-e16882110314
I don't think comment 0 is a regression, I see the same on production. Correlations seem to be working for me on stage e.g. https://crash-stats.stage.mozilla.com/report/index/5835fcd9-f125-4cfa-b484-4e1672110323
Assignee: laura → rhelmer
Status: NEW → ASSIGNED
(In reply to comment #0) > The following are all returning 500 Internal Server Errors: > > https://crash-stats.stage.mozilla.com/topcrasher > https://crash-stats.stage.mozilla.com/topcrasher/ > https://crash-stats.stage.mozilla.com/report/ > https://crash-stats.stage.mozilla.com/correlation/bulk_ajax/cpu/Firefox/ > https://crash-stats.stage.mozilla.com/correlation/bulk_ajax/cpu/ > https://crash-stats.stage.mozilla.com/correlation/bulk_ajax/addon/ > > Expected: > > I'm not sure any of these are valid, but we should at least be returning a 404, > if they're not including the right parameters. The errors we log for these all do sound like expected parameters are missing: 2011-03-25 14:16:41 -07:00 --- error: [5xx Error] File: application/controllers/ topcrasher.php; Line: 150; Message: Undefined variable: product 2011-03-25 14:17:23 -07:00 --- error: [5xx Error] File: application/controllers/report.php; Line: 369; Message: Missing argument 1 for Report_Controller::index() 2011-03-25 14:17:28 -07:00 --- error: [5xx Error] File: application/controllers/correlation.php; Line: 111; Message: Missing argument 3 for Correlation_Controller::bulk_ajax() Many of these require an OOID so catching these and exiting earlier with a nicer message is probably warranted. Really I think only the /topcrashers case should have reasonable defaults, it looks like it tries right now, not for product but for version: https://crash-stats.stage.mozilla.com/topcrasher/byversion/Firefox That will redirect you to ".../3.6.3", which is an odd choice :) Offhand I'd say returning the top of the "current versions" list would be a good default, the rest of these should check for arguments and exit much earlier.
Created attachment 525503 [details] [diff] [review] First patch for this bug This patch takes two approaches. For topcrashes, the error was that an uninitialized variable was being passed. Given the default nature of the topcrashes index page, the product is simply pulled from the defaults. For the other two, sane defaults of "null" are passed to the action methods to ensure that further checking would create proper errors. In the case of the reports index, this is a 404 (not found) and in the case of the AJAX, this is a "ERROR: Expected POST with a list of signatures" error.
Attachment #525503 - Flags: review? → review?(laura)
Comment on attachment 525503 [details] [diff] [review] First patch for this bug Looks good in principle; small typo: >Index: correlation.php >=================================================================== >--- correlation.php (revision 3044) >+++ correlation.php (working copy) >@@ -108,7 +108,7 @@ > * @param string $product A Product name > * @param string $version A Product Version > */ >- public function bulk_ajax($type, $product, $version) >+ public function bulk_ajax($type = null, $product = null, $versioin = null) (Spelling of "version" ^)
Created attachment 525510 [details] [diff] [review] Corrected patch Corrects a misspelled variable.
Comment on attachment 525510 [details] [diff] [review] Corrected patch lgtm on my dev instance. /topcrasher/ is a little ugly, but not your fault at all :) better than throwing 500.
Attachment #525510 - Flags: review?(rhelmer) → review+
This issue was fixed in revision 3045.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(In reply to comment #8) > This issue was fixed in revision 3045. Are we still on manual updates? Don't see this fixed -- suspecting that's the issue.
(In reply to comment #9) > (In reply to comment #8) > > This issue was fixed in revision 3045. > > Are we still on manual updates? Don't see this fixed -- suspecting that's the > issue. Stage was updated; I think we focused on the URLs in comment 0 (none of which return HTTP 500) but haven't covered the URLs in comment 1. Brandon, can you take a look at those?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sure can. This is similar behavior where an incorrect number of parameters is passed, in this case no signature. This issue is more systemic than simply these issues; the others simply haven't been found. When an incorrect number of parameters is passed through the query string or URL, Kohana issues a 500 instead of a 404, and doesn't allow us to intelligently evaluate the request. My guess is that a large number of URLs within the system fail in this way. Should this bug be re-purposed as an audit/fix of these issues? Note that I'm wary of assigning defaults to circumvent the standard Kohana behavior in all cases; we need a better strategy for dealing with these issues, or at least have exhausted our other options first.
Can you take a look at whether this Kohana behavior has improved in newer versions than the one we run?
To make sure this gets into 1.7.7 we will fix the underlying cause of this issue, which is the broken URLs in Comment 0 and Comment 1. We will investigate the default behavior as a part of Bug #556828.
Created attachment 525772 [details] [diff] [review] New patch to fix the additional URLs missed in the first one
Comment on attachment 525772 [details] [diff] [review] New patch to fix the additional URLs missed in the first one With comment 13 in mind, looks fine to me.
Attachment #525772 - Flags: review?(rhelmer) → review+
This was resolved in revision 3049 and includes the additional URLs in comment 1.
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago → 8 years ago
Resolution: --- → FIXED
Verified FIXED using the URLs in comment 0 and comment 1. I spun off bug 649793 for comment 13.
Status: RESOLVED → VERIFIED
Component: Socorro → General
Product: Webtools → Socorro
You need to log in before you can comment on or make changes to this bug.