1218443 - pageActions do not update correctly in some circumstances

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Dan Callahan [:callahad]]

STR:

1. Install my RES fork from https://github.com/callahad/RES-WebExtension (clone, run `make xpi` to build an XPI)
2. Visit http://example.com
3. Open a new tab, visit https://reddit.com/r/firefox
4. Notice that the pageAction icon appears in the URL bar
5. Switch back to the example.com tab

What should happen:

- The pageAction disappears, since it's not relevant for example.com

What actually happens:

- The pageAction persists, even on unrelated domains, and even after the relevant tab is closed.

Comment 1

[Kris Maglione [:kmag]]

I can't reproduce this. What build are you testing on?

Comment 2

[Dan Callahan [:callahad]]

I was using a Nightly Build from 10/25 or 10/26 at the time. I can't repro on today's nightly. So... coincidentally resolved?

Comment 3

[Kris Maglione [:kmag]]

I guess. Please reopen if you run into this again.

Comment 4

[Dan Callahan [:callahad]]

Oh no, sorry for the false hope. I *can* still repro with today's Nightly.

Immediately after installing the add-on, I see Bug 1220751, but not this bug.

After restarting the browser, I see this bug, but not Bug 1220751.

Comment 5

[Kris Maglione [:kmag]]

Hm. It sounds like we may be doing something that relies on event listeners being added in the right order. I'll look into it.

Comment 8

[Kris Maglione [:kmag]]

Attachment: [webext] Fix some instances of window listeners not being added correctly

I'd like to test for the listeners being added correctly at browser
startup, since that was a separate problem from them not being added
to new windows, but I can't think of a good way to do it.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8687765 [details] [diff] [review]
[webext] Fix some instances of window listeners not being added correctly

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

::: browser/components/extensions/ext-utils.js
@@ +414,5 @@
>  
>    // Returns an iterator for all browser windows. Unless |includeIncomplete| is
>    // true, only fully-loaded windows are returned.
>    *browserWindows(includeIncomplete = false) {
> +    let e = Services.wm.getEnumerator("");

Please comment about this.

Comment 10

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/6fa3b2df62cc80058ea2f63834dcbdd6ec7caa36
Bug 1218443: [webext] Fix some instances of window listeners not being added correctly. r=billm

Comment 11

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/c699b2c839a9 for browser_ext_pageAction_context.js leaks, https://treeherder.mozilla.org/logviewer.html#?job_id=5784496&repo=fx-team

Comment 12

[Kris Maglione [:kmag]]

Hm. It looks like this was caused by an extension.onMessage listener closing over a reference to the closed browser window. We should probably fix whatever's preventing them from being released before the end of the test.

Also relevant: `mach try` apparently picks the wrong test groups for our test paths, and for some reason I had to do a clobber build before I could reproduce this locally: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39bd7314824f

Comment 13

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/91404fe9f051540726004881c023b61e1ef91684
Bug 1218443: [webext] Fix some instances of window listeners not being added correctly. r=billm

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/91404fe9f051

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8687765 [details] [diff] [review]
[webext] Fix some instances of window listeners not being added correctly

Approval Request Comment
[Feature/regressing bug #]: Bug 1197422
[User impact if declined]:

Without this fix, the pageActions API is not stable enough to be useful, and the stability of several other APIs should be affected as well. Given the current attention that WebExtensions are receiving, especially on Developer Edition, even in their pre-release state, this instability is likely to give developers a negative impression of the framework, and make it unsuitable for near-term development.

[Describe test coverage new/current, TreeHerder]:

The tests for the affected pageAction API are fairly extensive, and this patch adds sufficient additional tests to ensure that the fix is effective.

[Risks and why]: 

Low. This patch only affects the WebExtensions API, which developers should be aware is in a pre-release state. The changes are fairly minor, and well-tested, so the chance for regression is minimal, and outweighed by the instability of the current state.

[String/UUID change made/needed]: None.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8687765 [details] [diff] [review]
[webext] Fix some instances of window listeners not being added correctly

This fix has been in Nightly for almost two weeks, and given that WebExtensions API is at pre-release quality, it makes sense to take this in Aurora44.

Comment 17

[Carsten Book [:Tomcat]]

also has problems during uplift 

grafting 315524:91404fe9f051 "Bug 1218443: [webext] Fix some instances of window listeners not being added correctly. r=billm"
merging browser/components/extensions/ext-utils.js
merging browser/components/extensions/test/browser/browser_ext_pageAction_context.js
merging browser/components/extensions/test/browser/head.js
warning: conflicts while merging browser/components/extensions/ext-utils.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

Comment 18

[Kris Maglione [:kmag]]

Attachment: Patch grafted onto aurora

Comment 19

[Kris Maglione [:kmag]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/858b1b930b16

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/858b1b930b16