521906 - Breakpad should report if extension compatibility overrides are being used

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Justin Dolske [:Dolske]]

A suspicion on some crashes is that extensions (esp. with binary components) are crashing as a result of the user having overridden compatibility checking. (eg, by setting extensions.checkCompatibility = false, though I think Nightly Tester Tools might to it a different way).

It would be good to have the crash reporter report if this is happening (at least the pref, maybe more), so that by looking at the report we can know if the user dug their own hole, and how big of a problem this is. [The suggestion was raised of resetting this pref after a major update, so data would help with that decision.]

Comment 1

[(not currently active) Ted Mielczarek]

Needs some EM expertise.

Comment 2

[Dave Townsend [:mossop]]

One cheap way would be just to include the value of extensions.checkCompatibility in the crash report. A slightly better way would be to actually check whether any add-ons were installed that should be incompatible in that case, though this would be a perf hit unless you were happy to defer gathering that information until after startup. Neither of these though can detect what Nightly Tester Tools does.

A more all encompassing way would be to do lookups on AMO based on the extension ID and version already in the crash report and see what compatibility version the extension really should have, though that wouldn't work for non-AMO hosted add-ons of course.

Comment 3

[Justin Dolske [:Dolske]]

Just the pref would be a good start, to allow correlating crashes with users who have the pref enabled.

Though for accurately knowing if a compatibility override actually resulted in loading an outdated extension, we'd probably want the client to report that explicitly. If not "incompatible Extension X was loaded anyway", at least "loaded at least 1 incompatible extension".

Comment 4

[Dave Townsend [:mossop]]

Actually thinking about the way we currently load up the list of add-ons it probably wouldn't make for a perf hit to include whether it should be incompatible at that point.

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

So, maybe just do this? (subject to any changes bug 521905 ends up introducing).

It occurs to me that since we get the list of installed extensions, we can just look to see if Nightly Tester Tools is installed. That can be a cue to look closer at the possibility of forcibly-installed extensions. [It might be possible to use NTT, uninstall it, and still have an incompatible addon used, but I'd imagine that case is rare (if even possible?).] Not perfect, but probably good enough for the purposes of topcrash analysis.

[n.b. didn't test this yet]

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 409838 [details] [diff] [review]
Patch v.1

I'm ok with this, but if you want to mark specific items as being forcibly enabled then I think that can be done inside of _updateExtensionsManifest without really incurring a penalty

Comment 7

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/0b8f4bc23a6c

Comment 8

[(mostly gone) XtC4UaLL [:xtc4uall]]

I presume the information of the pref's status is not exposed to the public Crash Report (or its JSON)?
I'm asking because testing a recent Hourly build including this checkin (http://hg.mozilla.org/mozilla-central/rev/5d321b7b899f) + the "Crash Me" Extension + setting "extensions.checkCompatibility;false" results in a Crash Report (-> bp-6a3af55e-2083-4e12-b331-dedc52091107) where i cannot see anything what mentions the pref's status?!

Comment 9

[(not currently active) Ted Mielczarek]

We will need to file a separate bug report to get Socorro to collect and store this information.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 409838 [details] [diff] [review]
Patch v.1

a192=beltzner

Comment 11

[Justin Dolske [:Dolske]]

Pushed to 192: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/11053e6b2626

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 409838 [details] [diff] [review]
Patch v.1

Is this a 1.9.1-applicable patch? With "LightweightThemeManager" in the context I have my doubts.

In any case we want more testing/baking of this so it's not one we want to approve with hours to go until the code-freeze. Next time.

Comment 13

[Samuel Sidler (old account; do not CC)]

(And where's the Socorro bug to actually start collecting this information?)

Comment 14

[Justin Dolske [:Dolske]]

(bug 521906 for the Socorro followup)