Closed
Bug 1277910
Opened 8 years ago
Closed 8 years ago
about:support should use nsIWindowMediator rather than nsIWindowWatcher
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 51
People
(Reporter: Dolske, Assigned: Dolske)
Details
Attachments
(1 file)
2.94 KB,
patch
|
Felipe
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
On OS X Nightly: Multiprocess Windows 1/2 (Enabled by user) I only have 1 window open, not clear why it's reporting 2 windows.
Assignee | ||
Comment 2•8 years ago
|
||
1) Use OS X Nightly 2) Open about:support, scroll down to Multiprocess Windows 3) See "N / N+1" reported when there are only N windows.
Flags: needinfo?(dolske)
Comment 3•8 years ago
|
||
Tested on the latest Nightly 50.0a1 (2016-07-26) and this is my result: Multiprocess Windows 1/1 (Enabled by default) Justin did you try with a new profile? Or did you change some config from about:config?
Flags: needinfo?(dolske)
Assignee | ||
Comment 4•8 years ago
|
||
Ah, so, here's the missing step: 1.5) Install a WebExtension that uses a generated page (in my case, https://addons.mozilla.org/en-US/firefox/addon/open-selected-links/) I ran the following in a scratchpad and inspected the window with devtools: var e = Services.ww.getWindowEnumerator("navigator:browser"); while (e.hasMoreElements()) { var w = e.getNext(); if (!w.location.toString().startsWith("data")) continue; console.log("Weird window:", w); } I get a weird window.location as data:application/vnd.mozilla.xul+xml;charset=utf-8,%3C?xml%20version=%221.0%22?%3E%0A%20%20%3Cwindow%20id=%22documentElement%22/%3E, and window.frames[0].location as _generated_background_page.html So... The first thought is that about:support should filter these out. But I'm also confused why these show up as type navigator:browser in the first place. (OTOH, the devtools console & scratchpad also show up in with that enumerator filter, so I'm not sure if that's just a default of some legacy requirement.)
Flags: needinfo?(dolske) → needinfo?(felipc)
Assignee | ||
Comment 5•8 years ago
|
||
The total window count actually goes up by 1 for every such WebExtension installed, if I install the one from the last comment plus https://addons.mozilla.org/en-US/firefox/addon/better-mxr/, I get "Multiprocess windows: 1/3" with only 1 window open on the desktop.
Comment 6•8 years ago
|
||
Hm. I can't see any reason those windows would show up in the navigator:browser enumerator. I guess it wouldn't hurt to add a windowtype attribute to the root node either way, though.
Comment 7•8 years ago
|
||
If I disabled the add-on I get "Multiprocess Windows 1/1 (Enabled by default)" If the add-on is enabled I get "Multiprocess Windows 1/2 (Enabled by default)"
Comment 8•8 years ago
|
||
Hah, so the prob here is that nsIWindowWatcher.getWindowEnumerator doesn't take any parameters.. It's just discarding the "navigator:browser" passed to it. Turns out this is only supported in nsIWindowMediator.getEnumerator
Flags: needinfo?(felipc)
Summary: about:support window count appears to be wrong → about:support should use nsIWindowMediator rather than nsIWindowWatcher
Assignee | ||
Comment 9•8 years ago
|
||
Ah! *doh* Well then that's easy to fix. I also found a couple other instances of getWindowEnumerator() being called with an arg, and fixed those too. (Arguably we could fix getWindowEnumerator to just take a filter, or even just settle on a single interface, but meh.)
Assignee: nobody → dolske
Assignee | ||
Comment 10•8 years ago
|
||
(No reviewboard because it stopped working for me, and after dicking around with it for 15 minutes I still can't get it working.)
Attachment #8775722 -
Flags: review?(felipc)
Comment 11•8 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #10) > (No reviewboard because it stopped working for me, and after dicking around > with it for 15 minutes I still can't get it working.) I wound up having do this again a few days ago: http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html#manually-associating-your-ldap-account-with-mozreview
Comment 12•8 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #10) > (No reviewboard because it stopped working for me, and after dicking around > with it for 15 minutes I still can't get it working.) (Also, our built-in spell check dictionary should really probably include the word "dicking"...)
Updated•8 years ago
|
Attachment #8775722 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/bdfe8fd853b927c7d4fc3a45237f4cd35e49e86c Bug 1277910 - about:support should use nsIWindowMediator rather than nsIWindowWatcher r=felipe
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bdfe8fd853b9
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Comment 15•8 years ago
|
||
Comment on attachment 8775722 [details] [diff] [review] Patch v.1 Approval Request Comment [Feature/regressing bug #]: about:support shows wrong number of Multiprocess Windows if there are webextensions enabled [User impact if declined]: Wrong count of windows. Since Firefox 49 is the first one that might rollout out e10s for webextension addons, it would be nice to have this bug fixed there. [Describe test coverage new/current, TreeHerder]: landed on central [Risks and why]: minimal, limited to about:support [String/UUID change made/needed]: none
Attachment #8775722 -
Flags: approval-mozilla-beta?
Attachment #8775722 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
status-firefox50:
--- → affected
Comment 16•8 years ago
|
||
Comment on attachment 8775722 [details] [diff] [review] Patch v.1 Review of attachment 8775722 [details] [diff] [review]: ----------------------------------------------------------------- This patch fixes the wrong number of Multiprocess Windows in about:support. Take it in 50 aurora and 49 beta. Should be in 49 beta 2.
Attachment #8775722 -
Flags: approval-mozilla-beta?
Attachment #8775722 -
Flags: approval-mozilla-beta+
Attachment #8775722 -
Flags: approval-mozilla-aurora?
Attachment #8775722 -
Flags: approval-mozilla-aurora+
Comment 17•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/d8fca4cfc033
Comment 18•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/bc4c1a256d84
You need to log in
before you can comment on or make changes to this bug.
Description
•