Closed Bug 1307551 Opened 5 years ago Closed 4 years ago

Disabled browserAction still loads popup on click

Categories

(WebExtensions :: Frontend, defect, P1)

defect

Tracking

(firefox50 unaffected, firefox51 verified, firefox52 verified)

VERIFIED FIXED
mozilla52
Tracking Status
firefox50 --- unaffected
firefox51 --- verified
firefox52 --- verified

People

(Reporter: freaktechnik, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Whiteboard: triaged)

Attachments

(2 files)

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.
Assignee: nobody → kmaglione+bmo
Priority: -- → P1
Whiteboard: triaged
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 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 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 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!
Attachment #8806404 - Flags: review?(bob.silverberg) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcb9ea438c0146ecc96754940884aeb852909d01
Bug 1307551: Don't attempt to pre-load popup for disabled browserAction. r=bsilverberg
https://hg.mozilla.org/mozilla-central/rev/dcb9ea438c01
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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.
Attachment #8806404 - Flags: approval-mozilla-aurora?
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.
Attachment #8806404 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
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.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.