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

VERIFIED FIXED in Firefox 61

Status

()

defect
VERIFIED FIXED
Last year
Last year

People

(Reporter: alice0775, Assigned: florian)

Tracking

({regression})

61 Branch
Firefox 61
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 unaffected, firefox60 unaffected, firefox61 verified)

Details

(Whiteboard: [fxperf:p1])

Attachments

(2 attachments, 2 obsolete attachments)

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.
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?
Blocks: 1447719
Flags: needinfo?(florian)
In addition to the comment #0.
If HWA is enabled, white window is opened instead of black.
Summary: 2 browser windows are opened when click local html file. → 2 browser windows are opened when click local html file. one is white/black window without UI, the other one seems normal
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.
OS: Windows 10 → All
Screenshot Nightly running on VM(sorry it so slow)
Posted patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → florian
Status: NEW → ASSIGNED
Flags: needinfo?(florian)
Whiteboard: [fxperf:p1]
Duplicate of this bug: 1448530
Duplicate of this bug: 1448561
Duplicate of this bug: 1448678
Posted patch Patch v2 (obsolete) — Splinter Review
This turned out to require a pretty significant refactoring of the nsBrowserContentHandler.js file, but it's nicely removing code duplication :-).
Attachment #8963271 - Flags: review?(mconley)
Attachment #8961699 - Attachment is obsolete: true
Posted patch Patch v3Splinter Review
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.
Attachment #8963308 - Flags: review?(mconley)
Attachment #8963271 - Attachment is obsolete: true
Attachment #8963271 - Flags: review?(mconley)
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.
(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 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)
Attachment #8963308 - Flags: review?(mconley) → review+
(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.
Also, I'm tempted to entirely eliminate this "browser.chromeURL" pref, but I figured it was way out of the scope of this bug.
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4c46aab1b2f
re-use the blank window whenever possible in nsBrowserContentHandler.js, r=mconley.
Depends on: 1450267
https://hg.mozilla.org/mozilla-central/rev/e4c46aab1b2f
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Duplicate of this bug: 1450408
Duplicate of this bug: 1450822
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
I have verified this bug fix on the latest nightly.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: ciprian.georgiu
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.