590245 - EMCheckCompatibility is always submitted as False by the Crash Reporter to Socorro/Crash stats site

Status: VERIFIED FIXED

(Socorro :: General, task)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

+++ This bug was initially created as a clone of Bug #553114 +++

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Sorry, submitted too early.

Firefox 3.6.8 with Windows XP Home 32-bit

1. Install Crash me http://code.google.com/p/crashme/downloads/detail?name=crashme-advanced.xpi&can=2&q= (if necessary, drag and drop to the add-ons manager).
2. Restart and choose a crash reason from the Tools submenu, submit the crash.
3. Call about:crashes and open the latest crash report.

Actual result:
EMCheckCompatibility is always False.

Expected result:
It should be True in new profiles (and the ones without ignoring compatibility checks).

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Problem also present on trunk:

Mozilla/5.0 (Windows NT 5.1; rv:2.0b5pre) Gecko/20100824 Minefield/4.0b5pre (.NET CLR 3.5.30729)

Comment 3

[Dave Townsend [:mossop]]

Double check for yourself by clicking details in the submission UI but the crash reporter is definitely sending the right value: http://grab.by/63Ag. THis is a problem with socorro not collecting it properly.

Comment 4

[Dave Townsend [:mossop]]

Seems very similar to bug 572501 but Laura said the issue there was resolved

Comment 5

[Wayne Mery (:wsmwk)]

I just tested 4.0b5pre with a new profile and EMCheckCompatibility is reported by soccorro as being false - bp-63ab9f69-88fd-4274-80e1-4cbcf2100829.  crash reporter details shows EMCheckCompatibility: true

Comment 6

[(not currently active) Ted Mielczarek]

Laura: sounds like this is not fixed.

Comment 7

[Laura Thomson :laura]

Slating for 1.9.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm still seeing this as well.

It would really help to have this fixed so we know whether the recent spikes in mozilla-central crash reports due to binary-compatibility changes require us to blocklist extensions, or whether they're just from users who have compatibility checking turned off.

Comment 9

[Laura Thomson :laura]

Will get this into 1.7.7 release.  Lars, can you take a look please?

Comment 10

[K Lars Lohn [:lars] [:klohn]]

I have verified that both True and False values are in the database for the 'addons_checked' column.  So the problem looks like it is in the UI code.

So I did this research: the field 'EMCheckCompatibility' exists in the raw metadata json as a text field containing string literals 'true' or "false'.  The processor tests that value and places a proper boolean into the 'addons_checked' column the 'reports' table.  'addons_checked' doesn't appear to be propagated to the processed json.  

Looking at the PHP code that populates the crash, .../models/report.php, the function 'getByUUID' appears to be actor that fetches data to display.  It first does a query of the database, but it does not fetch the column 'addons_checked'.   The function then goes on to get the processed json.  It never fetches the raw json, so it has by-passed any opportunity to fetch the value for 'EMCheckCompatibility'.

In .../views/report/index.php, the code that displays 'EMCheckCompatibility' first checks to see if the key 'addons_checked' exists in the report.  To me, this shows intent that it was going to fetch the value from the database.  Since the database was never asked to fetch that value, it would never be found.

My reading of the PHP code is somewhat shaky.  It looks to me that the 'EMCheckCompatibility' field should not be displayed at all if 'addons_checked' is not found.  However, it clearly is displayed always as 'false'.  The could be used as a counter argument indicating that I'm not seeing the whole picture.

Comment 11

[Laura Thomson :laura]

I have a patch but I'm having trouble finding data on dev to test it.  Going to follow up in IRC.

Comment 12

[Laura Thomson :laura]

Attachment: possible solution

I think this is logically correct, but can't find the right data to test it fully.  If you don't have it either let me know and I'll go about configuring collector/monitor/processor to work again :P

Comment 13

[Robert Helmer [:rhelmer]]

Comment on attachment 526069 [details] [diff] [review]
possible solution

This patch is correct and necessary, but setting r- because there are further changes required to fix the bug.

I set up my dev instance to point to staging, and found a recent-enough crash ID that has "addons_checked = 't'" in the the postgres reports table: 0625bed4-954a-4b56-a658-2ca1b2110414

Here's the processed JSON for this crash:
https://crash-stats.stage.mozilla.com/reporter/dumps/0625bed4-954a-4b56-a658-2ca1b2110414.jsonz

This patch does not work as-is, but I dug into it a bit and found out why: the processed JSON has "addons_checked" set to "null", and all the keys from the reports table get overridden when the processed JSON is merged here:

http://code.google.com/p/socorro/source/browse/trunk/webapp-php/application/libraries/CrashReportDump.php#64

Three options I can think of:
1) correct the processed JSON
2) have CrashReportDump override processed JSON with reports table
3) skip overriding just "addons_checked" in CrashReportDump

I patched my dev instance with approach #3 as a test, and EMCheckCompatibility seems to match the value of reports.addons_checked (although I also had to modify views/report/index.php to check that ($report->addons_checked == 't') not just ($report->addons_checked), would be nice if this was converted into a boolean in PHP earlier in the process).

Comment 14

[Laura Thomson :laura]

Thanks rhelmer.  With a data point on staging to test (yay) I can make the fix v2.

Comment 15

[Laura Thomson :laura]

Attachment: version 2

Still having stupid issues with staging, but here's a revised patch.

Comment 16

[Robert Helmer [:rhelmer]]

Comment on attachment 526825 [details] [diff] [review]
version 2

lgtm: I can find examples on stage DB where addons_checked is 't' and EMCheckCompatibility is True (d1138bd5-bf13-4ce2-9961-b02dc2110407) and also the where addons_checked is 'f' and EMCheckCompatiblity is False (f9496295-b85b-4cff-be3e-c28ce2110418).

Comment 17

[Laura Thomson :laura]

Sending        webapp-php/application/libraries/CrashReportDump.php
Sending        webapp-php/application/models/report.php
Transmitting file data ..
Committed revision 3065.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks for the fix! It works fine: bp-df17661f-a0e7-4fba-9cf6-b5f5a2110426