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

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop] (he/him)]

Attachment: patch

Because we attempt to create a new nsIURI from an nsIURI :s

Comment 1

[:Irving Reid (No longer working on Firefox)]

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.

Comment 2

[Dave Townsend [:mossop] (he/him)]

(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.

Comment 3

[:Irving Reid (No longer working on Firefox)]

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);

Comment 4

[Dave Townsend [:mossop] (he/him)]

Attachment: patch

Yeah the mixed types sucks but right now I want to get this fixed quickly as it's breaking the add-on debugger :(

Comment 5

[Dave Townsend [:mossop] (he/him)]

https://hg.mozilla.org/integration/fx-team/rev/37e27fd1a94a

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/37e27fd1a94a