Closed
Bug 711767
Opened 14 years ago
Closed 14 years ago
Unable to display top crashers for Fennec 11.0a1
Categories
(Socorro :: Webapp, task)
Socorro
Webapp
Tracking
(Not tracked)
VERIFIED
FIXED
2.3.5
People
(Reporter: scoobidiver, Assigned: rhelmer)
References
()
Details
Attachments
(1 file)
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
Updated•14 years ago
|
Target Milestone: --- → 2.3.5
Reporter | ||
Comment 3•14 years ago
|
||
(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 | ||
Updated•14 years ago
|
Assignee: nobody → rhelmer
Assignee | ||
Comment 4•14 years ago
|
||
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
Assignee | ||
Comment 5•14 years ago
|
||
* 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.
Assignee | ||
Comment 6•14 years ago
|
||
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.
Assignee | ||
Comment 7•14 years ago
|
||
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
Comment 8•14 years ago
|
||
Lets move this to a middleware service and let psycopg2 handle the escaping. Its scheduled to be moved anyway.
Assignee | ||
Comment 9•14 years ago
|
||
(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).
Comment 10•14 years ago
|
||
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.
Assignee | ||
Comment 12•14 years ago
|
||
(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".
Assignee | ||
Comment 13•14 years ago
|
||
Assignee | ||
Comment 14•14 years ago
|
||
Moved this out of PHP and into python services, up for review at https://github.com/mozilla/socorro/pull/234
Comment 15•14 years ago
|
||
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 ...
Comment 16•14 years ago
|
||
Github robot missed resolving this.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 17•14 years ago
|
||
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
Comment 18•14 years ago
|
||
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 ...
Comment 19•14 years ago
|
||
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
Comment 20•14 years ago
|
||
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
Comment 21•14 years ago
|
||
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
Comment 22•14 years ago
|
||
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
Updated•14 years ago
|
Component: Socorro → General
Product: Webtools → Socorro
Updated•14 years ago
|
Component: General → Webapp
You need to log in
before you can comment on or make changes to this bug.
Description
•