Closed Bug 1166066 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: General, defect)

38 Branch
defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 41
Iteration:
41.1 - May 25
Tracking Status
firefox38 --- wontfix
firefox39 + verified
firefox38.0.5 --- wontfix
firefox40 --- verified
firefox41 --- fixed
firefox-esr38 39+ verified
relnote-firefox --- 39+

People

(Reporter: soeren.hentzschel, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
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
It looks like bug 1108547 caused the regression
Blocks: 1108547
(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.
Flags: needinfo?(cadeyrn)
[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?
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 41.1 - May 25
Points: --- → 3
Flags: qe-verify+
Flags: in-testsuite?
Flags: firefox-backlog+
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.
(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.
Flags: needinfo?(cadeyrn)
Duplicate of this bug: 1166255
Attached file MozReview Request: bz://1166066/Gijs (obsolete) —
/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/
Attachment #8607577 - Flags: review?(bobbyholley)
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 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.
Attachment #8607577 - Flags: review?(bobbyholley) → review?(josh)
Attachment #8607577 - Flags: review?(josh) → review+
Comment on attachment 8607577 [details]
MozReview Request: bz://1166066/Gijs

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

Ship It!
I don't have any better solutions; comment 9 sums up my feelings about it quite well.
burning tree and cantina to follow -> ci-needed.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9b5b1229a5c4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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 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.
Attachment #8607577 - Flags: approval-mozilla-beta?
Attachment #8607577 - Flags: approval-mozilla-aurora?
Comment on attachment 8607577 [details]
MozReview Request: bz://1166066/Gijs

I am taking it to aurora to improve the testing coverage.
Attachment #8607577 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs rebasing for uplift.
Flags: needinfo?(gijskruitbosch+bugs)
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.
Flags: needinfo?(florin.mezei)
Tracked for 39.
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.
Flags: needinfo?(florin.mezei)
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 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.
Attachment #8607577 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8607577 - Attachment is obsolete: true
Attachment #8620316 - Flags: review+
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.
Flags: qe-verify+
[Tracking Requested - why for this release]:
will this fix be considered for the esr 38 cycle as well?
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.
Flags: needinfo?(bobowen.code)
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).
Flags: needinfo?(bobowen.code)
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.
Attachment #8620316 - Flags: approval-mozilla-esr38?
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.
Attachment #8620316 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
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.
Duplicate of this bug: 1182237
You need to log in before you can comment on or make changes to this bug.