125282 - Webpage-JS can steal focus from URLbar / chrome

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Gili Tzabari]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:0.9.8) Gecko/20020204
BuildID:    2002020417

If I hit CTRL-L while www.google.com is loading it shouldn't be able to steal
the cursor focus away from the location bar.

Reproducible: Always
Steps to Reproduce:
1. Visit www.google.com while the cursor is in the location bar
2.
3.

Comment 1

[Peter Trudelle]

Do you mean the caret?  That's what Ctrl-L does.  This seems invalid.

Comment 2

[Gili Tzabari]

I think you misunderstand. I am complaining that when the caret is in the
location field (CTRL-L, etc) and google.com loads up, its Javascript steals the
caret away from the location field into the page. The Javascript code shouldn't
be allowed to steal the caret if it's in the location field.

Comment 3

[Peter Trudelle]

->bryner for triage, cc aaronl. 

Comment 4

[Tom Mraz]

Very similar is bug 124750.
Seeing on Linux 2002030414 m0.9.9branch build

Comment 5

[sairuh (rarely reading bugmail)]

urlbar

Comment 6

[Peter Trudelle]

This doesn't seem like an URL bar problem so much as a focus problem. cc bryner.
 Dup?

Comment 7

[Boris Zbarsky [:bzbarsky]]

*** Bug 143973 has been marked as a duplicate of this bug. ***

Comment 8

[Neil Marshall]

URL From Bug 143973
http://www.episode-x.com/episode2/index.php#685

Comment 9

[Jesse Ruderman]

*** Bug 140524 has been marked as a duplicate of this bug. ***

Comment 10

[Jesse Ruderman]

*** Bug 230648 has been marked as a duplicate of this bug. ***

Comment 11

[Gili Tzabari]

This bug is almost 2 years old. Is anyone following up on it?

Comment 12

[graamone]

The search bar in Firefox, and using multiple tabs as well as multiple instances
of the browser are also effected by this problem.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

*** Bug 271565 has been marked as a duplicate of this bug. ***

Comment 14

[Brian 'netdragon' Bober]

Google.com loads pretty quickly for testing this. Slower loading testcase coming up.

Comment 15

[Brian 'netdragon' Bober]

Attachment: slower loading testcase

Just start slowly typing in the URL bar as the page loads, and watch the focus
get stolen by the Google form.

Perhaps a solution should be when a page sets focus, to make the next tabstop
the form the page sets focus to.

Comment 16

[Gili Tzabari]

+1

I like Brian's idea of redirecting focus requests to he next tabstop.

Comment 17

[Vaclav Dvorak]

As much as I hate focus stealing, this is definitely not a "major loss of function".

Comment 18

[Aaron Leventhal]

*** Bug 229007 has been marked as a duplicate of this bug. ***

Comment 19

[Phil Ringnalda (:philor)]

*** Bug 274747 has been marked as a duplicate of this bug. ***

Comment 20

[Dave Townsend [:mossop]]

*** Bug 302671 has been marked as a duplicate of this bug. ***

Comment 21

[Robert Claypool]

I posted bug 302671 marked duplicate for this bug and pointed out that forms
should not be allowed to steal focus while typing. There are some times where
you want the forms to steal focus. If I were to type google.com into the url
box, that would be the behavior I would expect.

Comment 22

[Gili Tzabari]

I disagree. When I first filed this issue 3 years ago my original complaint was
that if Google.com finishes loading while I am in the middle of entering a new
URL in the location bar, it should not steal the focus away.

Specifically, if my browser homepage was google and I started typing in the
location bar immediately after loading the browser and before Google requested
keyboard focus, then the browser should not allow it to steal the focus.

Simple rule: if the user began doing something after the page began loading but
before it finished loading, then the user is deemed to be "busy" and the website
should not be allowed to steal focus. This is exactly what happens if you open a
new window in Windows and shift focus away before it actually pops up. When it
actually pops up, its focus request will be rejected and instead you'll get a
flashing indicator in your taskbar. This is exactly the kind of behavior I'm
voting for.

Comment 23

[Robert Claypool]

I think you misunderstood me. When I'm finished typing google.com in the URL 
bar and hit enter, I expect google to steal the focus. otherwise the caret 
should stay in the location bar, but a quick test of firefox shows that focus 
is transferred to the page regardless. IE has the same behavior. Rats!

Comment 24

[Kevin Brosnan]

*** Bug 317643 has been marked as a duplicate of this bug. ***

Comment 25

[djstealth]

A simple solution to this would be as follows:

Detect current typing, and do not change focus until typing is complete.
(For example, prior to changing focus, if there were more than 2 keypresses in
the last second, abort [or delay] the focus change; otherwise, change focus)


NOTE:
This happens in other occurances of data entry, potentially (worst-case) I
could begin typing in a password somewhere, and would finish typing this in, in
a clear-text box, and accidentally submit this somewhere.

