knipesteven Reporter
	Description • 8 years ago

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161208153507

Steps to reproduce:

Install any addon with an options menu to firefox beta (51.0b9) or nightly (53.0a1) and go Tools -> Add-ons -> Select the addon

Examples include uBlock Origin, Save Link Menus, and my own addon (attached).

Tested on two devices, Android 6.0.1 and 5.1


Actual results:

None of the options are visible. Disabling then reenabling the addon usually makes the options menu reappear.

I've attached an addon I made where disabling then reenabling causes the 'Options' text to briefly appear, then disappears. This may be a problem with my addon but the primary bug applies to all addons with options.


Expected results:

Visible options menu. Works as expected on Firefox (50.0.2)

	Sorina Florean [:sflorean]
	Comment 1 • 8 years ago

Hello,

I didn't manage to reproduce what you described on all branches with Android 6.0.1 and 5.1 (LG G4, HTC Desire 820) and addons: uBlock Origin, Save Link Menus. If i enable and disable, the options menu is displayed. 
And, your addon I couldn't install on Nightly, only on Beta, Aurora. On what device did you see the issue?

	knipesteven Reporter
	Comment 2 • 8 years ago

The devices I used were a Moto G (1st gen) and an Nvidia SHIELD tablet K1.

I may have miscommunicated, after the enable+disable the options menu is displayed, it's before that that there's a problem. Here's some pictures to try to better explain the problem. http://imgur.com/a/OBuTq

Also, confirming that the latest Nightly doesn't install my addon. Not sure what the problem is there, it installed fine when I reported the bug. I'll try to look into it later.

	Dave Royal
	Comment 3 • 8 years ago

Same as bug 1334695?
When I raised that I thought it was only SDK simple prefs but I now see it affects all my addons.
3 devices - Android 4.4 and 5.1

Disabling+enabling makes the word 'OPTIONS' reappear very briefly, but the options themselves do not appear.

	u287251
	Comment 4 • 8 years ago

Many users have reported such issue with uBlock Origin:
https://github.com/gorhill/uBlock/issues/1379

Unfortunately, I am not in a position to try to reproduce for Firefox Android.

	Jan Henning [:JanH]
	Comment 5 • 8 years ago

[Tracking Requested - why for this release]:
Add-on options may fail to display in the add-on manager.

On my phone, I can reproduce this with Logview and uBlock, but not TapTranslate. For the former two, the options only appear after dis- and re-enabling the add-on, but not after a normal startup. I'm seeing this on Release as well as on Nightly - for the latter both with my regular and a fresh profile.

Unless someone beats me to it, I'll try and see if mozregression yields anything useful.

	Jan Henning [:JanH]
	Comment 7 • 8 years ago

Regressing push is https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=5b40e209b9a7, so most probably bug 1300808.

	Wladimir Palant
	Comment 8 • 8 years ago

The corresponding Adblock Plus issue is https://issues.adblockplus.org/ticket/4867.

	Gerry Chang [:gchang]
	Comment 9 • 8 years ago

Too late for 51. Mark 51 won't fix.

	Julien Cristau [:jcristau]
	Comment 10 • 8 years ago

This seems like a severe regression.  Tracking for 52 and up, though there's little time left if we want to fix this in 52.

	Ricardo
	Comment 11 • 8 years ago

Could someone please make this high priority? Two 'stable' versions already with this. Not everyone will figure out by themselves that it is necessary to disable and re-enable affected addons in order to restore their Options. Without being able to access addons options the impression is that a core feature is missing...

	Dave Royal
	Comment 12 • 8 years ago

As I already mentioned in comment 3, disabling and re-enabling does not restore the options of my SDK addons - originally bug 1334695. Nor does it work for others, e.g. Stylish 2.0.7

(And webextensions options_ui is not implemented yet.)

	Jan Henning [:JanH]
	Comment 13 • 8 years ago

Flagging again, since this was reset during this game of hot potato...

	David Hedlund
	Comment 14 • 8 years ago

The corresponding uBlock Origin issue is https://github.com/gorhill/uBlock/issues/1379.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 15 • 8 years ago

Agree to P1 this regression bug.
I'll get the team to look at this in the next sprint planning (mar20), but Fennec core browser team currently is quite occupied.

NI to Matthew that per comment 7 there could be something relevant to bug 1300808 or Bug 1302504

	Matthew Wein [:mattw] Assignee
	Comment 16 • 8 years ago

I confirmed that bug 1300808 is the root cause of this regression. I will follow up in a few minutes with a patch which should fix this issue.

	Kris Maglione [:kmag]
	Comment 18 • 8 years ago
mozreview-review

