Closed Bug 1240651 Opened 4 years ago Closed 4 years ago

Annotate addonId into crash report

Categories

(Core :: Security: CAPS, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- affected
firefox47 --- fixed

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch. (obsolete) — Splinter Review
add ifdef
Attachment #8709249 - Attachment is obsolete: true
Attached patch Patch.Splinter Review
Attachment #8709353 - Attachment is obsolete: true
Attachment #8709354 - Flags: review?(wmccloskey)
Attachment #8709354 - Flags: review?(bobbyholley)
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.
Attachment #8709354 - Flags: review?(wmccloskey) → review+
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.
Attachment #8709354 - Flags: review?(gfritzsche)
Attachment #8709354 - Flags: review?(bobbyholley)
Attachment #8709354 - Flags: review+
(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.
Flags: needinfo?(allstars.chh)
Comment on attachment 8709354 [details] [diff] [review]
Patch.

I'm not a data collection peer - redirecting.
Attachment #8709354 - Flags: review?(gfritzsche) → review?(benjamin)
Sorry for the trouble, I didn't notice that I have to get another review.
I'll be more careful next time.
Flags: needinfo?(allstars.chh)
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.
https://hg.mozilla.org/mozilla-central/rev/62600c1a0dcb
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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"
Flags: needinfo?(allstars.chh)
I think Bill might be better able to answer here.
Flags: needinfo?(allstars.chh) → needinfo?(wmccloskey)
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.
Flags: needinfo?(wmccloskey)
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.
Attachment #8709354 - Flags: review?(benjamin) → review-
You need to log in before you can comment on or make changes to this bug.