Comment 26

[Mikel Ward]

I think Epiphany <http://www.gnome.org/projects/epiphany/> has solved this problem.  Maybe we can see how they did it.

Comment 27

[Jonathan Amir]

In reply to comment 23: hitting enter is not the same as continuously typing. When you hit enter you are telling the browser "I am done with what I was typing, so take me there", and you are relinquishing your focus. When you are typing, you are busy.

I think that bug 359549 is a duplicate of this one. It happens to me a lot. my homepage is mail.yahoo.com, which is heavy on javascript and loads slowly. While it loads, I try to type something into the search bar, with the intention of opening the search in another tab (using alt-enter). however, yahoo keeps stealing the focus from the search bar *repeatedly* while it is loading. I think this behavior is completely wrong.

Regarding comment 25: I don't think that detecting typing is the right approach either. If I interactively placed the focus on the search bar, it should just stay there, no matter what. The search bar is used for many references other than google - perhaps I am thinking about what to type in for a couple of seconds before actually typing it in. Same goes for password prompts and other firefox components.

My rule of thumb is: if the user placed the focus on any component which is a browser component, and not a component inside the web page, then the web page shouldn't be able to steal it.

In my mind, anything that is not placed inside a tab (e.g., search bar, url bar, password prompts, and any input that originates from a firefox extension) is a browser component, and focus should not be stolen from it.

Comment 28

[Gili Tzabari]

+1

I agree with #27

Comment 31

[Ben Bucksch (:BenB)]

Attachment: Testcase

Comment 32

[Ben Bucksch (:BenB)]

This is a focus problem, not related to Urlbar. Changing component to forms.

Please note that unlike the previous testcase, the new one (by pdp) works reliably, is very simple, and has been published on bugtraq and FD, and may be a vector of other, more severe bugs (like 370092), i.e. may be a security problem. I hope this raises the priority.

Comment 34

[Tony]

UI elements such as location should have a higher focus priority than page elements.

Comment 37

[Jesse Ruderman]

The reporter of bug 417915 points out that this is especially annoying now that Firefox's default homepage is one that has a textbox.focus() call.  If you start Firefox and hit Cmd+L right away, http://www.google.com/firefox will often steal focus from the address bar.

Comment 41

[Andrew Cook]

It's amusing when I launch Firefox, click in the search bar to search MSDN, type in "System.Windows.Forms.Form.FormBorderStyle", and end up on Google's page with the search results for "orderStyle". Nice information about Forex's JavaBeans, but definitely not MSDN and certainly nothing to do with Windows Forms.

I hit this bug on a daily basis, and it's becoming quite annoying.

Comment 45

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Proposal patch

This is my proposal patch. I think this patch should be correct. I'll recreate the patch after focus refactoring if nobody hates this patch.

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

adding focus refactoring owner to cc list.

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

test builds:
https://build.mozilla.org/tryserver-builds/2009-05-13_20:[e-mail]-kill-focus-robber/

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

I think that current focused content is not accessible from the caller of nsFocusManager::SetFocus, it should not do it.

Comment 50

[Neil Deakin]

Comment on attachment 383063 [details] [diff] [review]
Patch v1.0

This will break way too many sites.

When the user loads google.com, the expected behaviour is that the search field on the page is focused.

What you actually want is to only not do this when the user has changed the url field in the meantime.

What you probably would need to do is have a CanBlur type function for textboxes that checks for modification and only allows focus changes if the content to be focused has the same access.

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #50)
> When the user loads google.com, the expected behaviour is that the search field
> on the page is focused.

No, at loading a new page, Fx sets focus to the loading content, therefore, onload function of the web site can set focus to its content. I think that this patch only suppress the focus changing after an user sets focus to a chrome content.

you can test by the patched build:
https://build.mozilla.org/tryserver-builds/[e-mail]-focus_steal_1/

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Neil, would you check my comment 51?

Comment 53

[Neil Deakin]

So Safari seems to do this as well, so maybe it is OK.

Two issues:

