Last Comment Bug 299677 - Almost every element can steal focus from another tab with focus or blur method
: Almost every element can steal focus from another tab with focus or blur method
Status: RESOLVED FIXED
[sg:fix]
: fixed1.8, fixed1.8.0.4, fixed1.8.1, testcase
Product: SeaMonkey
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: x86 Windows XP
: -- normal with 2 votes (vote)
: mozilla1.8beta4
Assigned To: Brian Ryner (not reading)
:
Mentors:
Depends on: 124750 306173
Blocks: 140346
  Show dependency treegraph
 
Reported: 2005-07-04 15:13 PDT by Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
Modified: 2008-07-31 04:30 PDT (History)
16 users (show)
dveditz: blocking1.7.13-
dveditz: blocking‑aviary1.0.8-
asa: blocking1.8b5+
dveditz: blocking1.8.0.4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
testcase (634 bytes, text/html)
2005-07-04 15:19 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details
testcase2, using blur (634 bytes, text/html)
2005-07-04 15:21 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details
patcha (3.43 KB, patch)
2005-07-19 11:54 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
bryner: review+
jst: superreview+
asa: approval1.8b4+
Details | Diff | Review
patchb (1.13 KB, patch)
2005-07-19 11:56 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
bryner: review+
jst: superreview+
asa: approval1.8b4+
Details | Diff | Review
patchc (2.41 KB, patch)
2005-07-19 12:11 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
bryner: review-
Details | Diff | Review
button patch (1010 bytes, patch)
2006-03-26 16:11 PST, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
bryner: review+
dveditz: superreview+
bryner: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Review

Description Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2005-07-04 15:13:16 PDT
See upcoming testcase.
To reproduce:
- Open testcase in new tab in background.
- Try to type some text in the bugzilla textarea.
You'll see that the result of you typing shows up in the testcase's tab.
Comment 1 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2005-07-04 15:19:11 PDT
Created attachment 188245 [details]
testcase

This uses the <span> element. But it doesn't matter which element I use,
because with tabindex="1" or overflow:auto or -moz-user-focus:normal for xul I
can make virtually every element focusable.
Comment 2 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2005-07-04 15:21:32 PDT
Created attachment 188246 [details]
testcase2, using blur

Same testcase, but now using .blur() which has the same effect.
Comment 3 Jaime Mitchell (use bugmail@jaimem.org.uk for email) 2005-07-04 15:22:25 PDT
fgdfgdd
Comment 4 Jaime Mitchell (use bugmail@jaimem.org.uk for email) 2005-07-04 15:24:59 PDT
(In reply to comment #3)
> fgdfgdd

Sorry was trying the test case and cc myself.

With
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050703
Firefox/1.0+ 

I find that trying to type into the bugzilla text box also activates the FATY
toolbar, the text then disapears from the FAYT every second and is copied to the
other tab.
Comment 5 Henrik Skupin (:whimboo) 2005-07-04 16:11:58 PDT
asda
Comment 6 J.P.Klippel 2005-07-05 09:44:55 PDT
observed with:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050703 Firefox/1.0+
Comment 7 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2005-07-19 11:54:09 PDT
Created attachment 189796 [details] [diff] [review]
patcha

This patch prevents the blur() method from stealing focus on the -already
patched for focus- elements.
Comment 8 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2005-07-19 11:56:06 PDT
Created attachment 189797 [details] [diff] [review]
patchb

This prevents focus stealing on every other html element.
Comment 9 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2005-07-19 12:11:21 PDT
Created attachment 189806 [details] [diff] [review]
patchc

This patch adds a check if the window has focus when an element calls focus()
or blur().
I've more or less randomly copied it from somewhere else.

Patcha fixes bug 138646.
The combination of patcha and patchc fixes bug 258285. I suspect it could also
fix bug 187083, since I get the same phenomenon when I try out "minimized test
case" in bug 258285.
Comment 10 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2005-07-19 16:02:01 PDT
Brian, basically Boris told me to cc you. Any suggestions?
Comment 11 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-08-11 11:22:23 PDT
Re-assigning to bryner as per drivers' security bug pass.

Brian: can you take a look at the attached patch, and drive this in for 1.8b4?
Comment 12 Brian Ryner (not reading) 2005-08-19 10:01:33 PDT
cc'ing aaron in case he has any thoughts on these patches
Comment 13 Brian Ryner (not reading) 2005-08-19 10:19:31 PDT
Comment on attachment 189806 [details] [diff] [review]
patchc

This seems like it would prevent, among other things, putting focus onto an
element in an iframe if the iframe isn't focused.  I can imagine that breaking
a lot of things.  I haven't applied this patch to test though... does what I
said actually happen?
Comment 14 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2005-08-19 10:47:37 PDT
Yes, I think you're right. That's probably what patchc does. So it isn't any
good. When I tested it at that time, I didn't test it very good.
I didn't test it for the situation you described.
Comment 15 Brian Ryner (not reading) 2005-08-22 16:47:19 PDT
Comment on attachment 189796 [details] [diff] [review]
patcha

jst, look ok? (you added the similar checks for focus())
Comment 16 Aaron Leventhal 2005-08-23 06:57:28 PDT
Comment on attachment 189796 [details] [diff] [review]
patcha

Is the visibility check in nsIFrame::IsFocusable() not enough?
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2005-08-23 16:12:19 PDT
Comment on attachment 189797 [details] [diff] [review]
patchb

sr=jst
Comment 18 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2005-08-23 16:18:07 PDT
(In reply to comment #16)
> (From update of attachment 189796 [details] [diff] [review] [edit])
> Is the visibility check in nsIFrame::IsFocusable() not enough?
Aaron, I don't know. I hope Brian of jst can answer that question.
Comment 19 Brian Ryner (not reading) 2005-08-23 21:49:24 PDT
(In reply to comment #16)
> (From update of attachment 189796 [details] [diff] [review] [edit])
> Is the visibility check in nsIFrame::IsFocusable() not enough?

That doesn't get invoked for this code path, as far as I can tell (only seems 
to be used for tabbing).  Seems like we should unify these checks, but I'd go 
with consistency between focus() and blur() for the moment.
Comment 20 Asa Dotzler [:asa] 2005-08-24 11:50:18 PDT
please land this on the trunk and we'll evaluate for branch after it's proved
itself. 
Comment 21 Brian Ryner (not reading) 2005-08-24 13:46:53 PDT
This ("patcha" and "patchb") landed on the trunk yesterday.  I'd like to verify
whether the original testcases are fixed without "patchc" which I think would
break other things.
Comment 22 Brian Ryner (not reading) 2005-08-24 18:21:53 PDT
Verified that the patches landed fix the two testcases in the bug.  I think this
is safe to land on the branch.
Comment 23 Boris Zbarsky [:bz] 2005-08-26 20:00:12 PDT
Could this possibly have caused bug 306076?
Comment 24 Brian Ryner (not reading) 2005-08-27 17:17:35 PDT
Checked in patcha and patchb on the branch.
Comment 25 Uri Bernstein (Google) 2005-08-31 10:03:40 PDT
This might have caused bug 306173 (Mac only?)
Comment 26 Daniel Veditz [:dveditz] 2006-02-02 14:29:36 PST
Is this really a problem in ff1.0/1.7? The attached testcases don't seem to work there.
Comment 27 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-02-02 14:35:02 PST
Yes, sorry, the testcase uses a feature that isn't in 1.7 builds, e.g. setting tabindex attribute makes the element focusable.
I could quite easily make a testcase that also works in 1.7 builds, though.
So this is really a problem in ff1.0/1.7.
Comment 28 Daniel Veditz [:dveditz] 2006-02-07 14:31:05 PST
(In reply to comment #27)
> I could quite easily make a testcase that also works in 1.7 builds, though.
> So this is really a problem in ff1.0/1.7.

Could you do that please? Would help ensure this fix gets tested that it's backported correctly
Comment 29 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-02-08 16:03:00 PST
Hmm, actually, I haven't been able to make a testcase that shows this bug in Mozilla1.7.12.
Comment 30 Jesse Ruderman 2006-02-08 17:00:06 PST
What did you try? -moz-user-focus?
Comment 31 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-02-08 17:10:28 PST
Yes, also tried it with anchors and select elements, I just can't get the bug to show in Mozilla1.7.12
Comment 32 Daniel Veditz [:dveditz] 2006-02-10 11:42:56 PST
per comments, not needed in old branches
Comment 33 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-03-26 16:04:06 PST
Ugh, I forgot to fix nsHTMLButtonElement.cpp. 
Comment 34 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-03-26 16:11:47 PST
Created attachment 216353 [details] [diff] [review]
button patch

Patch that should fix the button blur case.
I'm wondering why nshtmlbuttonelement.cpp and others have ::blur and ::focus methods, since they all look the same as nsgenerichtmlelement.cpp.
I think they could safely be removed, couldn't they?
Comment 35 Tim Riley [:timr] 2006-03-27 09:44:15 PST
Removed QAWanted.  This was from dveditz on 2006-02-01 and we have since minused it for 1.0.x/1.7.x.  Let us know if anyone still wanted QAWanted.
Comment 36 Daniel Veditz [:dveditz] 2006-03-31 12:20:23 PST
Could we get more guidance on why this should go into a .0.x release? The benefit vs. risk picture is unclear.
Comment 37 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-03-31 12:35:04 PST
(In reply to comment #36)
> Could we get more guidance on why this should go into a .0.x release? The
> benefit vs. risk picture is unclear.
There should be no risk here. As you can see in "patcha" and "patchb", the other elements alrady got the same treatment.
Without this patch, there is some risk that some website could steal sensitive information.

Comment 38 Daniel Veditz [:dveditz] 2006-04-03 11:44:25 PDT
Comment on attachment 216353 [details] [diff] [review]
button patch

sr=dveditz
Comment 39 Tim Riley [:timr] 2006-04-10 12:12:35 PDT
jst- can we get a review on this!
Comment 40 Brian Ryner (not reading) 2006-04-19 11:51:46 PDT
Comment on attachment 216353 [details] [diff] [review]
button patch

looks good
Comment 41 Daniel Veditz [:dveditz] 2006-04-20 12:02:53 PDT
Comment on attachment 216353 [details] [diff] [review]
button patch

approved for 1.8.0 branch, a=dveditz for drivers
Comment 42 Brian Ryner (not reading) 2006-04-20 19:14:34 PDT
follow-up button fix checked into trunk, 1.8 branch, and 1.8.0 branch.

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