1166066 - Firefox 38 regression: link in private window with target="_blank" opens window with no tabs/toolbars/menus

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Sören Hentzschel]

This bug sounds similar to bug 1114299, but there is a difference: in this scenario e10s is not involved, but the private mode. The same testcase can be used to reproduce, the result is the same.

It's a regression in Firefox 38, Firefox 37 works as expected.

STR:

1. make sure "Open new windows in a new tab instead" is *not* checked
2. open the testcase from bug 1114299 in a private window
3. click the link

ER:

A new browser window with toolbar items, tabstrip and scrollbar opens. 

AR:

A new browser window opens, but it has no tabstrip and no toolbar items and it's not possible to scroll.

Comment 1

[Sören Hentzschel]

Last good revision: 38e4719e71af (2015-01-27)
First bad revision: 9b6b80222e66 (2015-01-28)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=38e4719e71af&tochange=9b6b80222e66

Comment 2

[[:philipp]]

It looks like bug 1108547 caused the regression

Comment 3

[:Gijs (he/him)]

(In reply to Sören Hentzschel from comment #0)
> This bug sounds similar to bug 1114299, but there is a difference: in this
> scenario e10s is not involved, but the private mode. The same testcase can
> be used to reproduce, the result is the same.
> 
> It's a regression in Firefox 38, Firefox 37 works as expected.
> 
> STR:
> 
> 1. make sure "Open new windows in a new tab instead" is *not* checked
> 2. open the testcase from bug 1114299 in a private window
> 3. click the link
> 
> ER:
> 
> A new browser window with toolbar items, tabstrip and scrollbar opens. 


Does this only happen if you set the browser.link.open_newwindow pref to a non-default value (namely, open _blank links in windows instead of tabs) ? Because I cannot reproduce with the steps as-is.

Comment 4

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Serious functional regression if you prefer windows over tabs and use private browsing mode.


Just checked, and I can reproduce with the pref flipped. Probably not worth the risk for 38.0.5 unless the fix is very simple indeed. Going to request tracking for 39 but I'll look into it right now.

Sören, keeping the needinfo because now I'm wondering if this is reproducible in permanent private browsing mode (selecting "never remember history" in the prefs). Do you know?

Comment 5

[:Gijs (he/him)]

diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
+            nsAutoString features;
+            if (mInPrivateBrowsing) {
+              features.AssignLiteral("private");
+            }
             rv = win->OpenNoNavigate(NS_ConvertUTF8toUTF16(spec),
                                      name,          // window name
-                                     EmptyString(), // Features
+                                     features,

Yeah, except "" has meaning, which gets nuked if you supply a non-empty string, so just assigning "private" doesn't keep the rest of the features non-private windows get here.

Comment 6

[Sören Hentzschel]

(In reply to :Gijs Kruitbosch from comment #4) 
> Sören, keeping the needinfo because now I'm wondering if this is
> reproducible in permanent private browsing mode (selecting "never remember
> history" in the prefs). Do you know?

Yes, it's also reproducible in permanent private browsing mode.

Comment 8

[:Gijs (he/him)]

Attachment: MozReview Request: bz://1166066/Gijs

/r/9055 - Bug 1166066 - fix opening new windows from a private window, r?bholley

Pull down this commit:

hg pull -r a692a04003880ba61a128f830dea0dfa68f76595 https://reviewboard-hg.mozilla.org/gecko/

Comment 9

[:Gijs (he/him)]

I'm not particularly thrilled with this patch, but it seems there's a lot of hairiness here, so I decided to cut through some of the hairiness.

The principal issue is that windowwatcher itself decides whether the window you're opening is a dialog, irrespective of what you've told it (bug 779939). It does this based on the argc of the call (ie length of the 4th param of window.open in JS, AFAICT). While we could add fake arguments to work around this, that would be lame and probably be web-visible and sadfaces.

The other thing is that if the browser thinks you're not a dialog, you don't get to use the "chrome" or "all" flags or really any other useful stuff. And really what we want here is that just passing "private" behaves the same as "" except that the window should be private. So that's what I've tried to implement here.

If there's better alternatives, I'd love to hear about them.

Try push from mozreview: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec3577673ac2

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8607577 [details]
MozReview Request: bz://1166066/Gijs

I'm not really familiar with stuff. Looks like Josh reviewed the origin patch - josh, do you feel comfortable reviewing this? Could also try bobowen.

Comment 11

[Josh Matthews [:jdm]]

Comment on attachment 8607577 [details]
MozReview Request: bz://1166066/Gijs

https://reviewboard.mozilla.org/r/9053/#review7829

Ship It!

Comment 12

[Josh Matthews [:jdm]]

I don't have any better solutions; comment 9 sums up my feelings about it quite well.

Comment 13

[:Gijs (he/him)]

burning tree and cantina to follow -> ci-needed.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9b5b1229a5c4

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9b5b1229a5c4

Comment 16

[[:philipp]]

is there any possibility to uplift this? it's a regression that is quite visible to users with the affected settings & therefore comes up quite often in support: https://support.mozilla.org/en-US/questions/firefox?tagged=bug1166066&show=all

Comment 17

[:Gijs (he/him)]

Comment on attachment 8607577 [details]
MozReview Request: bz://1166066/Gijs

Approval Request Comment
[Feature/regressing bug #]: bug 1108547
[User impact if declined]: broken window chrome on window.open calls from private windows / in permanent private browsing mode, iff the "open new windows as new tabs" option is disabled by the user.
[Describe test coverage new/current, TreeHerder]: sadly, no tests
[Risks and why]: medium-low. This should not change any behaviour except this particular case. AFAICT almost any other case goes through different codepaths anyway, and the change is fairly specific to the bug in question. Would have been just "very low" if it weren't for the lack of automated testing. Would recommend increased manual QA.
[String/UUID change made/needed]: no.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8607577 [details]
MozReview Request: bz://1166066/Gijs

I am taking it to aurora to improve the testing coverage.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for uplift.

Comment 20

[:Gijs (he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=9b7747061d33

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9b7747061d33

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Florin can your team test this fix on aurora?  If it seems ok there and hasn't caused obvious problems then I'd like to uplift this for beta 4.

Comment 23

[jen strickland :jenstrickland]

Tracked for 39.

Comment 24

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I reproduced the original issue with Firefox 38 and older Dev Edition build, as specified in comment 0, with both New Private Browsing Window and Permanent Private Browsing mode.

The issue no longer reproduces with the latest Firefox 40 Aurora, on Windows 7 x64, Mac OS X 10.8.5, and Ubuntu 14.04 x86. The new window that opens now correctly displays tabs/toolbars/menus, also buttons functionality and navigation all seem to work fine in the new window.

Comment 25

[Loic]

Reported on the French community board: https://forums.mozfr.org/viewtopic.php?f=5&t=124487
Would be nice to have a backport into FF39 too.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8607577 [details]
MozReview Request: bz://1166066/Gijs

Approved for uplift to beta.  This has had extra manual testing work on aurora; fixes a regression in private browsing.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a7f385942c76

Comment 29

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1166066 - fix opening new windows from a private window, r?bholley

Comment 30

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I have also verified this with Firefox 39 Beta 5, on Windows 7 x64, Mac OS X 10.8.5, and Ubuntu 14.0.4 x64. I got the same results as in comment 24.

Comment 31

[[:philipp]]

[Tracking Requested - why for this release]:
will this fix be considered for the esr 38 cycle as well?

Comment 32

[:Gijs (he/him)]

Bob, Josh is on PTO for the next while. I'd like a second opinion about whether this is suitable for esr or not, considering risk/reward. The patch is self-contained but this code is not my usual habitat... Let me know if you need more detail beyond the bug comments / patch so far.

Comment 33

[Bob Owen (:bobowen)]

Wow, my head did not need that on a Friday morning. :-)

I'm not sure I'm the right person to suggest what gets into ESR.

However, having said that it seems like quite a bad regression for people who use that setting and given your stated aim:
'And really what we want here is that just passing "private" behaves the same as "" except that the window should be private.'

The patch seems relatively straight forward (if you ignore all the craziness around it).

Comment 34

[:Gijs (he/him)]

Comment on attachment 8620316 [details]
MozReview Request: Bug 1166066 - fix opening new windows from a private window, r?bholley

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: serious functional regression for people using private browsing / permanent private browsing (which seems plausible for e.g. schools and other enterprises where multiple people share computers that need to live without history etc.) if they prefer windows over tabs (which seems like it'd correlate with ESR usage...)
User impact if declined: _blank links in private browsing windows are broken
Fix Landed on Version: 41, uplifted to 39, already verified there and on 40.
Risk to taking this patch (and alternatives if risky): low-risk; as comment 33 notes, this is fairly straightforward considering the actual change we made.
String or UUID changes made by this patch: nope.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 35

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8620316 [details]
MozReview Request: Bug 1166066 - fix opening new windows from a private window, r?bholley

We can also take stability/important fixes for the two first ESR. I think this is the case here. Taking it.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/5e08ce860c9f

Comment 37

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I have also verified this fix with Firefox ESR 38.1.0 (BuildID=20150624141534), on Windows 7 x64, Mac OS X 10.9.5, and Ubuntu 12.0.4 x86. I got the same results as in comment 24.