Closed Bug 1114299 Opened 9 years ago Closed 9 years ago

[e10s] link with target="_blank" opens window with no tabs/toolbars/menus

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 42
Tracking Status
e10s m8+ ---
firefox42 --- fixed

People

(Reporter: jst, Assigned: mconley)

References

Details

(Whiteboard: [STR in comment 5])

Attachments

(5 files, 1 obsolete file)

Attached file Testcase
With e10s enabled a link with target="_blank" opens a new window but that window has no chrome features, i.e. no tabs, no toolbars, no menus, etc. See testcase.
Keywords: regression
How sure are you that this is a regression, rather than just an e10s problem? I tried mozregression, at least the 11-26 nightly reproduces this, so if it /is/ a regression, it's not terribly recent and 36 is probably affected, too. :-(

(reproducible on Windows, so setting all/all)
Blocks: e10s
tracking-e10s: --- → ?
Flags: needinfo?(jst)
OS: Linux → All
Hardware: x86_64 → All
This reproduces in July, and in May I can't get it to honor the "open links in windows rather than tabs" prefs in e10s mode, so I'm going to clear the 'regression' flag.

That said, we should obviously fix this... :-(
Flags: needinfo?(jst)
Keywords: regression
Oh, good question... I'm not convinced this is a regression from any previous point in time with e10s enabled, but it is, as you noticed as well, a regression from non-e10s behavior.
Mike, I feel like we have a related bug filed and you're assigned to it. How closely does my memory reflect reality?
Flags: needinfo?(mconley)
You might be thinking of bug 1047603, which also involves target="_blank" links, but is (I believe) caused by different problems.

This sounds more like a foul-up in how we implemented the fix in bug 989501.

STR:

1) In Preferences (General pane), in the "Tabs" section, make sure "Open new windows in a new tab instead" is _not_ checked
2) Click on the test case attachment in this bug
3) Click on the link in the test case

ER:

A new, full browser window should open.

AR:

A window opens, but it has no tabstrip or toolbar items. It looks like a popup window instead.
Flags: needinfo?(mconley)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
Still happens here... Reopening.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Flags: needinfo?(mconley)
Ditto.
Flags: needinfo?(mconley)
Whiteboard: [STR in comment 5]
Assignee: nobody → mconley
oh, sorry, I guess I misread the STR :) my bad.
The problem here appears that nsWindowWatcher::CalculateChromeFlags is not prepared to handle the case where only ",remote" or ",non-remote" are passed as features.

Note that when ",private" is passed in by itself, we have code at the start of CalculateChromeFlags to detect this, and automatically set chromeFlags to nsIWebBrowserChrome::CHROME_ALL, which gives the window all of the default features.

I figure we want to do something similar when aFeatures equals any of:

"private"
"remote"
"non-remote"
"private,remote"
"remote,private"
"private,non-remote"
"non-remote,private"

So I think we need to put some logic somewhere in CalculateChromeFlags to detect these cases. Kinda gross, but there you have it.
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #11)
> The problem here appears that nsWindowWatcher::CalculateChromeFlags is not
> prepared to handle the case where only ",remote" or ",non-remote" are passed
> as features.
> 
> Note that when ",private" is passed in by itself, we have code at the start
> of CalculateChromeFlags to detect this, and automatically set chromeFlags to
> nsIWebBrowserChrome::CHROME_ALL, which gives the window all of the default
> features.
> 
> I figure we want to do something similar when aFeatures equals any of:
> 
> "private"
> "remote"
> "non-remote"
> "private,remote"
> "remote,private"
> "private,non-remote"
> "non-remote,private"
> 
> So I think we need to put some logic somewhere in CalculateChromeFlags to
> detect these cases. Kinda gross, but there you have it.

Gaaaah. So this is basically exactly the same as bug 1166066.

This code really really really needs a rewrite (see also other comments in aforementioned bug), but I feel bad suggesting we do it here seeing as I fixed the other bug with a "hack" too (that's the explicit check for "private" there). Don't want to hold this up.

OOI, why are we needing to pass this at all with e10s the default? Feels like we shouldn't need to pass it as a feature flag in that case, can just check the pref and the Right Thing should happen?
(In reply to :Gijs Kruitbosch from comment #12)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #11)
> OOI, why are we needing to pass this at all with e10s the default? Feels
> like we shouldn't need to pass it as a feature flag in that case, can just
> check the pref and the Right Thing should happen?

The only reason to support the "remote" feature for window opening, as far as I can tell, is so that we can open e10s windows in non-e10s mode for various tests.

We do want to support non-remote at least for a little while, since that's what allows us to open non-e10s windows while in e10s mode.
Bug 1114299 - Make it so that certain features passed to window.open imply all window chrome by default. r?Gijs
Not requesting review yet, since I'm waiting on a try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b3f9e650a80

I'll also want to add some regression tests, since this stuff is so touchy.
The linked patch does fix the testcase here!
Notes:

1) there are suspicious-looking e10s-bc3 failures
2) I would suggest limiting this to altering the conditions for the early return at the top of the function in order to reduce complexity of the patch and possible side-effects on stuff we don't want to touch
3) AFAICT from looking quickly (maybe I missed something!) your refactoring means we're no longer checking isCallerChrome before we allow something to open with chrome features. I expect we should keep that check.
(also, notwithstanding comment #17, I am not a good reviewer for this patch. Try :bobowen or :josh . :-) )
Comment on attachment 8627414 [details]
MozReview Request: Bug 1114299 - Be more strict about which window chromeFlags we compute from content. r?smaug.

Bug 1114299 - Make it so that certain features passed to window.open imply all window chrome by default.
Attachment #8627414 - Attachment description: MozReview Request: Bug 1114299 - Make it so that certain features passed to window.open imply all window chrome by default. r?Gijs → MozReview Request: Bug 1114299 - Make it so that certain features passed to window.open imply all window chrome by default.
I've spent a few days spelunking through our window opening code. I should hopefully have a more correct patch up today.
Comment on attachment 8627414 [details]
MozReview Request: Bug 1114299 - Be more strict about which window chromeFlags we compute from content. r?smaug.

Bug 1114299 - Make it so that certain features passed to window.open imply all window chrome by default.
Corrective experiment
Comment on attachment 8627414 [details]
MozReview Request: Bug 1114299 - Be more strict about which window chromeFlags we compute from content. r?smaug.

Bug 1114299 - Be more strict about which window chromeFlags we compute from content.
Attachment #8627414 - Attachment description: MozReview Request: Bug 1114299 - Make it so that certain features passed to window.open imply all window chrome by default. → MozReview Request: Bug 1114299 - Be more strict about which window chromeFlags we compute from content.
Attachment #8632187 - Attachment is obsolete: true
Comment on attachment 8627414 [details]
MozReview Request: Bug 1114299 - Be more strict about which window chromeFlags we compute from content. r?smaug.

Bug 1114299 - Be more strict about which window chromeFlags we compute from content.
Comment on attachment 8627414 [details]
MozReview Request: Bug 1114299 - Be more strict about which window chromeFlags we compute from content. r?smaug.

Bug 1114299 - Be more strict about which window chromeFlags we compute from content.
https://reviewboard.mozilla.org/r/12233/#review12131

::: dom/ipc/TabChild.cpp:1528
(Diff revision 6)
>  
> +  // The content process should never be in charge of computing whether or
> +  // not a window should be private or remote - the parent will do that.
> +  MOZ_ASSERT(!(aChromeFlags & nsIWebBrowserChrome::CHROME_PRIVATE_WINDOW));
> +  MOZ_ASSERT(!(aChromeFlags & nsIWebBrowserChrome::CHROME_NON_PRIVATE_WINDOW));
> +  MOZ_ASSERT(!(aChromeFlags & nsIWebBrowserChrome::CHROME_PRIVATE_LIFETIME));
> +  MOZ_ASSERT(!(aChromeFlags & nsIWebBrowserChrome::CHROME_REMOTE_WINDOW));

We should probably move these assertions into TabParent - I think the general principal is that we should always assume that the content process is potentially compromised.

::: dom/ipc/PBrowser.ipdl:166
(Diff revision 6)
> -                      nsString aFeatures,
> +                      nsCString aFeatures,

Maybe split out the nsCString stuff into a separate commit.
Bug 1114299 - Pass window features up from the content process via nsCString. r?smaug

The code that deals with the features is looking for char *, and we were going through
the hassle of converting that char * to a UTF16 string, and then back down to UTF8,
and then to char *. Really, we just need to send up a nsCString, which simplifies things.
Comment on attachment 8627414 [details]
MozReview Request: Bug 1114299 - Be more strict about which window chromeFlags we compute from content. r?smaug.

Bug 1114299 - Be more strict about which window chromeFlags we compute from content. r?smaug.

We don't ever want to accept "private", "non-private", "remote" or "non-remote" from the
content process. We only let the parent decide when to open those types of windows.
Attachment #8627414 - Attachment description: MozReview Request: Bug 1114299 - Be more strict about which window chromeFlags we compute from content. → MozReview Request: Bug 1114299 - Be more strict about which window chromeFlags we compute from content. r?smaug.
Bug 1114299 - Regression test. r?bholley
Attachment #8635463 - Flags: review?(bobbyholley)
Comment on attachment 8635345 [details]
MozReview Request: Bug 1114299 - Pass window features up from the content process via nsCString. r?smaug

Bug 1114299 - Pass window features up from the content process via nsCString. r?smaug

The code that deals with the features is looking for char *, and we were going through
the hassle of converting that char * to a UTF16 string, and then back down to UTF8,
and then to char *. Really, we just need to send up a nsCString, which simplifies things.
Attachment #8635345 - Flags: review?(bugs)
Comment on attachment 8627414 [details]
MozReview Request: Bug 1114299 - Be more strict about which window chromeFlags we compute from content. r?smaug.

Bug 1114299 - Be more strict about which window chromeFlags we compute from content. r?smaug.

We don't ever want to accept "private", "non-private", "remote" or "non-remote" from the
content process. We only let the parent decide when to open those types of windows.
Attachment #8635345 - Flags: review?(bugs) → review+
Comment on attachment 8627414 [details]
MozReview Request: Bug 1114299 - Be more strict about which window chromeFlags we compute from content. r?smaug.

Bug 1114299 - Be more strict about which window chromeFlags we compute from content. r?smaug.

We don't ever want to accept "private", "non-private", "remote" or "non-remote" from the
content process. We only let the parent decide when to open those types of windows.
Attachment #8627414 - Flags: review?(bugs)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #27)
> https://reviewboard.mozilla.org/r/12233/#review12131
> 
> ::: dom/ipc/TabChild.cpp:1528
> (Diff revision 6)
> >  
> > +  // The content process should never be in charge of computing whether or
> > +  // not a window should be private or remote - the parent will do that.
> > +  MOZ_ASSERT(!(aChromeFlags & nsIWebBrowserChrome::CHROME_PRIVATE_WINDOW));
> > +  MOZ_ASSERT(!(aChromeFlags & nsIWebBrowserChrome::CHROME_NON_PRIVATE_WINDOW));
> > +  MOZ_ASSERT(!(aChromeFlags & nsIWebBrowserChrome::CHROME_PRIVATE_LIFETIME));
> > +  MOZ_ASSERT(!(aChromeFlags & nsIWebBrowserChrome::CHROME_REMOTE_WINDOW));
> 
> We should probably move these assertions into TabParent - I think the
> general principal is that we should always assume that the content process
> is potentially compromised.
Well, naturally just having assertions there doesn't help at all if child process is compromised unless one is using debug builds.
why does this not regress Bug 1108547?
This doesn't regress that, but I don't understand why.
Ahaa, it was Bug 1166066 which made the code a bit hard to follow.
Attachment #8627414 - Flags: review?(bugs) → review+
Comment on attachment 8635463 [details]
MozReview Request: Bug 1114299 - Regression test. r?bholley

Bug 1114299 - Regression test. r?bholley
Attachment #8635463 - Flags: review?(bobbyholley) → review?(bugs)
Attachment #8635463 - Flags: review?(bugs) → review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/ad10d2933a6cd7705a594bd64e806316690c0406
changeset:  ad10d2933a6cd7705a594bd64e806316690c0406
user:       Mike Conley <mconley@mozilla.com>
date:       Fri Jul 17 11:30:51 2015 -0400
description:
Bug 1114299 - Pass window features up from the content process via nsCString. r=smaug

The code that deals with the features is looking for char *, and we were going through
the hassle of converting that char * to a UTF16 string, and then back down to UTF8,
and then to char *. Really, we just need to send up a nsCString, which simplifies things.

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/3e8d94c73096d71029b71cb1d1466d0fa9fd29d8
changeset:  3e8d94c73096d71029b71cb1d1466d0fa9fd29d8
user:       Mike Conley <mconley@mozilla.com>
date:       Fri Jul 10 11:40:19 2015 -0400
description:
Bug 1114299 - Be more strict about which window chromeFlags we compute from content. r=smaug

We don't ever want to accept "private", "non-private", "remote" or "non-remote" from the
content process. We only let the parent decide when to open those types of windows.

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/310cb0314751018aad19ba6a21914af54b290d6c
changeset:  310cb0314751018aad19ba6a21914af54b290d6c
user:       Mike Conley <mconley@mozilla.com>
date:       Fri Jul 17 13:51:39 2015 -0400
description:
Bug 1114299 - Regression test. r=smaug
https://hg.mozilla.org/mozilla-central/rev/ad10d2933a6c
https://hg.mozilla.org/mozilla-central/rev/3e8d94c73096
https://hg.mozilla.org/mozilla-central/rev/310cb0314751
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Status: RESOLVED → VERIFIED
Depends on: 1189554
Depends on: 1196706
Depends on: 1222879
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: