641902 - /topcrasher, /report, and /correlation are all returning 500 Internal Server Errors

Status: VERIFIED FIXED

(Socorro :: Webapp, task)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

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.

Comment 1

[Stephen Donner [:stephend] Not actively reading bugmail]

[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

Comment 2

[Robert Helmer [:rhelmer]]

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

Comment 3

[Robert Helmer [:rhelmer]]

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

Comment 4

[Brandon Savage [:brandon]]

Attachment: 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.

Comment 5

[Robert Helmer [:rhelmer]]

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" ^)

Comment 6

[Brandon Savage [:brandon]]

Attachment: Corrected patch

Corrects a misspelled variable.

Comment 7

[Robert Helmer [:rhelmer]]

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.

Comment 8

[Brandon Savage [:brandon]]

This issue was fixed in revision 3045.

Comment 9

[Stephen Donner [:stephend] Not actively reading bugmail]

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

Comment 10

[Robert Helmer [:rhelmer]]

(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?

Comment 11

[Brandon Savage [:brandon]]

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.

Comment 12

[Laura Thomson :laura]

Can you take a look at whether this Kohana behavior has improved in newer versions than the one we run?

Comment 13

[Brandon Savage [:brandon]]

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.

Comment 14

[Brandon Savage [:brandon]]

Attachment: New patch to fix the additional URLs missed in the first one

Comment 15

[Robert Helmer [:rhelmer]]

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.

Comment 16

[Brandon Savage [:brandon]]

This was resolved in revision 3049 and includes the additional URLs in comment 1.

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using the URLs in comment 0 and comment 1.  I spun off bug 649793 for comment 13.