1240651 - Annotate addonId into crash report

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

As suggested by Bobby and Bill in 
https://bugzilla.mozilla.org/show_bug.cgi?id=1210099#c5 and
https://bugzilla.mozilla.org/show_bug.cgi?id=1210099#c18

I'll try to annotate addon id into crash report.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f50fc8dc4ac

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28232c7b1cb4

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f8b4008cf35

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

add ifdef

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8709354 [details] [diff] [review]
Patch.

Review of attachment 8709354 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: caps/BasePrincipal.cpp
@@ +129,5 @@
> +#ifdef MOZ_CRASHREPORTER
> +      CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("Crash_AddonId"),
> +                                         NS_ConvertUTF16toUTF8(mAddonId));
> +#endif
> +      MOZ_RELEASE_ASSERT(false);

This can just be MOZ_CRASH.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8709354 [details] [diff] [review]
Patch.

Review of attachment 8709354 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the BasePrincipal.cpp changes.

Need data collection review. Flagging Georg.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cc1bbba0b4b

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a76600c2820bdc68e729a376d16cfe614a4a0b56
Bug 1240651 - Annotate addonId into crash report. r=billm, bholley

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to Yoshi Huang[:allstars.chh] from comment #10)
> https://hg.mozilla.org/integration/b2g-inbound/rev/
> a76600c2820bdc68e729a376d16cfe614a4a0b56
> Bug 1240651 - Annotate addonId into crash report. r=billm, bholley

This has pending data collection review, it shouldn't have landed yet.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8709354 [details] [diff] [review]
Patch.

I'm not a data collection peer - redirecting.

Comment 13

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=3796035&repo=b2g-inbound

Comment 14

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/b2g-inbound/rev/5426c8d6c687

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Sorry for the trouble, I didn't notice that I have to get another review.
I'll be more careful next time.

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7db6a580967

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/62600c1a0dcb

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This is a pretty bad crash, so I landed the patch with only part of the data collection. We already collect add-on IDs in crashes, so there shouldn't be any harm in doing it here. I left out the part that collects UUIDs since that's a new thing that I guess would require data collection review.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/62600c1a0dcb

Comment 20

[Benjamin Smedberg]

Comment on attachment 8709354 [details] [diff] [review]
Patch.

I don't understand what ExtensionManagement.jsm is trying to do. We already have the list of addons in crash reports, right? How is this list different? 

What is mAddonId? Addons can have arbitrary strings as their extension ID, so I'm a little concerned about this assert in general, unless mAddonId is somehow unrelated to the <em:id> ID. And in general for this kind of annotation, you'd want something more specific, such as "Bug1240651_AddonID"

Comment 21

[Bobby Holley (:bholley)]

I think Bill might be better able to answer here.

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This code isn't really relevant anymore since we now know that the OriginAttributes crash wasn't related to a particular add-on. However, each WebExtension gets its own unique UUID when it's installed. This UUID is used in moz-extension: URIs rather than the add-on ID to make fingerprinting harder. The patch was including the mapping between UUIDs and add-on IDs in the crash report.

Also, my understanding is that add-on IDs have to match this regular expression here:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#270
So I think the assertion is valid.

Comment 23

[Benjamin Smedberg]

Comment on attachment 8709354 [details] [diff] [review]
Patch.

In that case, the "Extensions" annotation is very poorly named: it should be something like "ExtensionUUIDMappings" and you might as well make it machine-parseable JSON instead of this.

The other annotation, if it's still needed, should reference a specific bug and not be generic.