- it should only be checked for non-key and non-mouse focusing.
- it blocks any focus changes even those in hidden tabs. You probably want to do this check inside the block in SetFocusInner that starts with:
  if (isElementInActiveWindow ...

Comment 54

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0

(In reply to comment #53)
> - it should only be checked for non-key and non-mouse focusing.

nsContentUtils::CanCallerAccess returns always true at that time because the instance of the principal is nsSystemPrincipal.

> - it blocks any focus changes even those in hidden tabs. You probably want to
> do this check inside the block in SetFocusInner that starts with:
>   if (isElementInActiveWindow ...

if mFocusedContent isn't null, mFocusedWindow isn't null too, right? I inserted the new check in under the block of |if (mFocusedWindow)| of |if (isElementInActiveWindow...| of SetFocusInner.

Comment 55

[Neil Deakin]

> nsContentUtils::CanCallerAccess returns always true at that time because the
> instance of the principal is nsSystemPrincipal.

But checking for non-key and mon-mouse events eliminates the extra time needed for the access check for most cases, and reduces the likelihood of regressions, so I still we should check anyway.

+        if (!nsContentUtils::CanCallerAccess(currentFocusedNode)) {
+          return;
+        }

I think that for a hidden tab, the element should still receive focus, so that when that tab is switched back to, the element is focused. Then the code here should actually fall through and do the else block (where the comment "otherwise, for inactive windows..." is).

I'd like to have some tests for this bug.

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #55)
> > nsContentUtils::CanCallerAccess returns always true at that time because the
> > instance of the principal is nsSystemPrincipal.
> 
> But checking for non-key and mon-mouse events eliminates the extra time needed
> for the access check for most cases, and reduces the likelihood of regressions,
> so I still we should check anyway.

O.K. But I'm not sure that how to do it, I'll find the way.

> +        if (!nsContentUtils::CanCallerAccess(currentFocusedNode)) {
> +          return;
> +        }
> 
> I think that for a hidden tab, the element should still receive focus, so that
> when that tab is switched back to, the element is focused. Then the code here
> should actually fall through and do the else block (where the comment
> "otherwise, for inactive windows..." is).

The block is in the
|if (isElementInActiveWindow && allowFrameSwitch && IsWindowVisible(newWindow))|
, so I guessed that |!IsWindowVisible(newWindow)| means that the new window is in hidden tab, isn't it right?

> I'd like to have some tests for this bug.

O.K. I'll try to create the tests.

Comment 57

[Neil Deakin]

> The block is in the
> |if (isElementInActiveWindow && allowFrameSwitch &&
> IsWindowVisible(newWindow))|
> , so I guessed that |!IsWindowVisible(newWindow)| means that the new window is
> in hidden tab, isn't it right?

Ah yes. I was confusing this with the previous patch.

Comment 58

[Neil Deakin]

Comment on attachment 385343 [details] [diff] [review]
Patch v2.0

- for now, waiting for tests

Comment 60

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.0

Adding the automated tests.

By the tests, I could know there is a bug in the previous patch. When a chrome element has focus, the active element didn't updated. This patch fixes the bug.

Comment 61

[Neil Deakin]

Comment on attachment 390238 [details] [diff] [review]
Patch v3.0

>+
>+  let canRetry = 10;
>+
>+  function doTest2() {
>+    if (canRetry-- > 0 &&
>+        (tabs[0].linkedBrowser.contentDocument.activeElement.tagName != "INPUT" ||
>+         tabs[1].linkedBrowser.contentDocument.activeElement.tagName != "INPUT")) {
>+      setTimeout(doTest2, 10); // retry
>+      return;
>+    }

Why is this extra test needing to use a timeout?


>+  PRBool canStealFocus = PR_TRUE;
>+  if (mFocusedContent) {
>+    // Check whether the new content should be able to steal the focus from
>+    // current focused node.
>+    nsCOMPtr<nsIDOMNode> currentFocusedNode =
>+                           do_QueryInterface(mFocusedContent);
>+    NS_ASSERTION(currentFocusedNode,
>+                 "mFocusedContent doesn't have nsIDOMNode");

The focus cannot be set to anything but an element, so this assertion isn't needed.


>+    // When the current focused node is in chrome, any web contents should
>+    // not be able to steal the focus.
>+    if (!(aFlags & (FLAG_BYMOUSE | FLAG_BYKEY)) &&

I'd check the flag on the if (mFocusedContent) line instead.


>-    if (aFlags & FLAG_RAISE)
>+    if ((aFlags & FLAG_RAISE) && canStealFocus)
>       RaiseWindow(newRootWindow);

The one caller where FLAG_RAISE is used already checks to ensure that the relevant permission is set, so need for this check here.

Comment 62

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #61)
> (From update of attachment 390238 [details] [diff] [review])
> >+
> >+  let canRetry = 10;
> >+
> >+  function doTest2() {
> >+    if (canRetry-- > 0 &&
> >+        (tabs[0].linkedBrowser.contentDocument.activeElement.tagName != "INPUT" ||
> >+         tabs[1].linkedBrowser.contentDocument.activeElement.tagName != "INPUT")) {
> >+      setTimeout(doTest2, 10); // retry
> >+      return;
> >+    }
> 
> Why is this extra test needing to use a timeout?

The second test set is testing that the focus is set to a chrome element during the page loading. I need to setTimeout in the second test page's onload event for testing it. By this, when onload event fired, the focus may not be moved to the input element. Therefore, I also use setTimeout at onLoad function too. However, it sometimes needs longer delay, it depends on the state of the machine. So, this extra code is for suppressing the new random orange failure.

> >-    if (aFlags & FLAG_RAISE)
> >+    if ((aFlags & FLAG_RAISE) && canStealFocus)
> >       RaiseWindow(newRootWindow);
> 
> The one caller where FLAG_RAISE is used already checks to ensure that the
> relevant permission is set, so need for this check here.

Sorry? I cannot understand this. Did you mean that I don't need to check canSteadFocus?

Comment 63

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.1

Comment 64

[Neil Deakin]

> The second test set is testing that the focus is set to a chrome element during
> the page loading. I need to setTimeout in the second test page's onload event
> for testing it. By this, when onload event fired, the focus may not be moved to
> the input element. Therefore, I also use setTimeout at onLoad function too.
> However, it sometimes needs longer delay, it depends on the state of the
> machine. So, this extra code is for suppressing the new random orange failure.
> 

Do you mean to use the focus event instead of load?

Otherwise, the patch looks ok.

Comment 65

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #64)
> Do you mean to use the focus event instead of load?

The focus event shouldn't be fired on the background tab, right? I think I cannot use the focus event for the test.

Comment 66

[Neil Deakin]

Comment on attachment 390418 [details] [diff] [review]
Patch v3.1

>+    nsCOMPtr<nsIDOMNode> currentFocusedNode =
>+                           do_QueryInterface(mFocusedContent);
>+    // If the caller cannot access to the current focused node,

remove the 'to' here

Comment 67

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.1.1

Thank you, Neil.

Comment 69

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Olli: Would you sr the patch?

Comment 70

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/8366e5cc9f57

Comment 71

[(no longer active)]

Since this patch landed, the Windows unit test box has been orange with a timeout on the mochitest-plain suite.  This doesn't seem to be random.  Maybe someone can investigate this or back the patch out?

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1249192498.1249198477.31960.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1249185298.1249190788.12549.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1249178098.1249184964.15737.gz

Comment 72

[(no longer active)]

The next run turned out orange with another timeout:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1249199698.1249204534.800.gz

Comment 73

[(no longer active)]

OK, I backed the patch out:

http://hg.mozilla.org/mozilla-central/rev/94e882183752

Comment 74

[(no longer active)]

The cycle after the backout turned green:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1249209899.1249214579.25739.gz

Comment 75

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ehsan: Is the patch really the cause? Because the patch changes the XP level behavior, not only for Win32.

Comment 76

[(no longer active)]

(In reply to comment #75)
> Ehsan: Is the patch really the cause? Because the patch changes the XP level
> behavior, not only for Win32.

I must say that it's very likely to be the cause.  We can't be 100% certain, but all the five test runs with this patch were orange in a similar way, so I'd be very surprised if this is not the cause.

I would suggest further investigation using the try server (or better yet a Windows box if you have one.)

Comment 77

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #76)
> (In reply to comment #75)
> > Ehsan: Is the patch really the cause? Because the patch changes the XP level
> > behavior, not only for Win32.
> 
> I must say that it's very likely to be the cause.  We can't be 100% certain,
> but all the five test runs with this patch were orange in a similar way, so I'd
> be very surprised if this is not the cause.
> 
> I would suggest further investigation using the try server (or better yet a
> Windows box if you have one.)

Yeah, I (re-)posted the patch to the tryserver.

Comment 78

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And note that I rechecked the tryserver logs which are tried before the final review. But the all tests passed on Windows too.

> Your Try Server unit test (focus-steal3) was successfully completed on win32.  It should be available for download at http://build.mozilla.org/tryserver-builds/[e-mail]-focus-steal3
> 
> Summary of unittest results:
> check: 1098/0
> xpcshell-tests: 644/0
> reftest: 3264/0/171
> crashtest: 1259/0/8
> mochitest-plain: 86165/0/1272
> mochitest-chrome: 7461/0/105
> mochitest-browser-chrome: 3447/0/5
> mochitest-a11y: 4137/0/79
> 
> Visit http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry to view the full logs.

Comment 79

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The last orange:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1249206898.1249216159.11903.gz

This is bug 498339. At this time:

> mochitest-plain
> 86899/0/1256

So, I guess, the actual issue is an intermittent failure of handlerApp.xhtml.

Comment 80

[(no longer active)]

Please note that the last orange contains two failures.  Bug 498339 is about the mochitest-browser-chrome failure.

Comment 81

[(no longer active)]

(In reply to comment #80)
> Please note that the last orange contains two failures.  Bug 498339 is about
> the mochitest-browser-chrome failure.

Please ignore this comment, I was on the wrong tab.  :-(

Comment 82

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's possible that this also caused a performance regression:
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/e88e144637e3a531#
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/4ef877841efb2231#

Comment 83

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #82)
> It's possible that this also caused a performance regression:
> http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/e88e144637e3a531#
> http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/4ef877841efb2231#

Um. The patch can cause the performance regression if the |nsContentUtils::CanCallerAccess| is expensive.

Comment 84

[(no longer active)]

Hrm, this run also ended up in a similar timeout (after the backout):

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1249211586.1249214831.28549.gz

So he orange might not be the fault of this patch after all...  I'm really puzzled here.

Comment 85

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I guess that the performance regression might increase the possibility of the intermittent failure...

Comment 86

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> Your Try Server unit test (bug125282) was successfully completed on win32.  It should be available for download at http://build.mozilla.org/tryserver-builds/[e-mail]-bug125282
> 
> Summary of unittest results:
> check: 1104/0
> xpcshell-tests: 648/0
> reftest: 3327/0/173
> crashtest: 1263/0/8
> mochitest-plain: 86905/0/1256
> mochitest-chrome: 7484/0/105
> mochitest-browser-chrome: 3501/0/5
> mochitest-a11y: 4276/0/79
> 
> Visit http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry to view the full logs.

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1249219348.1249225850.29601.gz

The patch passes all tests on Windows. So, I bet that the failure is non-related intermittent failure. But I should fix the perf regression before relanding.

Comment 87

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #83)
> (In reply to comment #82)
> > It's possible that this also caused a performance regression:
> > http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/e88e144637e3a531#
> > http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/4ef877841efb2231#
> 
> Um. The patch can cause the performance regression if the
> |nsContentUtils::CanCallerAccess| is expensive.

Um, the |nsFocusManager::SetFocusInner| isn't called in most cases during normal navigation, do tp4 tests use the method in many times??

Comment 88

[Micah Gersten]

Ubuntu Bug:
https://bugs.launchpad.net/bugs/239831

Comment 89

[Boris Zbarsky [:bzbarsky]]

"expensive" is a relative concept.  It shouldn't be particularly expensive compared to other things involved in a focus change (e.g. focus event firing).

Comment 90

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, Boris.

I tried to use some ugly checking before calling nsContentUtils::CanCallerAccess. But I cannot fix the tp score down. I'm thinking that the current patch should be relanded without any changes. And also I should create a new patch which uses nsIFocusManager::SetFocus instead of nsIDOM*::Focus() directly with a new flag (e.g., FLAG_NO_PERMISSION_CHECK) at the cause of the tp score down point. (I'm not sure where is it.)

Comment 91

[Boris Zbarsky [:bzbarsky]]

Point of comment 89 was that I seriously doubt the security check would be a performance problem here....

Comment 92

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v4.0

Comment 93

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v4.0.1

This is faster than the previous patch. (I don't find the perf regression in tryservers.)

nsContentUtils::CanCallerAccess is expensive for there. But it's really needed in some cases. So, I tried to reduce the using times.

It's needed only when the focus is moved from content to another content. But I realized that it's rare case. I used following checking for other cases:

1. When the focus is moved to chrome content, that means that the caller can access chrome. So, we can reduce the checking at this time.

2. When the focus is moved in same document, we don't need to suppress the focus changing.

3. If the focused content is in chrome and the new content is in content, we only need the capability check for UniversalXPConnect. The cost is cheaper than the nsContentUtils::CanCallerAccess.

And also if we can know whether the caller is trusted or not, we can reduce the checking in many times. Therefore, I add nsIFocusManager::FLAG_NOTRUSTED which should be specified when the caller cannot trust the context without the capability checking. I worried over whether it should be FLAG_NOTRUSTED or FLAG_TRUSTED. Now, I believe FLAG_NOTRUSTED is better because most nsIFocusManager clients are in trusted context. So, only nsIDOMElement::Focus should use them.

And also I changed to not use FLAG_BYMOUSE and FLAG_BYKEY because they are passed by unstrasted event. We should check the trust flag in nsEvent.

Comment 94

[Boris Zbarsky [:bzbarsky]]

Uh... how often does this code get called, exactly?  CanCallerAccess is NOT that expensive.  If it's not happening thousands of times per pageload, it shouldn't be noticeable.

Comment 95

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #94)
> Uh... how often does this code get called, exactly?  CanCallerAccess is NOT
> that expensive.  If it's not happening thousands of times per pageload, it
> shouldn't be noticeable.

I'm not sure. I guess it's at most one or two times. But that made the tp damage, I think.

Comment 96

[Neil Deakin]

It should only be called once or twice per pageload, plus any attempts by script to focus the page while its loading.

The patch itself adds a lot of extra complexity. We should narrow down why there's a performance issue. From the description above, it's just on one machine?

Comment 97

[Boris Zbarsky [:bzbarsky]]

One or two calls to CanCallerAccess per pageload are not going to move the Tp needle.

Comment 98

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Note: The tp scores on tryserver were:

without patch or with the latest patch : about 217
with old patch: about 220

Comment 99

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #96)
> The patch itself adds a lot of extra complexity. We should narrow down why
> there's a performance issue. From the description above, it's just on one
> machine?

No. I used tryserver many times for tp testing. But looks like the scores are similar between the machines.

Comment 100

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I re-posted the previous patch (v3.1.1 which was landed to trunk) to the tryserver.

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1249898428.37.1249901033.24447.gz&fulltext=1

The lines of "NOISE: nsContentUtils::CanCallerAccess Called" are the log of nsContentUtils::CanCallerAccess called (I put printf() to the patch).

It was called 32 times per a cycle.

Comment 101

[Boris Zbarsky [:bzbarsky]]

So basically < 1 time per pageload?

There's no way it's causing a 1% Tp regression, if so.

Comment 102

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I reposted the patch to the tryserver without the printf().

tp on linux are:

tp: 220.72
tp: 220.95

clearly, 3 or more slower than other people's patched score.

Perhaps, there are no reasons we can say "nsContentUtils::CanCallerAccess doesn't cause the tp damage"...

Comment 103

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #102)
> Perhaps, there are no reasons we can say "nsContentUtils::CanCallerAccess
> doesn't cause the tp damage"...

Er, if the else block (beginning with "if (isElementInActiveWindow && allowFrameSwitch &&...) is more expensive than the if block, the tp regression can be...

Comment 104

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ugh... but the my latest patch faster...

Comment 105

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The latest patch scores on tryserver:

tp: 217.22
tp: 216.36

I bet the tp regression causes the cost of nsContentUtils::CanCallerAccess...

Comment 106

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v4.1

this is little simpler than the v4.0.1.

Comment 107

[Rob Campbell [:rc] (:robcee)]

this may be causing another problem with extensions like Firebug and Twitterfox that have editable textfields.

see bug 509478.

Comment 108

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #107)
> this may be causing another problem with extensions like Firebug and Twitterfox
> that have editable textfields.
> 
> see bug 509478.

Why? Isn't the .focus() called in chrome context?

Comment 109

[Rob Campbell [:rc] (:robcee)]

not sure it's explicitly called.

we'll do a little work to verify that this patch is indeed the culprit before making any more wild-guesses. Stay tuned! :)

Comment 110

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If you can reproduce the bug on current trunk build, this bug's patch is not the cause because the patch was backed-out already.
# the patch landed only several hours.

Comment 111

[Rob Campbell [:rc] (:robcee)]

ok, thanks Masayuki. I'm seeing the problem in today's nightly. That saved me a build.

Comment 112

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v4.2

Adding some tests to the new test.

However, this test makes an unexpected failure of non-related test only on Linux. I'm looking for the reason.

# If this tests are not run, the unexpected failure doesn't happen.

> Running chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js...
> TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Console message: [JavaScript Error: "[Exception... "'JavaScript component does not have a method named: "observe"' when calling method: [nsIObserver::observe]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js :: notifyAction :: line 664"  data: no]" {file: "file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js" line: 664}]
> TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Console message: [JavaScript Error: "[Exception... "'JavaScript component does not have a method named: "observe"' when calling method: [nsIObserver::observe]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js :: notifyAction :: line 664"  data: no]" {file: "file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js" line: 664}]
> TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Console message: [JavaScript Error: "[Exception... "'JavaScript component does not have a method named: "observe"' when calling method: [nsIObserver::observe]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js :: notifyAction :: line 664"  data: no]" {file: "file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js" line: 664}]
> TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Console message: [JavaScript Error: "[Exception... "'JavaScript component does not have a method named: "observe"' when calling method: [nsIObserver::observe]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js :: notifyAction :: line 664"  data: no]" {file: "file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js" line: 664}]
> TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Console message: [JavaScript Error: "[Exception... "'JavaScript component does not have a method named: "observe"' when calling method: [nsIObserver::observe]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js :: notifyAction :: line 664"  data: no]" {file: "file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js" line: 664}]
> TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Console message: [JavaScript Error: "[Exception... "'JavaScript component does not have a method named: "observe"' when calling method: [nsIObserver::observe]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js :: notifyAction :: line 664"  data: no]" {file: "file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js" line: 664}]
> TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Console message: [JavaScript Error: "[Exception... "'JavaScript component does not have a method named: "observe"' when calling method: [nsIObserver::observe]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js :: notifyAction :: line 664"  data: no]" {file: "file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js" line: 664}]
> TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Console message: [JavaScript Error: "[Exception... "'JavaScript component does not have a method named: "observe"' when calling method: [nsIObserver::observe]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js :: notifyAction :: line 664"  data: no]" {file: "file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js" line: 664}]
> TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Console message: [JavaScript Error: "[Exception... "'JavaScript component does not have a method named: "observe"' when calling method: [nsIObserver::observe]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js :: notifyAction :: line 664"  data: no]" {file: "file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js" line: 664}]
> TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Selecting the first tab scrolls it into view - Got 18, expected 16
> TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Scrolled one tab to the right with a single click
> TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Selecting the last tab scrolls it into view
> TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Scrolled one tab to the left with a single click
> TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Scrolled one page of tabs with a double click
> TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Scrolled to the start with a triple click
> TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Scrolled one tab to the right with the mouse wheel

Comment 113

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v4.3

ok, this should be good.

Boris still thinks nsContentUtils::CanCallerAccess isn't the cause of the tp regression. But the experimentation didn't show so. The method can be improved the performance, but I'm not expert of it. So, I have no idea to fix this bug without I reduce the calling it.

This patch is simpler than the previous reviewed patch. But this still add a new flag to nsIFocusManager. See the test case, the web developers can cheat the previous patch by using the createEvent. So, the flag is really needed.

Comment 114

[Neil Deakin]

(In reply to comment #100)
> The lines of "NOISE: nsContentUtils::CanCallerAccess Called" are the log of
> nsContentUtils::CanCallerAccess called (I put printf() to the patch).
> 
> It was called 32 times per a cycle.

Is that calls to nsContentUtils::CanCallerAccess or calls from the focus manager to it? When I run the normal performance test (not sure about this specific one) I get only one call to ShiftFocusInner.

What happens if you just replace the call to nsContentUtils::CanCallerAccess with code that just always matches/never matches?

I suspect more likely that removing a focus call is causing some other behaviour to occur, or changing the timing of a flush or somesuch. The pageload performance tests don't actually test focus time, they only test up until the page is loaded, and focus can occur after that point. So using the pageload tests for testing focus performance regressions is generally going to be inaccurate anyway.

I don't really like this newer patch as it adds quite a bit of extra complexity and requires callers to think about what to set the new flag to.

That said, I noticed a problem with the earlier patch anyway with the following test:

<textarea id="t">...</textarea><script>document.getElementById('t').focus()</script>

Focus the urlbar, reload the page, lower the window and then raise it again. The urlbar should be focused yet the textarea is now focused, even though the urlbar was focused before the window was lowered. This is caused because the setting of 'canStealFocus' should actually just be clearing 'allowFrameSwitch', so that the later conditions which check this don't match.

Personally I would rather just fix this problem, and check in the earlier patch.

Comment 115

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #114)
> (In reply to comment #100)
> > The lines of "NOISE: nsContentUtils::CanCallerAccess Called" are the log of
> > nsContentUtils::CanCallerAccess called (I put printf() to the patch).
> > 
> > It was called 32 times per a cycle.
> 
> Is that calls to nsContentUtils::CanCallerAccess or calls from the focus
> manager to it? When I run the normal performance test (not sure about this
> specific one) I get only one call to ShiftFocusInner.

That means latter (from the first landed patch).

> What happens if you just replace the call to nsContentUtils::CanCallerAccess
> with code that just always matches/never matches?

Did you mean the |foo = nsContentUtils::CanCallerAccess| to |foo = PR_TRUE|? If so, at that time, the perf regression didn't happen.

> I suspect more likely that removing a focus call is causing some other
> behaviour to occur, or changing the timing of a flush or somesuch. The pageload
> performance tests don't actually test focus time, they only test up until the
> page is loaded, and focus can occur after that point. So using the pageload
> tests for testing focus performance regressions is generally going to be
> inaccurate anyway.

If so, the new patch without the optimization shouldn't be faster than the old patch, I'll check it.

And I'll check the bug of you pointed out.

Comment 116

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I posted the old patch and following patterns:
* nsContentUtils::CanCallerAccess is replaced to PR_TRUE
* nsContentUtils::CanCallerAccess is replaced to PR_FALSE
* the if block was commented out

But the their tp*4* scores are almost same value. So, looks like the tp4 don't include the nsIDOMElement::focus cost in onload event to the score.

Boris, do you know the difference between tp3 and tp4?

Comment 117

[Boris Zbarsky [:bzbarsky]]

It's a different pageset.  I think the harness is the same.

Comment 119

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v5.0

I hope Enn likes this patch. In the AdjustWindowFocus, we need to check the permission too. And I removes to check the key/mouse events flag for the simple code.

I'll create some additional tests.

Comment 120

[Paul Bailey]

I don't know if this is a new bug or not, but this is a related problematic issue:

If you are using a form and you are editing a field, the page JS can move the focus to another spot in the form. Annoying, right?

But this is also a security problem because of login pages. Here entry 1 is often username name and entry 2 is often password. In some cases I can finish the user name, start in on the password and the JS moves me back to the username.

Now, imagine doing that when using a public computer lab or worse a projector. This issue makes me wonder if the importance of this bug should not be changed.

login page

Comment 121

[Ben Bucksch (:BenB)]

Paul Bailey, this is a good point, but not the same as this bug. Can you file a new one, please?

Comment 122

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The patch will forbid to steal focus from another document.

Comment 123

[Paul Bailey]

Ben Bucksch, good idea. I added bug 532097, with some more thought into what should be fixed.

Comment 124

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v5.0.1

o.k., would you review this again?

1. We should test only when the focus moves between different documents.
2. Don't refer the mouse flag and the keyboard flag, the web developers can cheat the check by DOM event creating.
3. AdjustWindowFocus() needs to check too only when it's called by |SetFocusInner|.

Comment 125

[Neil Deakin]

Comment on attachment 415556 [details] [diff] [review]
Patch v5.0.1

>+  // XXX Clear the dragging state here, if not so, it breaks next test result.
>+  EventUtils.synthesizeKey("VK_ESCAPE", {}, window);

This won't do anything as the escape key during a drag is handled by the system.


>+
>+    callback = doTest2;
>+    loadTestPage();
>+
>+    // Set focus to chrome element before onload events of the loading contents.
>+    BrowserSearch.searchBar.focus();

I think it would be clearer to call this last line during loadTestPage before loading the url.


> #include "nsTreeWalker.h"
> #include "nsIDOMNodeFilter.h"
>+#include "nsIScriptSecurityManager.h"

I don't think this is needed.

Comment 126

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #125)
> (From update of attachment 415556 [details] [diff] [review])
> >+  // XXX Clear the dragging state here, if not so, it breaks next test result.
> >+  EventUtils.synthesizeKey("VK_ESCAPE", {}, window);
> 
> This won't do anything as the escape key during a drag is handled by the
> system.

Oh, really? But this change fixed the orange which is happened by my new test file... I'll check it again tomorrow.

> >+
> >+    callback = doTest2;
> >+    loadTestPage();
> >+
> >+    // Set focus to chrome element before onload events of the loading contents.
> >+    BrowserSearch.searchBar.focus();
> 
> I think it would be clearer to call this last line during loadTestPage before
> loading the url.

No. Probably, Fx sets focus to contents when it starts to load a page on active tab. Therefore loadTestPage() sets focus to the active tab manually. This test tests a case which the focus is moved to chrome by user during the page loading. So, your suggestion changes the content of the testing.

Comment 127

[Neil Deakin]

(In reply to comment #126)

> > This won't do anything as the escape key during a drag is handled by the
> > system.
> 
> Oh, really? But this change fixed the orange which is happened by my new test
> file... I'll check it again tomorrow.

Possibly some side effect then. I don't mind if this is put in, but it would be good to know what is happening here.


> No. Probably, Fx sets focus to contents when it starts to load a page on active
> tab. Therefore loadTestPage() sets focus to the active tab manually. This test
> tests a case which the focus is moved to chrome by user during the page
> loading. So, your suggestion changes the content of the testing.

OK, although I'm worried that the timing of loading and focusing might cause intermittent failures.

Comment 128

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #127)
> OK, although I'm worried that the timing of loading and focusing might cause
> intermittent failures.

I think that is no problem. The onload event which fires doTest2() is going to be called after the doTest1() is finished. Can onload() event intrude into the stack?

Comment 129

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v5.1

I see what happens on browser_drag.js.

When the tests finished, the identity panel is opened by a click event which is generated by the drag emulation. So, we need to close it by the esc key event.

Comment 130

[Neil Deakin]

A better fix for that is to prevent the panel from opening by cancelling either the click or popupshowing events.

Comment 131

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think it needs more code than my approach, and it depends on the Fx's XUL structure, so, it could be broken by some UI changes...

Comment 132

[Dão Gottwald [:dao]]

Why is this in browser/base/content/test/ anyway, rather than dom/tests/browser/ for instance?

Comment 133

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v5.2

O.K. The new test file is moved to dom/tests/browser.

Comment 134

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v5.2

Oops...

Comment 135

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/a8f46161c891

Comment 136

[Paul Bailey]

Hey, having just fixed this, it might make sense to fix Bug 532097 while you know this portion of the code well.

Comment 137

[Dão Gottwald [:dao]]

Comment on attachment 417026 [details] [diff] [review]
Patch v5.2

>+  // If the previous (or more) test finished with cleaning up the tabs,
>+  // there may be some pending animations. That can cause a failure of
>+  // this tests, so, we should test this in another stack.
>+  setTimeout(doTest, 0);

Tabs are closed synchronously, so I don't think I understand this. What kind of animations are you referring to?

Comment 138

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #137)
> (From update of attachment 417026 [details] [diff] [review])
> >+  // If the previous (or more) test finished with cleaning up the tabs,
> >+  // there may be some pending animations. That can cause a failure of
> >+  // this tests, so, we should test this in another stack.
> >+  setTimeout(doTest, 0);
> 
> Tabs are closed synchronously, so I don't think I understand this. What kind of
> animations are you referring to?

I'm not sure, on Linux, the test failed without the code in the older patch. I have no idea except that the animation is the problem.

Comment 139

[Dão Gottwald [:dao]]

Which animation?

Comment 140

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

So, I guessed the tab closing isn't synchronously.

Comment 141

[Dão Gottwald [:dao]]

But it is.

Comment 145

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Web pages still can steal focus, see bug 604289. I'll fix it ASAP.