Last Comment Bug 604289 - Content still can steal focus from chrome by window.focus();
: Content still can steal focus from chrome by window.focus();
Status: ASSIGNED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 7 votes (vote)
: ---
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
data:text/html,<body onload="setTimeo...
Depends on: 591890 612251
Blocks: 656026 799413 125282 618907
  Show dependency treegraph
 
Reported: 2010-10-13 22:09 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2013-07-04 08:50 PDT (History)
11 users (show)
masayuki: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.x+
wanted


Attachments
Patch v0.1 (9.73 KB, patch)
2010-10-13 22:09 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch v0.2 (8.72 KB, patch)
2010-10-14 03:20 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
one testcase (315 bytes, text/html)
2010-10-14 06:14 PDT, Olli Pettay [:smaug]
no flags Details
Patch v0.3 (8.19 KB, patch)
2010-10-14 22:24 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch v1.0 (8.65 KB, patch)
2010-11-15 04:57 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-13 22:09:48 PDT
Created attachment 483079 [details] [diff] [review]
Patch v0.1

> data:text/html,<body onload="setTimeout(function () { window.focus(); document.getElementById('i').focus(); }, 3000);"><input id="i">

Like this, window.focus() can steal focus from chrome.

Here is a draft patch. I'll clean up it after bug 591890.
Comment 1 Olli Pettay [:smaug] 2010-10-14 02:12:33 PDT
Comment on attachment 483079 [details] [diff] [review]
Patch v0.1


>+PRBool
>+nsFocusManager::AllowsToBeStolenFocusBy(nsPIDOMWindow* aNewFocusWindow)
>+{
>+  NS_ENSURE_TRUE(aNewFocusWindow, PR_FALSE);
>+
>+  // When the following conditions are true:
>+  //  * an element has focus
>+  //  * isn't called by trusted event (i.e., called by untrusted event or by js)
>+  //  * the focus is moved to a different window
>+  // then, we need to check the permission.
>+
>+  if (!mFocusedContent ||
>+      mFocusedContent->GetOwnerDoc()->GetWindow() == aNewFocusWindow) {
>+    return PR_TRUE;
>+  }

I'm not quite sure I understand this. What if no element is focused, but only window?
Shouldn't you use mFocusedWindow?
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-14 02:47:22 PDT
> I'm not quite sure I understand this. What if no element is focused, but only
> window?
> Shouldn't you use mFocusedWindow?

Doesn't that break focus() methods excessively? I'll try to create such patch, wait a couple of hours.
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-14 03:20:24 PDT
Created attachment 483126 [details] [diff] [review]
Patch v0.2

posted to tryserver.
Comment 4 Olli Pettay [:smaug] 2010-10-14 04:02:14 PDT
Comment on attachment 483126 [details] [diff] [review]
Patch v0.2


>+PRBool
>+nsFocusManager::AllowsToBeStolenFocusBy(nsPIDOMWindow* aNewFocusWindow)
>+{
>+  // When the following conditions are true:
>+  //  * an element has focus
>+  //  * isn't called by trusted event (i.e., called by untrusted event or by js)
>+  //  * the focus is moved to a different window
>+  // then, we need to check the permission.
>+
>+  // If there is a focused window, caller should be able to access the window
>+  // when it can steal focus from the window.
>+  if (mFocusedWindow != aNewFocusWindow &&
>+      !nsContentUtils::CanCallerAccess(mFocusedWindow)) {
Just wondering. Do we really want to be this strict.
Would it be enough to do this check only if mFocusedWindow is a chrome window?
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-14 04:10:07 PDT
Should a web site be able to steal focus from another web site?
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-14 04:18:40 PDT
And I'm not sure the mFocusedContent check is needed or not after the check for mFocusedWindow.
Comment 7 Olli Pettay [:smaug] 2010-10-14 04:56:38 PDT
(In reply to comment #5)
> Should a web site be able to steal focus from another web site?
Yeah, this is the question.
Better to test how current browsers work.
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-14 05:52:36 PDT
Looks google chrome doesn't steal focus from another site.

I opened two windows. One is:

> data:text/html,<body onload="setTimeout(function () { window.focus(); document.getElementById('i').focus(); }, 10000);"><input id="i">

The other is google top page.

If I focus to google top page's window, the data page cannot steal focus.
Comment 9 Olli Pettay [:smaug] 2010-10-14 05:56:19 PDT
But can an iframe steal focus from the window.parent? That is IMO
more interesting case.
Comment 10 Olli Pettay [:smaug] 2010-10-14 05:59:53 PDT
And what does IE do?
Chrome has a bit strange behavior in some cases.
Comment 11 Olli Pettay [:smaug] 2010-10-14 06:14:41 PDT
Created attachment 483141 [details]
one testcase
Comment 12 Olli Pettay [:smaug] 2010-10-14 06:18:15 PDT
The testcase shows a regression.
FF3.6, Opera and Chrome, at least, do allow moving focus. FF4 doesn't.
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-14 07:55:45 PDT
How about these behavior?

* chrome can steal focus from everyone
* content cannot steal focus from chrome
* content can steal focus from its ancestor or descendant content window
* content can steal focus from its accessible content window
Comment 14 Olli Pettay [:smaug] 2010-10-14 08:10:20 PDT
Sounds reasonable.
Jst or Jonas may have something to say here.
Comment 15 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-14 22:24:13 PDT
Created attachment 483391 [details] [diff] [review]
Patch v0.3

I'm not sure whether I can write iframe test for third condition of comment 13...
Comment 16 Johnny Stenback (:jst, jst@mozilla.com) 2010-10-21 09:44:06 PDT
The behavior described in comment 13 sounds reasonable to me. And blocking on this.
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-11-15 04:57:28 PST
Created attachment 490548 [details] [diff] [review]
Patch v1.0

nsContentUtils::GetDocumentFromCaller() crashes during running browser_focus_steal_from_chrome.js...
Comment 18 Olli Pettay [:smaug] 2010-12-02 03:16:25 PST
Ah, hmm, could you not use nsContentUtils::GetDocumentFromCaller(), but
subject principal from security manager? Would that work?
Then do the comparison using nsIPrincipal::Subsumes.

(The crash is still a valid bug, IMHO. nsContentUtils::GetDocumentFromCaller() should just return null in that case.)
Comment 19 Olli Pettay [:smaug] 2010-12-02 03:17:26 PST
Hmm, no, you really need the document...
Comment 20 Olli Pettay [:smaug] 2010-12-02 06:08:10 PST
Does GetDocumentFromContext() fail too?
Comment 21 Olli Pettay [:smaug] 2010-12-02 06:26:36 PST
Or if nothing else works, perhaps we need to prevent few methods 
to be quickstubbed.
Comment 22 Olli Pettay [:smaug] 2010-12-02 06:29:17 PST
Like 'nsIDOMHTMLInputElement.select' in dom_quickstubs.qsconf, perhaps
also other selection related methods.
Maybe there aren't that many problematic cases after all.
Comment 23 Peter Van der Beken [:peterv] - away till Aug 1st 2010-12-02 10:49:06 PST
(In reply to comment #18)
> (The crash is still a valid bug, IMHO. nsContentUtils::GetDocumentFromCaller()
> should just return null in that case.)

I think I disagree (and the documentation disagrees with the implementation, see also the assert in GetDocumentFromCaller).

(In reply to comment #22)
> Like 'nsIDOMHTMLInputElement.select' in dom_quickstubs.qsconf, perhaps
> also other selection related methods.
> Maybe there aren't that many problematic cases after all.

If you're whitelisting, why can't you get the right document in the C++ callers anyway? nsHTMLInputElement::Select has easy access to the ownerDocument, which should be equivalent to GetDocumentFromCaller, no?
Comment 24 Olli Pettay [:smaug] 2010-12-02 11:18:28 PST
Chrome should be able to call somecontentobject.focus(). In that case
we need to detect that it is chrome which is calling.
Comment 25 Peter Van der Beken [:peterv] - away till Aug 1st 2010-12-02 11:51:54 PST
Sure, but why would that need a document to check for chrome?
Comment 26 Olli Pettay [:smaug] 2010-12-02 12:11:03 PST
Doesn't need to be document, principal would be ok too.
Hmm, can we always get the subject principal?
Security manager does use jsstack.


For nsContentUtils::ContentIsCrossDocDescendantOf calls we could use the
to-be-focused content object.
Comment 27 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-12-02 20:03:14 PST
(In reply to comment #20)
> Does GetDocumentFromContext() fail too?

No, GetDocumentFromContext() works fine (doesn't return NULL). And please note that my patch doesn't call GetDocumentFromCaller() when the caller is chrome. If the caller is chrome, it can always steal focus. Therefore, we can return from the method early.
Comment 28 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-12-02 20:23:42 PST
If a sub-window's function which tries to move focus is kicked by opener window's javascript, should it succeed to steal focus? If so, we should use GetDocumentFromContext() rather than GetDocumentFromCaller()?
Comment 29 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-12-02 23:24:59 PST
If site A has an iframe and site B is loaded in the iframe. Furthermore, B opens a new window which loads a B's page this is called as C. Then, there are 4 complex cases when A has focus.

1. B calls C's function which calls focus() of C.
2. B calls C's function which calls focus() of B.
3. C calls B's function which calls focus() of B.
4. C calls B's function which calls focus() of C.

If only 1 and 2 should succeed, GetDocumentFromContext() is better. Which cases should succeed?
Comment 30 Olli Pettay [:smaug] 2010-12-03 08:40:03 PST
Yeah, I think GetDocumentFromContext() makes more sense than GetDocumentFromCaller(), but I'm still worried that does it really work in
cases like bug 612251. Does it really return some reasonable document?
Or do we still need to change dom_quickstubs.qsconf - as far as I understand, 
we do need to change that.
Comment 31 Jonas Sicking (:sicking) PTO Until July 5th 2010-12-14 10:32:59 PST
We'd really like to get this fixed, and if a patch shows up we should take it (assuming its safe enough etc). But given that this behavior has been around for a while we can't block on this.
Comment 32 Joe Langeway 2011-03-23 14:58:16 PDT
Some interesting questions here for sure, but I would like to plead that at least one aspect of this problem be fixed.

Just don't let javascript in a web page steal the focus from the address bar. Just fix that. 

It is not possible to implement good keyboard navigation support that uses the native focus mechanism until you fix this. If the user clicks some whitespace on the page, it causes the focused element to lose focus, I want that element to grab it back so that the keyboard continues to work as intended, but this case is indistinguishable from the user clicking in the address bar, so I steal the focus from the address bar! When could it possibly be correct that the page can do this?
Comment 33 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-23 18:27:50 PDT
(In reply to comment #32)
> It is not possible to implement good keyboard navigation support that uses the
> native focus mechanism until you fix this. If the user clicks some whitespace
> on the page, it causes the focused element to lose focus, I want that element
> to grab it back so that the keyboard continues to work as intended, but this
> case is indistinguishable from the user clicking in the address bar, so I steal
> the focus from the address bar! When could it possibly be correct that the page
> can do this?

When user clicks an unfocusable element, its window will get focus. Therefore, the previous focused element lost it.
Comment 34 Joe Langeway 2011-03-24 09:36:17 PDT
Thank you, it did not occur to me that I was listening for focus events at the document and below but not at the window.

Still, if I put one button and this javascript in a web page, the user can not use the address bar or search box:

setInterval(function(){document.getElementsByTagName('button')[0].focus();},100);

It doesn't seem that a web page should be able to do that, and it can't do that in any other browser.

Note You need to log in before you can comment on or make changes to this bug.