Last Comment Bug 125282 - Webpage-JS can steal focus from URLbar / chrome
: Webpage-JS can steal focus from URLbar / chrome
Status: RESOLVED FIXED
parity-ie
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 29 votes (vote)
: mozilla1.9.3a1
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
:
Mentors:
https://bugzilla.mozilla.org/attachme...
: 124191 140524 143973 164339 229007 230648 271565 274747 302671 317643 359549 367440 370094 377011 417915 445050 449650 466341 475375 496502 503643 507107 513390 561131 565544 569254 623066 624680 626156 638047 (view as bug list)
Depends on: 419061 604289 656026 534840 535903 535970 591890 976673
Blocks: 55416 focusnav 140346 cuts-focus 295056
  Show dependency treegraph
 
Reported: 2002-02-13 10:11 PST by Gili
Modified: 2014-05-26 13:09 PDT (History)
70 users (show)
roc: wanted1.9.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
slower loading testcase (7.25 KB, text/html)
2004-11-24 11:03 PST, Brian 'netdragon' Bober
no flags Details
Testcase (236 bytes, text/html)
2007-02-11 15:56 PST, Ben Bucksch (:BenB)
no flags Details
Proposal patch (2.96 KB, patch)
2009-05-13 19:57 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
Patch v1.0 (1.02 KB, patch)
2009-06-12 21:52 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
enndeakin: review-
Details | Diff | Review
Patch v2.0 (1.94 KB, patch)
2009-06-26 02:38 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
enndeakin: review-
Details | Diff | Review
Patch v3.0 (6.73 KB, patch)
2009-07-23 08:40 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
Patch v3.1 (5.88 KB, patch)
2009-07-23 22:44 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
enndeakin: review+
Details | Diff | Review
Patch v3.1.1 (5.80 KB, patch)
2009-07-27 09:16 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
masayuki: review+
bugs: superreview+
Details | Diff | Review
Patch v4.0 (47.24 KB, patch)
2009-08-07 08:50 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
Patch v4.0.1 (47.43 KB, patch)
2009-08-07 09:16 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
Patch v4.1 (38.07 KB, patch)
2009-08-11 02:14 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
Patch v4.2 (41.36 KB, patch)
2009-08-14 21:39 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
Patch v4.3 (42.41 KB, patch)
2009-08-18 03:05 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
Patch v5.0 (16.88 KB, patch)
2009-09-01 01:20 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
Patch v5.0.1 (16.88 KB, patch)
2009-12-01 19:32 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
enndeakin: review+
Details | Diff | Review
Patch v5.1 (16.47 KB, patch)
2009-12-08 20:08 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
masayuki: review+
Details | Diff | Review
Patch v5.2 (15.02 KB, patch)
2009-12-10 18:05 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
masayuki: review+
Details | Diff | Review
Patch v5.2 (18.27 KB, patch)
2009-12-10 18:06 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
masayuki: review+
Details | Diff | Review

Description Gili 2002-02-13 10:11:40 PST
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 2002-02-13 22:00:52 PST
Do you mean the caret?  That's what Ctrl-L does.  This seems invalid.
Comment 2 Gili 2002-02-13 22:08:14 PST
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 2002-02-13 22:44:54 PST
->bryner for triage, cc aaronl. 
Comment 4 Tom Mraz 2002-03-05 06:21:25 PST
Very similar is bug 124750.
Seeing on Linux 2002030414 m0.9.9branch build

Comment 5 sairuh (rarely reading bugmail) 2002-03-05 11:30:55 PST
urlbar
Comment 6 Peter Trudelle 2002-03-06 00:53:30 PST
This doesn't seem like an URL bar problem so much as a focus problem. cc bryner.
 Dup?
Comment 7 Boris Zbarsky [:bz] 2002-05-12 23:01:45 PDT
*** Bug 143973 has been marked as a duplicate of this bug. ***
Comment 8 Neil Marshall 2002-05-13 09:11:07 PDT
URL From Bug 143973
http://www.episode-x.com/episode2/index.php#685
Comment 9 Jesse Ruderman 2002-07-22 20:03:54 PDT
*** Bug 140524 has been marked as a duplicate of this bug. ***
Comment 10 Jesse Ruderman 2004-01-12 01:58:31 PST
*** Bug 230648 has been marked as a duplicate of this bug. ***
Comment 11 Gili 2004-01-12 15:52:25 PST
This bug is almost 2 years old. Is anyone following up on it?
Comment 12 graamone 2004-10-15 03:06:00 PDT
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: gavin@gavinsharp.com] 2004-11-24 09:06:39 PST
*** Bug 271565 has been marked as a duplicate of this bug. ***
Comment 14 Brian 'netdragon' Bober 2004-11-24 11:00:57 PST
Google.com loads pretty quickly for testing this. Slower loading testcase coming up.
Comment 15 Brian 'netdragon' Bober 2004-11-24 11:03:54 PST
Created attachment 166947 [details]
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 2004-11-24 11:25:38 PST
+1

I like Brian's idea of redirecting focus requests to he next tabstop.
Comment 17 Vaclav Dvorak 2004-11-24 13:37:24 PST
As much as I hate focus stealing, this is definitely not a "major loss of function".
Comment 18 Aaron Leventhal 2004-12-02 14:31:16 PST
*** Bug 229007 has been marked as a duplicate of this bug. ***
Comment 19 Phil Ringnalda (:philor) 2004-12-15 10:43:13 PST
*** Bug 274747 has been marked as a duplicate of this bug. ***
Comment 20 Dave Townsend [:mossop] 2005-07-29 14:44:26 PDT
*** Bug 302671 has been marked as a duplicate of this bug. ***
Comment 21 Robert Claypool 2005-07-29 17:51:42 PDT
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 2005-07-29 18:07:47 PDT
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 2005-07-31 10:50:51 PDT
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 2005-11-23 21:18:23 PST
*** Bug 317643 has been marked as a duplicate of this bug. ***
Comment 25 djstealth 2005-11-23 21:50:41 PST
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 2005-11-25 16:17:53 PST
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 2006-11-25 11:33:20 PST
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 2006-11-25 16:55:27 PST
+1

I agree with #27
Comment 29 Ria Klaassen (not reading all bugmail) 2007-01-19 01:53:10 PST
*** Bug 367440 has been marked as a duplicate of this bug. ***
Comment 30 Jesse Ruderman 2007-02-11 15:25:46 PST
*** Bug 370094 has been marked as a duplicate of this bug. ***
Comment 31 Ben Bucksch (:BenB) 2007-02-11 15:56:26 PST
Created attachment 254761 [details]
Testcase
Comment 32 Ben Bucksch (:BenB) 2007-02-11 16:37:29 PST
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 33 Dave Townsend [:mossop] 2007-04-10 06:00:35 PDT
*** Bug 377011 has been marked as a duplicate of this bug. ***
Comment 34 Tony 2007-04-15 13:57:47 PDT
UI elements such as location should have a higher focus priority than page elements.
Comment 35 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-07-31 11:47:51 PDT
*** Bug 359549 has been marked as a duplicate of this bug. ***
Comment 36 Jesse Ruderman 2008-02-16 14:34:45 PST
*** Bug 417915 has been marked as a duplicate of this bug. ***
Comment 37 Jesse Ruderman 2008-02-16 14:42:58 PST
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 38 Wayne Mery (:wsmwk, use Needinfo for questions) 2008-02-23 10:39:47 PST
*** Bug 164339 has been marked as a duplicate of this bug. ***
Comment 39 Matthias Versen [:Matti] 2008-07-13 14:57:54 PDT
*** Bug 445050 has been marked as a duplicate of this bug. ***
Comment 40 Matthias Versen [:Matti] 2008-07-13 14:59:27 PDT
*** Bug 124191 has been marked as a duplicate of this bug. ***
Comment 41 Andrew Cook 2008-08-02 16:56:02 PDT
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 42 Jesse Ruderman 2008-08-07 18:14:37 PDT
*** Bug 449650 has been marked as a duplicate of this bug. ***
Comment 43 Matthias Versen [:Matti] 2009-01-26 20:01:45 PST
*** Bug 475375 has been marked as a duplicate of this bug. ***
Comment 44 Matthias Versen [:Matti] 2009-01-26 20:03:55 PST
*** Bug 466341 has been marked as a duplicate of this bug. ***
Comment 45 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-05-13 19:57:27 PDT
Created attachment 377320 [details] [diff] [review]
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] (Mozilla Japan) (working slowly due to injured) 2009-05-13 19:59:10 PDT
adding focus refactoring owner to cc list.
Comment 47 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-05-13 21:10:59 PDT
test builds:
https://build.mozilla.org/tryserver-builds/2009-05-13_20:15-masayuki@d-toybox.com-kill-focus-robber/
Comment 48 Henrik Skupin (:whimboo) 2009-06-06 12:05:02 PDT
*** Bug 496502 has been marked as a duplicate of this bug. ***
Comment 49 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-06-12 21:52:39 PDT
Created attachment 383063 [details] [diff] [review]
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 2009-06-13 05:16:53 PDT
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] (Mozilla Japan) (working slowly due to injured) 2009-06-13 05:28:45 PDT
(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/masayuki@d-toybox.com-focus_steal_1/
Comment 52 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-06-16 21:09:06 PDT
Neil, would you check my comment 51?
Comment 53 Neil Deakin 2009-06-25 05:50:59 PDT
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] (Mozilla Japan) (working slowly due to injured) 2009-06-26 02:38:14 PDT
Created attachment 385343 [details] [diff] [review]
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 2009-07-03 11:22:48 PDT
> 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] (Mozilla Japan) (working slowly due to injured) 2009-07-03 12:38:05 PDT
(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 2009-07-06 08:13:03 PDT
> 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 2009-07-09 06:57:00 PDT
Comment on attachment 385343 [details] [diff] [review]
Patch v2.0

- for now, waiting for tests
Comment 59 Matthias Versen [:Matti] 2009-07-11 00:40:47 PDT
*** Bug 503643 has been marked as a duplicate of this bug. ***
Comment 60 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-07-23 08:40:42 PDT
Created attachment 390238 [details] [diff] [review]
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 2009-07-23 12:25:20 PDT
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] (Mozilla Japan) (working slowly due to injured) 2009-07-23 19:10:55 PDT
(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] (Mozilla Japan) (working slowly due to injured) 2009-07-23 22:44:00 PDT
Created attachment 390418 [details] [diff] [review]
Patch v3.1
Comment 64 Neil Deakin 2009-07-27 08:31:37 PDT
> 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] (Mozilla Japan) (working slowly due to injured) 2009-07-27 08:45:02 PDT
(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 2009-07-27 08:55:24 PDT
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] (Mozilla Japan) (working slowly due to injured) 2009-07-27 09:16:37 PDT
Created attachment 390833 [details] [diff] [review]
Patch v3.1.1

