Webpage-JS can steal focus from URLbar / chrome

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
16 years ago
2 months ago

People

(Reporter: Gili, Assigned: masayuki)

Tracking

(Depends on: 3 bugs, Blocks: 3 bugs)

Trunk
mozilla1.9.3a1
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: parity-ie, URL)

Attachments

(2 attachments, 16 obsolete attachments)

236 bytes, text/html
Details
18.27 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
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

16 years ago
Do you mean the caret?  That's what Ctrl-L does.  This seems invalid.
(Reporter)

Comment 2

16 years ago
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

16 years ago
->bryner for triage, cc aaronl. 
OS: OS/2 → All
Hardware: PC → All

Comment 4

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

Status: UNCONFIRMED → NEW
Ever confirmed: true
urlbar
Assignee: trudelle → hewitt
Component: XP Apps → URL Bar
QA Contact: sairuh → claudius

Comment 6

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

Updated

15 years ago
Blocks: 140346
*** Bug 143973 has been marked as a duplicate of this bug. ***

Comment 8

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

Updated

15 years ago
Blocks: 55416

Updated

15 years ago
No longer blocks: 140346

Comment 9

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

Updated

15 years ago
Blocks: 140346

Comment 10

14 years ago
*** Bug 230648 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 11

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

Comment 12

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

*** Bug 271565 has been marked as a duplicate of this bug. ***
Assignee: hewitt → location-bar
QA Contact: claudius
Google.com loads pretty quickly for testing this. Slower loading testcase coming up.
Severity: normal → major
Keywords: helpwanted, testcase
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.
(Reporter)

Comment 16

13 years ago
+1

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

Comment 17

13 years ago
As much as I hate focus stealing, this is definitely not a "major loss of function".
Severity: major → normal

Updated

13 years ago
Blocks: 83552

Comment 18

13 years ago
*** Bug 229007 has been marked as a duplicate of this bug. ***
*** Bug 274747 has been marked as a duplicate of this bug. ***
*** Bug 302671 has been marked as a duplicate of this bug. ***

Comment 21

12 years ago
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.
(Reporter)

Comment 22

12 years ago
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

12 years ago
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!

Updated

12 years ago
Blocks: 273592

Updated

12 years ago
No longer blocks: 273592

Updated

12 years ago
Blocks: 295056

Comment 24

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

Comment 25

12 years ago
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

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

Comment 27

11 years ago
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.
(Reporter)

Comment 28

11 years ago
+1

I agree with #27
Duplicate of this bug: 367440

Updated

10 years ago
Duplicate of this bug: 370094

Comment 31

10 years ago
Created attachment 254761 [details]
Testcase
Attachment #166947 - Attachment is obsolete: true

Updated

10 years ago
Keywords: testcase
Summary: JS shouldn't steal focus when in URL bar → Webpage-JS can steal focus from URLbar / chrome

Comment 32

10 years ago
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.
Assignee: location-bar → nobody
Component: Location Bar → Layout: Form Controls
QA Contact: layout.form-controls

Updated

10 years ago
Duplicate of this bug: 377011

Comment 34

10 years ago
UI elements such as location should have a higher focus priority than page elements.
Duplicate of this bug: 359549

Updated

9 years ago
Duplicate of this bug: 417915

Comment 37

9 years ago
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.

Updated

9 years ago
Depends on: 419061
Duplicate of this bug: 164339
Duplicate of this bug: 445050
Duplicate of this bug: 124191

Comment 41

9 years ago
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.

Updated

9 years ago
Duplicate of this bug: 449650

Updated

9 years ago
Flags: wanted1.9.1?
Whiteboard: parity-ie
Flags: wanted1.9.1? → wanted1.9.1+
Duplicate of this bug: 475375
Duplicate of this bug: 466341
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.
adding focus refactoring owner to cc list.
test builds:
https://build.mozilla.org/tryserver-builds/2009-05-13_20:15-masayuki@d-toybox.com-kill-focus-robber/
Duplicate of this bug: 496502
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.
Assignee: nobody → masayuki
Attachment #377320 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #383063 - Flags: review?(enndeakin)
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.
Attachment #383063 - Flags: review?(enndeakin) → review-
(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/
Neil, would you check my comment 51?
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 ...
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.
Attachment #383063 - Attachment is obsolete: true
Attachment #385343 - Flags: review?(enndeakin)
> 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.
(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.
> 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 on attachment 385343 [details] [diff] [review]
Patch v2.0

- for now, waiting for tests
Attachment #385343 - Flags: review?(enndeakin) → review-
Duplicate of this bug: 503643
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.
Attachment #385343 - Attachment is obsolete: true
Attachment #390238 - Flags: review?(enndeakin)
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.
(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?
Created attachment 390418 [details] [diff] [review]
Patch v3.1
Attachment #390238 - Attachment is obsolete: true
Attachment #390418 - Flags: review?(enndeakin)
Attachment #390238 - Flags: review?(enndeakin)
> 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.
(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 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
Attachment #390418 - Flags: review?(enndeakin) → review+
Created attachment 390833 [details] [diff] [review]
Patch v3.1.1

Thank you, Neil.
Attachment #390418 - Attachment is obsolete: true
Attachment #390833 - Flags: superreview?(Olli.Pettay)
Attachment #390833 - Flags: review+
Keywords: helpwanted
Duplicate of this bug: 507107
Olli: Would you sr the patch?

Updated

8 years ago
Attachment #390833 - Flags: superreview?(Olli.Pettay) → superreview+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/8366e5cc9f57
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
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
The next run turned out orange with another timeout:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1249199698.1249204534.800.gz
OK, I backed the patch out:

http://hg.mozilla.org/mozilla-central/rev/94e882183752
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The cycle after the backout turned green:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1249209899.1249214579.25739.gz
Ehsan: Is the patch really the cause? Because the patch changes the XP level behavior, not only for Win32.
(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.)
(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.
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.
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.
Please note that the last orange contains two failures.  Bug 498339 is about the mochitest-browser-chrome failure.
(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.  :-(
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#
(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.
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.
I guess that the performance regression might increase the possibility of the intermittent failure...
> 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.
(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

8 years ago
Ubuntu Bug:
https://bugs.launchpad.net/bugs/239831
"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).
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.)
Point of comment 89 was that I seriously doubt the security check would be a performance problem here....
Created attachment 393195 [details] [diff] [review]
Patch v4.0
Attachment #390833 - Attachment is obsolete: true
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.
Attachment #393195 - Attachment is obsolete: true
Attachment #393202 - Flags: review?(enndeakin)
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.
(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.
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?
One or two calls to CanCallerAccess per pageload are not going to move the Tp needle.
Note: The tp scores on tryserver were:

without patch or with the latest patch : about 217
with old patch: about 220
(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.
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.
So basically < 1 time per pageload?

There's no way it's causing a 1% Tp regression, if so.
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"...
(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...
Ugh... but the my latest patch faster...
The latest patch scores on tryserver:

tp: 217.22
tp: 216.36

I bet the tp regression causes the cost of nsContentUtils::CanCallerAccess...
Created attachment 393732 [details] [diff] [review]
Patch v4.1

this is little simpler than the v4.0.1.
Attachment #393202 - Attachment is obsolete: true
Attachment #393202 - Flags: review?(enndeakin)
this may be causing another problem with extensions like Firebug and Twitterfox that have editable textfields.

see bug 509478.

Updated

8 years ago
Depends on: 509478
(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?
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! :)
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.
ok, thanks Masayuki. I'm seeing the problem in today's nightly. That saved me a build.

Updated

8 years ago
No longer depends on: 509478
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
Attachment #393732 - Attachment is obsolete: true
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.
Attachment #394621 - Attachment is obsolete: true
Attachment #395023 - Flags: review?(enndeakin)
(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.
(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.
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?
It's a different pageset.  I think the harness is the same.

Updated

8 years ago
Duplicate of this bug: 513390
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.
Attachment #395023 - Attachment is obsolete: true
Attachment #395023 - Flags: review?(enndeakin)

Comment 120

8 years ago
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
Paul Bailey, this is a good point, but not the same as this bug. Can you file a new one, please?
The patch will forbid to steal focus from another document.

Comment 123

8 years ago
Ben Bucksch, good idea. I added bug 532097, with some more thought into what should be fixed.
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|.
Attachment #397823 - Attachment is obsolete: true
Attachment #415556 - Flags: review?(enndeakin)
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.
Attachment #415556 - Flags: review?(enndeakin) → review+
(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.
(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.
(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?
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.
Attachment #415556 - Attachment is obsolete: true
Attachment #416682 - Flags: review+
Status: REOPENED → ASSIGNED
Whiteboard: parity-ie → parity-ie [needs landing]
Keywords: checkin-needed
Whiteboard: parity-ie [needs landing] → parity-ie
Target Milestone: mozilla1.9.2a1 → mozilla1.9.3a1
A better fix for that is to prevent the panel from opening by cancelling either the click or popupshowing events.
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...
Why is this in browser/base/content/test/ anyway, rather than dom/tests/browser/ for instance?
Created attachment 417025 [details] [diff] [review]
Patch v5.2

O.K. The new test file is moved to dom/tests/browser.
Attachment #416682 - Attachment is obsolete: true
Attachment #417025 - Flags: review+
Created attachment 417026 [details] [diff] [review]
Patch v5.2

Oops...
Attachment #417025 - Attachment is obsolete: true
Attachment #417026 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/a8f46161c891
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Keywords: checkin-needed

Comment 136

8 years ago
Hey, having just fixed this, it might make sense to fix Bug 532097 while you know this portion of the code well.
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?
(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.
Which animation?
So, I guessed the tab closing isn't synchronously.
But it is.

Updated

8 years ago
Depends on: 534840
Blocks: 535903

Updated

8 years ago

Updated

8 years ago
Depends on: 535970
No longer blocks: 535903
Depends on: 535903
Blocks: 561131

Updated

7 years ago
Blocks: 561125

Updated

7 years ago
No longer blocks: 561131
Duplicate of this bug: 561131

Updated

7 years ago
Blocks: 565510
No longer blocks: 561125

Updated

7 years ago
Duplicate of this bug: 569254
Duplicate of this bug: 565544
Depends on: 591890
Component: Layout: Form Controls → DOM: Other
QA Contact: layout.form-controls → general
Depends on: 604289
Component: DOM: Other → DOM: Core & HTML
Web pages still can steal focus, see bug 604289. I'll fix it ASAP.
Duplicate of this bug: 623066

Updated

7 years ago
Duplicate of this bug: 624680
Duplicate of this bug: 626156

Updated

6 years ago
Duplicate of this bug: 638047
Depends on: 656026

Updated

3 years ago
Depends on: 976673
You need to log in before you can comment on or make changes to this bug.