Comment on attachment 8847753 [details]
Bug 1329027 - Fix regression caused by bug 1300808

https://reviewboard.mozilla.org/r/120672/#review123688

::: mobile/android/chrome/content/aboutAddons.js:230
(Diff revision 1)
> +    if (aAddon.isWebExtension) {
> +      // TODO(matt): Support OPTIONS_TYPE_INLINE_BROWSER after bug 1302504 lands.
> -    switch (aAddon.optionsType) {
> +      switch (aAddon.optionsType) {
> -      case AddonManager.OPTIONS_TYPE_INLINE:
> +        case AddonManager.OPTIONS_TYPE_INLINE:
> -        optionsURL = aAddon.optionsURL || "";
> +          optionsURL = aAddon.optionsURL || "";
> -        break;
> +          break;
> -      default:
> +        default:
> -        optionsURL = "";
> +          optionsURL = "";
> -    }
> +      }
> +    }

This doesn't make sense. A WebExtension should never have OPTIONS_TYPE_INLINE, and any other extension with inline options should have OPTIONS_TYPE_INLINE. What options type are we trying to support here?

	Matthew Wein [:mattw] Assignee
	Comment 19 • 8 years ago
mozreview-review-reply

Comment on attachment 8847753 [details]
Bug 1329027 - Fix regression caused by bug 1300808

https://reviewboard.mozilla.org/r/120672/#review123688

> This doesn't make sense. A WebExtension should never have OPTIONS_TYPE_INLINE, and any other extension with inline options should have OPTIONS_TYPE_INLINE. What options type are we trying to support here?

I see, then I think this code should just be simplified to:

```javascript
if (aAddon.isWebExtension) {
  optionsURL = "";
}
```

We are trying to add support for options on Android, which is tracked by bug 1302504. Android expects options to be XUl-based, so when it tries to load a URL for WebExtension options it throws an XML parsing error - this is what bug 1300808 was meant to fix, but that bug caused this regression.

Do you agree or have a different suggestion?

	Matthew Wein [:mattw] Assignee
	Comment 20 • 8 years ago
mozreview-review-reply

Comment on attachment 8847753 [details]
Bug 1329027 - Fix regression caused by bug 1300808

https://reviewboard.mozilla.org/r/120672/#review123688

> I see, then I think this code should just be simplified to:
> 
> ```javascript
> if (aAddon.isWebExtension) {
>   optionsURL = "";
> }
> ```
> 
> We are trying to add support for options on Android, which is tracked by bug 1302504. Android expects options to be XUl-based, so when it tries to load a URL for WebExtension options it throws an XML parsing error - this is what bug 1300808 was meant to fix, but that bug caused this regression.
> 
> Do you agree or have a different suggestion?

s/Android/Fennec/g

	Kris Maglione [:kmag]
	Comment 21 • 8 years ago

No, we should be checking the optionsType exclusively. What is the optionsType in the case where this is failing?

	Matthew Wein [:mattw] Assignee
	Comment 22 • 8 years ago

Okay, I see what you are saying now. It's failing when the optionsType is OPTIONS_TYPE_INLINE_BROWSER. I'll upload a new patch.

	Kris Maglione [:kmag]
	Comment 24 • 8 years ago
mozreview-review

Comment on attachment 8847753 [details]
Bug 1329027 - Fix regression caused by bug 1300808

https://reviewboard.mozilla.org/r/120672/#review125074