Thank you, Neil.
Comment 68 Matthias Versen [:Matti] 2009-07-29 16:07:38 PDT
*** Bug 507107 has been marked as a duplicate of this bug. ***
Comment 69 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-07-31 02:23:54 PDT
Olli: Would you sr the patch?
Comment 70 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-08-01 18:52:49 PDT
http://hg.mozilla.org/mozilla-central/rev/8366e5cc9f57
Comment 71 :Ehsan Akhgari (busy, don't ask for review please) 2009-08-02 01:55:55 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2009-08-02 03:38:57 PDT
The next run turned out orange with another timeout:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1249199698.1249204534.800.gz
Comment 73 :Ehsan Akhgari (busy, don't ask for review please) 2009-08-02 03:45:17 PDT
OK, I backed the patch out:

http://hg.mozilla.org/mozilla-central/rev/94e882183752
Comment 74 :Ehsan Akhgari (busy, don't ask for review please) 2009-08-02 05:54:20 PDT
The cycle after the backout turned green:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1249209899.1249214579.25739.gz
Comment 75 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-08-02 06:19:36 PDT
Ehsan: Is the patch really the cause? Because the patch changes the XP level behavior, not only for Win32.
Comment 76 :Ehsan Akhgari (busy, don't ask for review please) 2009-08-02 06:34:17 PDT
(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] (Mozilla Japan) (working slowly due to injured) 2009-08-02 06:36:30 PDT
(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] (Mozilla Japan) (working slowly due to injured) 2009-08-02 06:39:47 PDT
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/masayuki@d-toybox.com-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] (Mozilla Japan) (working slowly due to injured) 2009-08-02 06:59:21 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2009-08-02 07:03:49 PDT
Please note that the last orange contains two failures.  Bug 498339 is about the mochitest-browser-chrome failure.
Comment 81 :Ehsan Akhgari (busy, don't ask for review please) 2009-08-02 07:05:30 PDT
(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-7 (review requests must explain patch) 2009-08-02 07:06:25 PDT
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] (Mozilla Japan) (working slowly due to injured) 2009-08-02 07:10:41 PDT
(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 :Ehsan Akhgari (busy, don't ask for review please) 2009-08-02 07:12:35 PDT
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] (Mozilla Japan) (working slowly due to injured) 2009-08-02 07:14:56 PDT
I guess that the performance regression might increase the possibility of the intermittent failure...
Comment 86 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-08-02 08:24:25 PDT
> Your Try Server unit test (bug125282) was successfully completed on win32.  It should be available for download at http://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-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] (Mozilla Japan) (working slowly due to injured) 2009-08-02 09:25:28 PDT
(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 2009-08-02 10:47:34 PDT
Ubuntu Bug:
https://bugs.launchpad.net/bugs/239831
Comment 89 Boris Zbarsky [:bz] 2009-08-02 20:32:33 PDT
"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] (Mozilla Japan) (working slowly due to injured) 2009-08-03 10:46:42 PDT
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 [:bz] 2009-08-03 11:19:00 PDT
Point of comment 89 was that I seriously doubt the security check would be a performance problem here....
Comment 92 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-08-07 08:50:50 PDT
Created attachment 393195 [details] [diff] [review]
Patch v4.0
Comment 93 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-08-07 09:16:14 PDT
Created attachment 393202 [details] [diff] [review]
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 [:bz] 2009-08-07 09:21:36 PDT
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] (Mozilla Japan) (working slowly due to injured) 2009-08-07 09:28:23 PDT
(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 2009-08-07 09:37:37 PDT
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 [:bz] 2009-08-07 09:39:16 PDT
One or two calls to CanCallerAccess per pageload are not going to move the Tp needle.
Comment 98 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-08-07 09:41:09 PDT
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] (Mozilla Japan) (working slowly due to injured) 2009-08-07 09:48:19 PDT
(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] (Mozilla Japan) (working slowly due to injured) 2009-08-10 06:42:18 PDT
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 [:bz] 2009-08-10 08:09:54 PDT
So basically < 1 time per pageload?

There's no way it's causing a 1% Tp regression, if so.
Comment 102 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-08-10 09:20:46 PDT
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] (Mozilla Japan) (working slowly due to injured) 2009-08-10 09:25:53 PDT
(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] (Mozilla Japan) (working slowly due to injured) 2009-08-10 09:32:18 PDT
Ugh... but the my latest patch faster...
Comment 105 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-08-10 11:31:59 PDT
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] (Mozilla Japan) (working slowly due to injured) 2009-08-11 02:14:40 PDT
Created attachment 393732 [details] [diff] [review]
Patch v4.1

this is little simpler than the v4.0.1.
Comment 107 Rob Campbell [:rc] (:robcee) 2009-08-11 09:22:27 PDT
this may be causing another problem with extensions like Firebug and Twitterfox that have editable textfields.

see bug 509478.
Comment 108 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-08-11 09:38:00 PDT
(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) 2009-08-11 09:40:19 PDT
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] (Mozilla Japan) (working slowly due to injured) 2009-08-11 09:45:29 PDT
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) 2009-08-11 09:52:19 PDT
ok, thanks Masayuki. I'm seeing the problem in today's nightly. That saved me a build.
Comment 112 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-08-14 21:39:40 PDT
Created attachment 394621 [details] [diff] [review]
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] (Mozilla Japan) (working slowly due to injured) 2009-08-18 03:05:33 PDT
Created attachment 395023 [details] [diff] [review]
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 2009-08-18 11:38:11 PDT
(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] (Mozilla Japan) (working slowly due to injured) 2009-08-18 23:17:19 PDT
(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] (Mozilla Japan) (working slowly due to injured) 2009-08-28 10:37:11 PDT
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 [:bz] 2009-08-28 13:26:03 PDT
It's a different pageset.  I think the harness is the same.
Comment 118 Jesse Ruderman 2009-08-28 19:55:23 PDT
*** Bug 513390 has been marked as a duplicate of this bug. ***
Comment 119 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-09-01 01:20:35 PDT
Created attachment 397823 [details] [diff] [review]
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 2009-11-30 18:11:26 PST
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) 2009-12-01 02:29:30 PST
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] (Mozilla Japan) (working slowly due to injured) 2009-12-01 05:12:50 PST
The patch will forbid to steal focus from another document.
Comment 123 Paul Bailey 2009-12-01 07:02:44 PST
Ben Bucksch, good idea. I added bug 532097, with some more thought into what should be fixed.
Comment 124 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-12-01 19:32:06 PST
Created attachment 415556 [details] [diff] [review]
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 2009-12-08 10:11:32 PST
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] (Mozilla Japan) (working slowly due to injured) 2009-12-08 10:27:20 PST
(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 2009-12-08 10:30:36 PST
(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] (Mozilla Japan) (working slowly due to injured) 2009-12-08 10:43:16 PST
(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] (Mozilla Japan) (working slowly due to injured) 2009-12-08 20:08:50 PST
Created attachment 416682 [details] [diff] [review]
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 2009-12-09 06:03:46 PST
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] (Mozilla Japan) (working slowly due to injured) 2009-12-09 06:12:07 PST
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] 2009-12-10 14:10:21 PST
Why is this in browser/base/content/test/ anyway, rather than dom/tests/browser/ for instance?
Comment 133 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-12-10 18:05:37 PST
Created attachment 417025 [details] [diff] [review]
Patch v5.2

