Closed Bug 993539 Opened 6 years ago Closed 6 years ago

sourceURI property of add-on fails if the repository has been updated

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: mossop, Assigned: mossop)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
Because we attempt to create a new nsIURI from an nsIURI :s
Attachment #8403452 - Flags: review?(irving)
Comment on attachment 8403452 [details] [diff] [review]
patch

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

This looks somewhat broken in the surrounding context; other places in XPIProvider and XPIProviderUtils assume that AddonInternal.releaseNotesURI is a string - we try to put it directly into the JSON to save, and in AI_updateAddonURIs we deliberately assign AddonInstall.releaseNotesURI.spec to AddonInternal.releaseNotesURI.

I think the right fix is to ensure we always store the string (nsIURI.spec) in AddonInternal.releaseNotesURI.

In other news, I seem to be drunk with my newfound powers.
Attachment #8403452 - Flags: review?(irving) → review-
(In reply to :Irving Reid from comment #1)
> Comment on attachment 8403452 [details] [diff] [review]
> patch
> 
> Review of attachment 8403452 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks somewhat broken in the surrounding context; other places in
> XPIProvider and XPIProviderUtils assume that AddonInternal.releaseNotesURI
> is a string - we try to put it directly into the JSON to save, and in
> AI_updateAddonURIs we deliberately assign AddonInstall.releaseNotesURI.spec
> to AddonInternal.releaseNotesURI.
> 
> I think the right fix is to ensure we always store the string (nsIURI.spec)
> in AddonInternal.releaseNotesURI.

That's right, AddonInternal.releaseNotesURI and AddonInternal.sourceURI are all strings. This code is generating the value of AddonWrapper.sourceURI aand AddonWrapper.releaseNotesURI though which are all nsIURIs. The problem is that AddonInternal.sourceURI is a string and the cached repository add-on's sourceURI is an nsIURI (http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/AddonRepository.jsm#1828). Right now the code assumes it is a string for some reason (maybe it used to be?) so it tries to generate a new nsIURI from it which fails.
Flags: needinfo?(irving)
Comment on attachment 8403452 [details] [diff] [review]
patch

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

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +6326,5 @@
>        let target = chooseValue(aAddon, aProp)[0];
>        if (!target)
>          return null;
> +      if (target instanceof Ci.nsIURI)
> +        return target;

OK, I've read through this code enough times now. Aside from conflicting feelings about the confusing mix of representations between our object types, and wanting a test, how about:

let [target, fromRepo] = chooseValue(aAddon, aProp);
if (!target)
  return null;
if (fromRepo)
  return target;
return NetUtil.newURI(target);
Flags: needinfo?(irving)
Assignee: nobody → dtownsend+bugmail
Attached patch patchSplinter Review
Yeah the mixed types sucks but right now I want to get this fixed quickly as it's breaking the add-on debugger :(
Attachment #8403452 - Attachment is obsolete: true
Attachment #8404278 - Flags: review?(irving)
Attachment #8404278 - Flags: review?(irving) → review+
https://hg.mozilla.org/mozilla-central/rev/37e27fd1a94a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.