::: mobile/android/chrome/content/aboutAddons.js:229
(Diff revision 2)
>      let updateable = (aAddon.permissions & AddonManager.PERM_CAN_UPGRADE) > 0;
>      let uninstallable = (aAddon.permissions & AddonManager.PERM_CAN_UNINSTALL) > 0;
>  
> -    // TODO(matt): Add support for OPTIONS_TYPE_INLINE_BROWSER once bug 1302504 lands.
>      let optionsURL;
> -    switch (aAddon.optionsType) {
> +    let optionsType = parseInt(aAddon.optionsType);

Why the parseInt? In any case, no need for a separate variable here.

::: mobile/android/chrome/content/aboutAddons.js:232
(Diff revision 2)
> -    // TODO(matt): Add support for OPTIONS_TYPE_INLINE_BROWSER once bug 1302504 lands.
>      let optionsURL;
> -    switch (aAddon.optionsType) {
> +    let optionsType = parseInt(aAddon.optionsType);
> +    switch (optionsType) {
>        case AddonManager.OPTIONS_TYPE_INLINE:
> -        optionsURL = aAddon.optionsURL || "";
> +        optionsURL = aAddon.optionsURL;

Still need the `|| ""` here.

::: mobile/android/chrome/content/aboutAddons.js:234
(Diff revision 2)
> +      case AddonManager.OPTIONS_TYPE_INLINE_BROWSER:
> +        // Ignore WebExtension options until options_ui support is added - Bug 1302504.
> +        optionsURL = "";
>          break;

There's no need for a separate case for this. It's already handled by the default.

	Kris Maglione [:kmag]
	Comment 27 • 8 years ago
mozreview-review

Comment on attachment 8847753 [details]
Bug 1329027 - Fix regression caused by bug 1300808

https://reviewboard.mozilla.org/r/120672/#review125620

	Pulsebot
	Comment 29 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/59f60e7f92ca
Fix regression caused by bug 1300808 r=kmag

	Carsten Book [:Tomcat]
	Comment 30 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/59f60e7f92ca

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 32 • 8 years ago

Matt, can you request uplift? This could still make the beta 7 build next Monday. 
I'd like to see us verify the fix in beta, too.

	Jan Henning [:JanH]
	Comment 33 • 8 years ago

Now working again with Nightly, thanks.

	Matthew Wein [:mattw] Assignee
	Comment 34 • 8 years ago

Comment on attachment 8847753 [details]
Bug 1329027 - Fix regression caused by bug 1300808

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1300808
[User impact if declined]: Users won't see add-on options for legacy add-ons
[Is this code covered by automated tests?]: No, but confirmed with manual tests
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes
   1. Download a legacy add-on and a WebExtension which have Preferences and confirm they show up in about:addons.
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No
[Why is the change risky/not risky?]: It is a simple one-line change that only affects add-on preferences.
[String changes made/needed]: N/A

	Gerry Chang [:gchang]
	Comment 35 • 8 years ago

Comment on attachment 8847753 [details]
Bug 1329027 - Fix regression caused by bug 1300808

Fix an addon regression that menu is missing. Aurora54+ & Beta53+.

	Carsten Book [:Tomcat]
	Comment 36 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/3394e3fa0f1e

	Carsten Book [:Tomcat]
	Comment 37 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/1b0b9e1915d1

	Oana Horvath [:ohorvath]
	Comment 38 • 8 years ago

Verified as fixed on Beta build 53.0b7.
Devices:
-Asus ZenPad 8.0 Z380KL (Android 6.0.1)
-Huawei Honor 5X (Android 5.1.1)

	Bogdan Surd, QA [:bsurd, NI]
	Comment 40 • 7 years ago

Hello,

 The settings appear for uBlock Origin for example but do not appear for Save Link Menus only after disabling and re-enabling the addon as mentioned in Comment 0.

The issue is the same for Beta (54.0b2) and Nightly (55.0a1). Could this still be Firefox related or is the issue related to the addon?

	Jan Henning [:JanH]
	Comment 41 • 7 years ago

"Save Link Menus" uses OPTIONS_TYPE_DIALOG. I don't know whether that is technically correct, although in practice it did work before bug 1300808.

Maybe we should just specifically block OPTIONS_TYPE_INLINE_BROWSER if I'm understanding the motivation beyond bug 1300808 right and leave the optionsURL alone for everything else just as before?

	Matthew Wein [:mattw] Assignee
	Comment 42 • 7 years ago

(In reply to Jan Henning [:JanH] from comment #41)
> "Save Link Menus" uses OPTIONS_TYPE_DIALOG. I don't know whether that is
> technically correct, although in practice it did work before bug 1300808.
> 
> Maybe we should just specifically block OPTIONS_TYPE_INLINE_BROWSER if I'm
> understanding the motivation beyond bug 1300808 right and leave the
> optionsURL alone for everything else just as before?

I think that sounds good. From my initial understanding I thought all XPCOM- and XUL-based add-ons used OPTIONS_TYPE_INLINE, but apparently that's not the case. Although, unless "Save Link Menus" is showing its options in a dialog box, I think it should be using OPTIONS_TYPE_INLINE. I'm working on adding support for OPTIONS_TYPE_INLINE_BROWSER in bug 1302504, so I could either fix this in that bug or upload a fix for this here depending on its urgency.

	Jan Henning [:JanH]
	Comment 43 • 7 years ago

It would be nice to uplift that to at least 54, so probably better here or maybe a clone of this bug?

	Gerry Chang [:gchang]
	Comment 44 • 7 years ago

Hi :mattw,
Do you consider to uplift this to Beta54?

	Jan Henning [:JanH]
	Comment 45 • 7 years ago

There's nothing new to uplift here.

	Matthew Wein [:mattw] Assignee
	Comment 46 • 7 years ago

Will fix in bug 1360448.