Closed Bug 1409697 Opened 7 years ago Closed 7 years ago

Reloading an add-on with options in about:addons displayed causes a second options section to be displayed

Categories

(WebExtensions :: General, defect, P5)

Product:
WebExtensions
Component:
General
Type:
defect

Tracking

(firefox57 wontfix, firefox59 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox59 --- verified

People

(Reporter: mkaply, Assigned: rpl)

References

Details

Attachments

(2 files)

Bug 1409697 - Fix multiple addon options rendered on addon reload.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug1409697.zip
1.11 MB, application/x-zip-compressed
Details 
If you load your add-on via about:debugging and then go to about:addons to display your options, then click reload in about:debugging, a second copy of your add-ons options displays below the first.
There's a dupe to this one, I just need to find it.
status-firefox57: --- → wontfix
Flags: needinfo?(amckay)
Apparently Luca is working on this and I couldn't find the dupe.
Assignee: nobody → lgreco
Flags: needinfo?(amckay)
Priority: -- → P5
I don't know if I'd call it a regression as such, since two copies seems better than the previous zero after a reload, but this was caused by bug 1385880 according to mozregression.
Blocks: 1385880
Attachment #8929517 - Flags: review?(aswan)
Comment on attachment 8929517 [details]
Bug 1409697 - Fix multiple addon options rendered on addon reload.

https://reviewboard.mozilla.org/r/200790/#review206046

::: toolkit/mozapps/extensions/content/extensions.js:3694
(Diff revision 1)
> +
> +    if (!browser.messageManager) {
> +      // If the browser.messageManager is undefined, the browser element has been
> +      // removed from the document in the meantime (e.g. due to a rapid sequence
> +      // of addon reload), ensure that the stack is also removed and return null.
> +      Cu.reportError(new Error("Extensions Options removed during initialization"));

Currently I'm reporting these errors (this one from line 3694 and the other one at line 3717) on the console instead of just raising the exception because they are currently raised when the new test is running.

The reason is that, because of how the event are delegated in this file, `_updateView` is called twice when the `onExternalInstall` is received:
- once because this object has subscribed itself as an "addon events listener" 
- and once more because it has also subscribed itself as a "install events listener".

Besides the above scenario (that is definitely an additional bug in extensions.js), another scenario that could lead to these errors is "a rapid sequence of addon.reload coming from the user" (e.g. coming from web-ext run when the user changes on the extension files are happening in a rapid sequence, or because a user clicks more than once the reload button from the "about:debugging" page in a short amount of time).
Comment on attachment 8929517 [details]
Bug 1409697 - Fix multiple addon options rendered on addon reload.

https://reviewboard.mozilla.org/r/200792/#review206064

::: browser/components/extensions/test/browser/browser-common.ini:98
(Diff revision 1)
>  [browser_ext_incognito_popup.js]
>  [browser_ext_lastError.js]
>  [browser_ext_menus.js]
>  [browser_ext_omnibox.js]
>  [browser_ext_openPanel.js]
> +[browser_ext_optionsPage_addon_reload.js]

Seems like this test should be in toolkit/mozapps/extensions/test/browser ?

::: browser/components/extensions/test/browser/browser_ext_optionsPage_addon_reload.js:29
(Diff revision 1)
> +}
> +
> +// Reloading the addon currently prevents the extension.awaitMessage test helper to be able
> +// to receive test messages from the reloaded extension, this test function helps to wait
> +// the extension has been restarted on addon reload.
> +function promiseAddonStartup() {

can just use AddonTEstUtils.promiseWebExtensionStartup() for this

::: browser/components/extensions/test/browser/browser_ext_optionsPage_addon_reload.js:101
(Diff revision 1)
> +      waitOptionsBrowserInserted(),
> +      promiseObserved(AddonManager.OPTIONS_NOTIFICATION_DISPLAYED),

do you need both of these?  seems like the observer should be sufficient?

::: toolkit/mozapps/extensions/content/extensions.js:3694
(Diff revision 1)
> +
> +    if (!browser.messageManager) {
> +      // If the browser.messageManager is undefined, the browser element has been
> +      // removed from the document in the meantime (e.g. due to a rapid sequence
> +      // of addon reload), ensure that the stack is also removed and return null.
> +      Cu.reportError(new Error("Extensions Options removed during initialization"));

This isn't really an error, do we need to report it here?

::: toolkit/mozapps/extensions/content/extensions.js:3717
(Diff revision 1)
> +
> +      if (!mm) {
> +        // If the browser.messageManager is undefined, the browser element has been
> +        // removed from the document in the meantime (e.g. due to a rapid sequence
> +        // of addon reload), ensure that the stack is also removed and return null.
> +        Cu.reportError(new Error("Extensions Options removed during initialization"));

same as above
Attachment #8929517 - Flags: review?(aswan) → review+
Status: NEW → ASSIGNED
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/44e3a8933093
Fix multiple addon options rendered on addon reload. r=aswan
https://hg.mozilla.org/mozilla-central/rev/44e3a8933093
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Attached file Bug1409697.zip — 
I can reproduce this issue on Firefox 57.0.1 (20171128222554) under Win 7 64-bit and Mac OS X 10.13.1.

This issue is verified as fixed on Firefox 59.0a1 (20171204234137) under Win 7 64-bit and Mac OS X 10.13.1.

Please see the attached videos.
Status: RESOLVED → VERIFIED
status-firefox59: fixed → verified
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: