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)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 42
People
(Reporter: jst, Assigned: mconley)
References
Details
(Whiteboard: [STR in comment 5])
Attachments
(5 files, 1 obsolete file)
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.
Reporter | ||
Updated•9 years ago
|
Keywords: regression
Comment 1•9 years ago
|
||
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)
Comment 2•9 years ago
|
||
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
Reporter | ||
Comment 3•9 years ago
|
||
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.
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
STR |
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)
Updated•9 years ago
|
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
Reporter | ||
Comment 7•9 years ago
|
||
Still happens here... Reopening.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(mconley)
I can reproduce this.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mconley
Comment 10•9 years ago
|
||
oh, sorry, I guess I misread the STR :) my bad.
Assignee | ||
Comment 11•9 years ago
|
||
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.
Comment 12•9 years ago
|
||
(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?
Assignee | ||
Comment 13•9 years ago
|
||
(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.
Assignee | ||
Comment 14•9 years ago
|
||
Bug 1114299 - Make it so that certain features passed to window.open imply all window chrome by default. r?Gijs
Assignee | ||
Comment 15•9 years ago
|
||
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.
Reporter | ||
Comment 16•9 years ago
|
||
The linked patch does fix the testcase here!
Comment 17•9 years ago
|
||
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.
Comment 18•9 years ago
|
||
(also, notwithstanding comment #17, I am not a good reviewer for this patch. Try :bobowen or :josh . :-) )
Assignee | ||
Comment 19•9 years ago
|
||
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.
Assignee | ||
Comment 20•9 years ago
|
||
I've spent a few days spelunking through our window opening code. I should hopefully have a more correct patch up today.
Assignee | ||
Comment 21•9 years ago
|
||
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.
Assignee | ||
Comment 22•9 years ago
|
||
Corrective experiment
Assignee | ||
Comment 23•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Attachment #8632187 -
Attachment is obsolete: true
Assignee | ||
Comment 24•9 years ago
|
||
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.
Assignee | ||
Comment 25•9 years ago
|
||
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.
Assignee | ||
Comment 26•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb9cc0d9969d
Assignee | ||
Comment 27•9 years ago
|
||
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.
Assignee | ||
Comment 28•9 years ago
|
||
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.
Assignee | ||
Comment 29•9 years ago
|
||
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.
Assignee | ||
Comment 30•9 years ago
|
||
Bug 1114299 - Regression test. r?bholley
Attachment #8635463 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 31•9 years ago
|
||
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)
Assignee | ||
Comment 32•9 years ago
|
||
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.
Updated•9 years ago
|
Attachment #8635345 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 33•9 years ago
|
||
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)
Comment 34•9 years ago
|
||
(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.
Comment 35•9 years ago
|
||
why does this not regress Bug 1108547?
Comment 36•9 years ago
|
||
This doesn't regress that, but I don't understand why.
Comment 37•9 years ago
|
||
Ahaa, it was Bug 1166066 which made the code a bit hard to follow.
Updated•9 years ago
|
Attachment #8627414 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 38•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8635463 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 39•9 years ago
|
||
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
Comment 40•9 years ago
|
||
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 ago → 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Assignee | ||
Comment 41•9 years ago
|
||
bugnotes |
http://people.mozilla.org/~mconley2/bugnotes/bug-1114299.html
Reporter | ||
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•