299677 - Almost every element can steal focus from another tab with focus or blur method

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Martijn Wargers (dead)]

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 (dead)]

Attachment: 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 (dead)]

Attachment: testcase2, using blur

Same testcase, but now using .blur() which has the same effect.

Comment 3

[Jaime Mitchell (use [e-mail] for email)]

fgdfgdd

Comment 4

[Jaime Mitchell (use [e-mail] for email)]

(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][⌚️UTC+1]]

asda

Comment 6

[J.P.Klippel]

observed with:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050703 Firefox/1.0+

Comment 7

[Martijn Wargers (dead)]

Attachment: patcha

This patch prevents the blur() method from stealing focus on the -already
patched for focus- elements.

Comment 8

[Martijn Wargers (dead)]

Attachment: patchb

This prevents focus stealing on every other html element.

Comment 9

[Martijn Wargers (dead)]

Attachment: 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 (dead)]

Brian, basically Boris told me to cc you. Any suggestions?

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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)]

cc'ing aaron in case he has any thoughts on these patches

Comment 13

[Brian Ryner (not reading)]

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 (dead)]

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)]

Comment on attachment 189796 [details] [diff] [review]
patcha

jst, look ok? (you added the similar checks for focus())

Comment 16

[Aaron Leventhal]

Comment on attachment 189796 [details] [diff] [review]
patcha

Is the visibility check in nsIFrame::IsFocusable() not enough?

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 189797 [details] [diff] [review]
patchb

sr=jst

Comment 18

[Martijn Wargers (dead)]

(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)]

(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]]

please land this on the trunk and we'll evaluate for branch after it's proved
itself. 

Comment 21

[Brian Ryner (not reading)]

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)]

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 [:bzbarsky]]

Could this possibly have caused bug 306076?

Comment 24

[Brian Ryner (not reading)]

Checked in patcha and patchb on the branch.

Comment 25

[Uri Bernstein]

This might have caused bug 306173 (Mac only?)

Comment 26

[Daniel Veditz [:dveditz]]

Is this really a problem in ff1.0/1.7? The attached testcases don't seem to work there.

Comment 27

[Martijn Wargers (dead)]

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]]

(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 (dead)]

Hmm, actually, I haven't been able to make a testcase that shows this bug in Mozilla1.7.12.

Comment 30

[Jesse Ruderman]

What did you try? -moz-user-focus?

Comment 31

[Martijn Wargers (dead)]

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]]

per comments, not needed in old branches

Comment 33

[Martijn Wargers (dead)]

Ugh, I forgot to fix nsHTMLButtonElement.cpp. 

Comment 34

[Martijn Wargers (dead)]

Attachment: 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]]

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]]

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 (dead)]

(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]]

Comment on attachment 216353 [details] [diff] [review]
button patch

sr=dveditz

Comment 39

[Tim Riley [:timr]]

jst- can we get a review on this!

Comment 40

[Brian Ryner (not reading)]

Comment on attachment 216353 [details] [diff] [review]
button patch

looks good

Comment 41

[Daniel Veditz [:dveditz]]

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)]

follow-up button fix checked into trunk, 1.8 branch, and 1.8.0 branch.