1448135 - 2 browser windows are opened when click local html file. one is white/black window without UI, the other one seems normal

Status: VERIFIED FIXED

(Firefox :: Shell Integration, defect)

Description

[Alice0775 White]

Reproducible : always

Steps To Reproduce:
1. Nightly install as a default browser.
2. Close all browser window
3. Double click on a local html file

Actual Results:
2 browser windows(A black window and a normal window) are opened.
The black window is completely black, no UI displayed.
And the black window cannot close. you must kill from right click taskbutton.

If enabled titlebar, the black window has only titlebar.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e21a2a57d05dfe3c5ff8ba71131127fa781ffdd0&tochange=f026ead5dbfce9d6530429ac568ecc5544cc9b3b

Regressed by: f026ead5dbfc	Florian Quèze — Bug 1447719 - Set browser.startup.blankWindow to true on Windows and Linux, r=mconley.


And I confirmed that setting browser.startup.blankWindow = false fixed the problem.


@:florian,
Your patch seems to cause the regression. Can you look into this?

Comment 2

[Alice0775 White]

In addition to the comment #0.
If HWA is enabled, white window is opened instead of black.

Comment 3

[Darkspirit]

Debian Testing (KDE, Radeon RX480, WebRender)
Opening Nightly by clicking on a link in Thunderbird has the same result. I didn't run mozregression, but it's unreproducible when browser.startup.blankWindow is false.

Comment 4

[Alice0775 White]

Attachment: one_black_screen_and_the_other_normal.mp4

Screenshot Nightly running on VM(sorry it so slow)

Comment 5

[Florian Quèze [:florian] (on leave until October 9)]

Attachment: Patch

This is easy to fix for this specific case, but I would like to audit all the other openWindow calls in this file before requesting review.

Comment 9

[Florian Quèze [:florian] (on leave until October 9)]

Attachment: Patch v2

This turned out to require a pretty significant refactoring of the nsBrowserContentHandler.js file, but it's nicely removing code duplication :-).

Comment 10

[Florian Quèze [:florian] (on leave until October 9)]

Attachment: Patch v3

Fixed eslint errors.

There's still a blank window that flickers for -chrome (for the non-preference cases, which I think nobody uses), -silent (I'm not sure it's still used by anybody, it was used to set the browser as the default browser 10+ years ago), -screenshot (this sets an environment variable early during startup that I may be able to access to avoid opening the blank window).

-private doesn't work with browser.startup.blankWindow, I think that'll be for a follow-up.

Comment 11

[Gary [:streetwolf52]]

The STR for this bug mentions opening up a local HTML file.  I want to add that the problem also happens opening up a link in some programs.  The program CPU-Z is just such a program.

Comment 12

[Florian Quèze [:florian] (on leave until October 9)]

(In reply to Florian Quèze [:florian] from comment #10)

> There's still a blank window that flickers for -chrome (for the
> non-preference cases, which I think nobody uses), -silent (I'm not sure it's
> still used by anybody, it was used to set the browser as the default browser
> 10+ years ago)

I sent https://mail.mozilla.org/pipermail/firefox-dev/2018-March/006310.html to propose removing these 2 parameters.

> -screenshot (this sets an environment variable early during
> startup that I may be able to access to avoid opening the blank window).

Actually, the blank window never gets shown when I use --screenshot on the command line. I suspect triggering the headless mode means we don't show any window, even if there's an openWindow call. Anyway, nice to take it off the list of fixes needed :-).

> -private doesn't work with browser.startup.blankWindow, I think that'll be
> for a follow-up.

I filed bug 1449925.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8963308 [details] [diff] [review]
Patch v3

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

That's a very nice refactor, florian! Thanks!

::: browser/components/nsBrowserContentHandler.js
@@ +172,5 @@
>  
>    return update.getProperty("openURL") || defaultOverridePage;
>  }
>  
> +function openBrowserWindow(cmdLine, urlOrUrlList, postData, forcePrivate) {

This function could use some documentation - specifically which arguments are optional, and the variations they can take (like urlOrUrlList), and the conditional arguments (postData).

You could also do postData = null and avoid the postData || null down below, though I'm not certain that's much clearer. Just neat that we can do it. :)

@@ +173,5 @@
>    return update.getProperty("openURL") || defaultOverridePage;
>  }
>  
> +function openBrowserWindow(cmdLine, urlOrUrlList, postData, forcePrivate) {
> +  let chromeURL = Services.prefs.getCharPref("browser.chromeURL");

Seems ripe for a lazy one-time getter - this is probably _never_ going to change, so it seems kinda silly to ask prefs for it each time.

(I admit, this isn't probably the hottest codepath, but still)

Comment 14

[Florian Quèze [:florian] (on leave until October 9)]

(In reply to Mike Conley (:mconley) from comment #13)

Thanks for the review!

> @@ +173,5 @@
> >    return update.getProperty("openURL") || defaultOverridePage;
> >  }
> >  
> > +function openBrowserWindow(cmdLine, urlOrUrlList, postData, forcePrivate) {
> > +  let chromeURL = Services.prefs.getCharPref("browser.chromeURL");
> 
> Seems ripe for a lazy one-time getter - this is probably _never_ going to
> change, so it seems kinda silly to ask prefs for it each time.
> 
> (I admit, this isn't probably the hottest codepath, but still)

Actually there was one that I removed: https://searchfox.org/mozilla-central/rev/57bbc1ac58816dc054df242948f3ecf75e12df5f/browser/components/nsBrowserContentHandler.js#300-310
Because this is going to be called exactly once, so I don't see the point of keeping a copy of this pref value in the scope of this component.

Comment 15

[Florian Quèze [:florian] (on leave until October 9)]

Also, I'm tempted to entirely eliminate this "browser.chromeURL" pref, but I figured it was way out of the scope of this bug.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4c46aab1b2f
re-use the blank window whenever possible in nsBrowserContentHandler.js, r=mconley.

Comment 17

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/e4c46aab1b2f

Comment 20

[Ionuț Goldan [:igoldan]]

Perf wins!

== Change summary for alert #12439 (as of Fri, 30 Mar 2018 09:04:06 GMT) ==

Improvements:

  6%  sessionrestore linux64 opt e10s stylo     282.75 -> 264.75
  5%  sessionrestore_no_auto_restore linux64 opt e10s stylo335.08 -> 316.92

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12439

Comment 21

[Kanchan Kumari QA]

I have verified this bug fix on the latest nightly.

Comment 22

[Ciprian Georgiu, Desktop QA]

I've also managed to reproduce this bug using an affected Nightly build (2018-03-22) and following the STR from comment 0.

This is verified fixed on Beta 61.0b10 (20180530184300) running Windows 10, 7 x64 and Ubuntu 16.04 x64.