Closed Bug 1218443 Opened 4 years ago Closed 4 years ago

pageActions do not update correctly in some circumstances

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox44 fixed, firefox45 fixed, b2g-v2.5 fixed)

RESOLVED FIXED
mozilla45
Iteration:
45.2 - Nov 30
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: callahad, Assigned: kmag)

References

Details

(Keywords: DevAdvocacy, Whiteboard: [pageAction])

Attachments

(2 files)

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.
Whiteboard: [pageAction]
I can't reproduce this. What build are you testing on?
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?
I guess. Please reopen if you run into this again.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WORKSFORME
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.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
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.
Assignee: nobody → kmaglione+bmo
Flags: blocking-webextensions?
Duplicate of this bug: 1220751
Summary: pageActions incorrectly appear on all tabs in a window → pageActions do not update correctly in some circumstances
Duplicate of this bug: 1218175
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.
Attachment #8687765 - Flags: review?(wmccloskey)
Iteration: --- → 45.2 - Nov 30
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.
Attachment #8687765 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/integration/fx-team/rev/6fa3b2df62cc80058ea2f63834dcbdd6ec7caa36
Bug 1218443: [webext] Fix some instances of window listeners not being added correctly. r=billm
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
https://hg.mozilla.org/integration/fx-team/rev/91404fe9f051540726004881c023b61e1ef91684
Bug 1218443: [webext] Fix some instances of window listeners not being added correctly. r=billm
https://hg.mozilla.org/mozilla-central/rev/91404fe9f051
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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.
Attachment #8687765 - Flags: approval-mozilla-aurora?
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.
Attachment #8687765 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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)
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(kmaglione+bmo)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.