Closed Bug 1277910 Opened 8 years ago Closed 8 years ago

about:support should use nsIWindowMediator rather than nsIWindowWatcher

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox49 --- fixed
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: Dolske, Assigned: Dolske)

Details

Attachments

(1 file)

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.
Justin please add some steps to reproduce it.
Flags: needinfo?(dolske)
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)
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)
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)
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.
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.
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)"
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
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
Attached patch Patch v.1Splinter Review
(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)
(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
(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"...)
Attachment #8775722 - Flags: review?(felipc) → review+
https://hg.mozilla.org/integration/fx-team/rev/bdfe8fd853b927c7d4fc3a45237f4cd35e49e86c
Bug 1277910 - about:support should use nsIWindowMediator rather than nsIWindowWatcher r=felipe
https://hg.mozilla.org/mozilla-central/rev/bdfe8fd853b9
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
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?
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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: