1307551 - Disabled browserAction still loads popup on click

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P1)

Description

[Martin Giger [:freaktechnik]]

Attachment: disabled-browseraction-load.zip

When clicking on a disabled browserAction, the popup contents should not load, however it appears the popup page is still loaded.

The attached extension has a popup, that asks to connect to the background page when loading. The background page then logs when a connection request comes in. The browserAction is disabled, yet when I click on it every about 5 clicks (not consistent), it will still show that an incoming connection is created.

I was only able to reproduce this in Nightly (on Linux and Windows), both with e10s enabled, while my release versions both have e10s disabled and don't reproduce.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1307551: Don't attempt to pre-load popup for disabled browserAction.

Review commit: https://reviewboard.mozilla.org/r/89844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89844/

Comment 2

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8806404 [details]
Bug 1307551: Don't attempt to pre-load popup for disabled browserAction.

https://reviewboard.mozilla.org/r/89844/#review89322

The change looks good, but I have a question about the test.

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:340
(Diff revision 1)
> +
> +
> +  // Test completed click.
> +  EventUtils.synthesizeMouseAtCenter(widget.node, {type: "mousedown", button: 0}, window);
> +
> +  is(browserAction.pendingPopup, null, "Have pending popup");

The comment should read `Have no pending popup`.

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:347
(Diff revision 1)
> +
> +  // We need to do these tests during the mouseup event cycle, since the click
> +  // and command events will be dispatched immediately after mouseup, and void
> +  // the results.
> +  let mouseUpPromise = BrowserTestUtils.waitForEvent(widget.node, "mouseup", event => {
> +    isnot(browserAction.pendingPopup, null, "Pending popup was not cleared");

I am confused by this. Shouldn't `browserAction.pendingPopup` and `browserAction.pendingPopupTimeout` both be `null` at this point, just as they were at lines 340 and 341? And if that is the case, why is the test passing?

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8806404 [details]
Bug 1307551: Don't attempt to pre-load popup for disabled browserAction.

https://reviewboard.mozilla.org/r/89844/#review89322

> I am confused by this. Shouldn't `browserAction.pendingPopup` and `browserAction.pendingPopupTimeout` both be `null` at this point, just as they were at lines 340 and 341? And if that is the case, why is the test passing?

They should, yes. I copied this from the previous test and forgot to change them. They're passing because apparently the check function needs to be the fourth argument, rather than the third. :/

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8806404 [details]
Bug 1307551: Don't attempt to pre-load popup for disabled browserAction.

https://reviewboard.mozilla.org/r/89844/#review89322

> They should, yes. I copied this from the previous test and forgot to change them. They're passing because apparently the check function needs to be the fourth argument, rather than the third. :/

Which apparently changed before this test was written, but I didn't realize: http://searchfox.org/mozilla-central/diff/2aa057e7ff5585b4500c27b1eca9b63b751d63fd/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#260

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8806404 [details]
Bug 1307551: Don't attempt to pre-load popup for disabled browserAction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89844/diff/1-2/

Comment 6

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8806404 [details]
Bug 1307551: Don't attempt to pre-load popup for disabled browserAction.

https://reviewboard.mozilla.org/r/89844/#review89348

Looks good, thanks!

Comment 7

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dcb9ea438c0146ecc96754940884aeb852909d01
Bug 1307551: Don't attempt to pre-load popup for disabled browserAction. r=bsilverberg

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/dcb9ea438c01

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8806404 [details]
Bug 1307551: Don't attempt to pre-load popup for disabled browserAction.

Approval Request Comment
[Feature/regressing bug #]: Bug 1259093
[User impact if declined]: This may cause extension popups for disabled toolbar buttons to run code with side-effects under certain circumstances. This is made more likely by the fact that these buttons appearances do not clearly indicate when they're disabled.
[Describe test coverage new/current, TreeHerder]: The related features are covered by extensive tests, and this patch adds coverage for this issue.
[Risks and why]: Very low. This change simply checks a flag to prevent pre-loading a popup for a disabled button. Even if it somehow triggered false positives, the worst effect would be a visual performance regression.
[String/UUID change made/needed]: None.

Comment 10

[Gerry Chang [:gchang]]

Comment on attachment 8806404 [details]
Bug 1307551: Don't attempt to pre-load popup for disabled browserAction.

Fix a webextension issue related to popups. Take it in 51 aurora.

Comment 11

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 52.0a1 (2016-10-04) under Windows 10 64-bit.

Verified fixed on Firefox 52.0a1 (2016-11-06) under Windows 10 64-bit and Ubuntu 16.04 32-bit. The panel content in no longer loaded while clicking on the webextension icon.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e83b4da197ff

Comment 13

[Vasilica Tamas, QA [:vtamas]]

Confirm that this bug is also fixed on Firefox 51.0a2 (2016-11-08) under Windows 10 64-bit and Ubuntu 16.04 32-bit.