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

VERIFIED FIXED in 1.7.7

Status

--
major
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: stephend, Assigned: brandon)

Tracking

Trunk
1.7.7
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fuzzer], URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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.
Flags: in-testsuite?
Flags: in-litmus?
(Reporter)

Comment 1

8 years ago
[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

Updated

8 years ago
Assignee: nobody → laura
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.

Updated

8 years ago
Assignee: rhelmer → bsavage
(Assignee)

Comment 4

8 years ago
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?
Attachment #525503 - Flags: feedback?(rhelmer)
(Assignee)

Updated

8 years ago
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" ^)
(Assignee)

Comment 6

8 years ago
Created attachment 525510 [details] [diff] [review]
Corrected patch

Corrects a misspelled variable.
Attachment #525503 - Attachment is obsolete: true
Attachment #525503 - Flags: review?(laura)
Attachment #525503 - Flags: feedback?(rhelmer)
Attachment #525510 - Flags: review?(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.
Attachment #525510 - Flags: review?(rhelmer) → review+
(Assignee)

Comment 8

8 years ago
This issue was fixed in revision 3045.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Reporter)

Comment 9

8 years ago
(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 → ---
(Assignee)

Comment 11

8 years ago
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?
(Assignee)

Comment 13

8 years ago
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.
(Assignee)

Comment 14

8 years ago
Created attachment 525772 [details] [diff] [review]
New patch to fix the additional URLs missed in the first one
Attachment #525772 - Flags: review?(rhelmer)
Attachment #525772 - Flags: feedback?(laura)
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+
(Assignee)

Comment 16

8 years ago
This was resolved in revision 3049 and includes the additional URLs in comment 1.
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
(Reporter)

Comment 17

8 years ago
Verified FIXED using the URLs in comment 0 and comment 1.  I spun off bug 649793 for comment 13.
Status: RESOLVED → VERIFIED

Updated

8 years ago
Attachment #525772 - Flags: feedback?(laura)
Component: Socorro → General
Product: Webtools → Socorro
(Reporter)

Updated

7 years ago
Component: General → Webapp
(Reporter)

Updated

7 years ago
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.