O.K. The new test file is moved to dom/tests/browser.
Comment 134 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-12-10 18:06:21 PST
Created attachment 417026 [details] [diff] [review]
Patch v5.2

Oops...
Comment 135 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-12-11 21:21:45 PST
http://hg.mozilla.org/mozilla-central/rev/a8f46161c891
Comment 136 Paul Bailey 2009-12-11 21:44:06 PST
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] 2009-12-12 09:24:26 PST
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] (Mozilla Japan) (working slowly due to injured) 2009-12-12 09:32:08 PST
(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] 2009-12-12 09:39:36 PST
Which animation?
Comment 140 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-12-12 09:41:38 PST
So, I guessed the tab closing isn't synchronously.
Comment 141 Dão Gottwald [:dao] 2009-12-12 09:46:58 PST
But it is.
Comment 142 Dão Gottwald [:dao] 2010-04-23 05:12:03 PDT
*** Bug 561131 has been marked as a duplicate of this bug. ***
Comment 143 Kevin Brosnan 2010-06-14 09:15:37 PDT
*** Bug 569254 has been marked as a duplicate of this bug. ***
Comment 144 Alex Limi (:limi) — Firefox UX Team 2010-06-21 01:30:31 PDT
*** Bug 565544 has been marked as a duplicate of this bug. ***
Comment 145 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2010-10-13 22:11:02 PDT
Web pages still can steal focus, see bug 604289. I'll fix it ASAP.
Comment 146 Matthias Versen [:Matti] 2011-01-04 17:36:23 PST
*** Bug 623066 has been marked as a duplicate of this bug. ***
Comment 147 John P Baker 2011-01-11 03:52:04 PST
*** Bug 624680 has been marked as a duplicate of this bug. ***
Comment 148 Matthias Versen [:Matti] 2011-01-15 17:57:52 PST
*** Bug 626156 has been marked as a duplicate of this bug. ***
Comment 149 John P Baker 2011-03-02 03:00:04 PST
*** Bug 638047 has been marked as a duplicate of this bug. ***

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