740289 - If Addon.optionsURL points to an inline options file that doesn't exist, the details pane won't show.

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

If Addon.optionsURL points to an inline options file that doesn't exist, the details pane will throw an exception and subsequently won't show.

Warning: WARN addons.manager: Exception calling callback: [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIXMLHttpRequest.open]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: chrome://mozapps/content/extensions/extensions.js :: <TOP_LEVEL> :: line 2996"  data: no]
Source file: chrome://mozapps/content/extensions/extensions.js
Line: 2996

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Seems the patch in bug 704751 will fix this.

Comment 2

[Geoff Lankow (:darktrojan)]

(In reply to Blair McBride (:Unfocused) from comment #1)
> Seems the patch in bug 704751 will fix this.

It will, but I'm going to patch AddonWrapper to check for an invalid optionsType/optionsURL/options.xul combination as well.

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: fix

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 652279 [details] [diff] [review]
fix

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

With the extra logic in optionsType, I think its possible for optionsURL to have a value when optionsType==null. It shouldn't affect the frontend, but it does make the two properties inconsistent.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +5789,5 @@
>      if (!this.isActive)
>        return null;
>  
> +    let hasOptionsXUL = this.hasResource("options.xul");
> +    let hasOptionsURL = this.optionsURL;

Nit: Using this as a boolean, my brain would hurt less if you explicitly make it into a boolean.

@@ +5793,5 @@
> +    let hasOptionsURL = this.optionsURL;
> +
> +    if (aAddon.optionsType) {
> +      if (aAddon.optionsType == AddonManager.OPTIONS_TYPE_INLINE && hasOptionsXUL)
> +        return aAddon.optionsURL;

Er... optionsURL?

Also: Is the test not catching that?

@@ +5795,5 @@
> +    if (aAddon.optionsType) {
> +      if (aAddon.optionsType == AddonManager.OPTIONS_TYPE_INLINE && hasOptionsXUL)
> +        return aAddon.optionsURL;
> +
> +      return hasOptionsURL ? aAddon.optionsType : null;

If optionsType is OPTIONS_TYPE_INLINE, optionsURL is "options.xul", but the file doesn't actually exist, then this will still return OPTIONS_TYPE_INLINE.

Might want to simplify this logic, so there's one condition for each of the types, and return null in each block if the sanity checks fail.

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: v2

I didn't really intend to check for the file's existence, since the patch in the other bug deals with that (and needing to be able to check the various protocols would suck). All I'm trying to do here is eliminate the case where an optionsType is specified but there's no way we can fulfill that type due to missing options.xul and/or optionsURL.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 652388 [details] [diff] [review]
v2

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

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +5792,5 @@
> +    let hasOptionsXUL = this.hasResource("options.xul");
> +    let hasOptionsURL = !!this.optionsURL;
> +
> +    if (aAddon.optionsType) {
> +      switch (parseInt(aAddon.optionsType, 10)) {

Huh, I expected JS to do type coercion for switch statements. Odd.

@@ +5799,5 @@
> +        return hasOptionsURL ? aAddon.optionsType : null;
> +      case AddonManager.OPTIONS_TYPE_INLINE:
> +        return (hasOptionsXUL || hasOptionsURL) ? aAddon.optionsType : null;
> +      }
> +      throw new Error("Invalid optionsType (" + aAddon.optionsType + ") for " + this.id);

This exception shouldn't be able to be reached - loadManifestFromRDF() already ensures aAddon.optionsType is a valid value. But if you do want a fallback, just return null - its consistent with returning null in other cases here (and code using this API can't do anything useful with an exception here).

(Btw, bug 759642 is making all our manually thrown exceptions use Components.Exception, so use that in the future.)

Comment 7

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/795f17fe361d

(Try: https://tbpl.mozilla.org/?tree=Try&rev=e611d711a765)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/795f17fe361d