Closed Bug 711767 Opened 10 years ago Closed 10 years ago

Unable to display top crashers for Fennec 11.0a1

Categories

(Socorro :: Webapp, task)

Product:
Socorro
Component:
Webapp
Type:
task
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: scoobidiver, Assigned: rhelmer)

References

(
URL
)

Details

Attachments

(1 file)

example of broken query
12.68 KB, text/plain
Details 
This issue happened this morning.
Setting to blocker as it blocks me from doing my job in crashkill.
Severity: normal → blocker
Target Milestone: --- → 2.3.5
(In reply to Naoki Hirata :nhirata from comment #2)
> Setting to blocker as it blocks me from doing my job in crashkill.
I don't think so as you can use the advanced search:
https://crash-stats.mozilla.com/query/query?product=Fennec&version=Fennec%3A11.0a1&range_value=1&range_unit=weeks&query_search=signature&query_type=contains&query=&reason=&build_id=&process_type=any&hang_type=any&do_query=1
Assignee: nobody → rhelmer
Hmm looks like the error is:

2011-12-17 14:03:01 -08:00 --- error: [5xx Error] File: application/libraries/drivers/Database/Pgsql.php; Line: 74; Message: pg_query(): Query failed: ERROR:  syntax error at or near "Fennec"

Trying to repro that locally
Status: NEW → ASSIGNED
* works on crash-stats-dev (2.4pre)
* works on stage (2.3.5pre)
* works locally with release version against stageDB (2.3.4.1)
* does not work locally with release version against prodDB

This is triggered by something that's unique to prodDB, I am going to start debugging now that I can repro locally.
I can tell just from the error message that the problem is improper escaping of the SQL string. Just confirmed that and located exactly where it's coming from:

https://github.com/mozilla/socorro/blob/v2.3.4.1/webapp-php/application/models/topcrashers.php#L136

This ultimately uses pg_escape_string() instead of parameterized queries. We really need to switch this to parameterized queries if we want this to work (the version of Kohana we're on doesn't support it, unfortunately).

I am going to see if I can hack pg_query_params() support in, but we're going to be playing whack-a-mole trying to escape strings like this.
I just spoke to Laura and we would like to wait until Monday to push a fix for this, since it could potentially regress other things and we're concerned about getting enough QA on it.

Please let us know if you need this pushed to production sooner, otherwise I'll work on getting the appropriate data to reproduce the problem and a proposed fix into our dev environment for now.
Severity: blocker → critical
Lets move this to a middleware service and let psycopg2 handle the escaping. Its scheduled to be moved anyway.
(In reply to Chris Lonnen :lonnen from comment #8)
> Lets move this to a middleware service and let psycopg2 handle the escaping.
> Its scheduled to be moved anyway.

Yes, this is the right fix. We might be able to use one of the existing services, in fact (need to check).
Yes, the correct fix is to move it to the middleware.  As Rob and I discussed I'd prefer not to push on this (or something more hackish) over the weekend due to lack of resources for dev, review, IT and QA, and the high probability of regressions.  Naoki: I think you should be able to work around this using the advanced search until then.  (If you have a problem there please let us know.)
Yes, that's true.  lonnen, rhelmer and I discussed this on irc.  I'll use the workaround scoobi gave me in order to do my work and it can wait until next week.
(In reply to Robert Helmer [:rhelmer] from comment #9)
> (In reply to Chris Lonnen :lonnen from comment #8)
> > Lets move this to a middleware service and let psycopg2 handle the escaping.
> > Its scheduled to be moved anyway.
> 
> Yes, this is the right fix. We might be able to use one of the existing
> services, in fact (need to check).

(for anyone playing along, psycopg2 lets us use parameterized queries which are guaranteed to escape correctly, and we intend to move all DB connections out of the UI code anyway).

So it looks like the topcrasher code does a middleware call (/topcrash/sig/trend/) and also does a single direct DB call (the code linked in comment 6), which is named fetchTopcrasherVersions. I'm taking a look to figure out what this is actually used for ... it seems to only affect the "Ver" and "First Appearance".

Most of the data comes from the service call already, and the query is using the productdims and signature_productdims both of which are deprecated per http://socorro.readthedocs.org/en/latest/databasetablesbysource.html#depreciated-tables

Looking at the current middleware call, I think I can just add the version_string to the output and add a JOIN to get the "first appearance".
Moved this out of PHP and into python services, up for review at https://github.com/mozilla/socorro/pull/234
Commits pushed to https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/084e9aabc6e9ac36100c1d04800a4c39ecbab227
bug 711767 - move topcrashers report completely to middleware, to avoid DB queries which can be broken by funky signatures

https://github.com/mozilla/socorro/commit/82b1e7be831db3bd3f52313bd6d986966370e4b2
Merge pull request #234 from rhelmer/bug711767-move-topcrash-to-service

bug 711767 - move topcrashers report completely to middleware, to avoid ...
Github robot missed resolving this.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Commit pushed to https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/0f390e51f529b34346a1a89f5b486d5057740225
bug 711767 - move topcrashers report completely to middleware, to avoid DB queries which can be broken by funky signatures
Commits pushed to https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/9a4f0051a198f6f3c360aed8a689fac3354fe01c
bug 711767 - signature_products_rollup has multiple products, make sure to restrict the join on product_name to avoid duplicates

https://github.com/mozilla/socorro/commit/3e28ad6cef63bf7a75b5088f990c8c9e1cf8f079
Merge pull request #235 from rhelmer/bug711767-move-topcrash-to-service

bug 711767 - signature_products_rollup has multiple products, make sure ...
Commit pushed to https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/f688f942a936acebbf2f331371d8f61fe74eebdc
bug 711767 - signature_products_rollup has multiple products, make sure to restrict the join on product_name to avoid duplicates
Blocks: 712195
Commits pushed to https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/b775d305c9cfc5495d449747829896f7cb93cf87
bug 711767 - move topcrashers report completely to middleware, to avoid DB queries which can be broken by funky signatures

https://github.com/mozilla/socorro/commit/1a6301e44d3a624b679b90bda2d3e5fb4ddf9962
bug 711767 - signature_products_rollup has multiple products, make sure to restrict the join on product_name to avoid duplicates
QA verified, top crashers table displays data for all versions of products that have data.

Duplicate signature no longer appear in the table; 'Ver' and 'First Appearance columns show the expected values.
Status: RESOLVED → VERIFIED
Commits pushed to https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/0f390e51f529b34346a1a89f5b486d5057740225
bug 711767 - move topcrashers report completely to middleware, to avoid DB queries which can be broken by funky signatures

https://github.com/mozilla/socorro/commit/f688f942a936acebbf2f331371d8f61fe74eebdc
bug 711767 - signature_products_rollup has multiple products, make sure to restrict the join on product_name to avoid duplicates
Component: Socorro → General
Product: Webtools → Socorro
Component: General → Webapp
You need to log in before you can comment on or make changes to this bug.