313337 - Fastclick popup gets around popup blocking by abusing the onchange event

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Jesse Ruderman]

From bug 176079.  I was able to reproduce once on Mac.


------- Bug 176079 comment 89 From David Barr -------

Ucomics has figured out a way around the fix...
http://www.ucomics.com/forbetterorforworse/2005/10/11/


------- Bug 176079 comment 90 From Bob Clary -------

This appears to be a fastclick "innovation". It is opening popunder windows on
page unload using flash. I also see this on http://wsls.com/ which is also
fastclick related. See this in today's ff 1.8 branch build. And yes, I do have
privacy.popups.disable_from_plugins set to 2.

Comment 1

[Jesse Ruderman]

I'm not convinced this is Flash-related based on my partially simplified
testcase.  It's possible that Bob and I are seeing different exploits, though.

Comment 2

[Jesse Ruderman]

Attachment: partially simplified testcase

Comment 3

[Jesse Ruderman]

Attachment: a little simpler

Comment 4

[Jesse Ruderman]

Attachment: simplified testcase

Comment 5

[Jesse Ruderman]

IE doesn't fire onchange in this case, because the value of the textbox was set
by script.  Firefox probably shouldn't either.

Comment 6

[Asa Dotzler [:asa]]

plussing this. I think this might be the last major case where pop-ups are
getting around our blocker. We may not get this but I don't want it to fall off
our radar.

Comment 7

[Jesse Ruderman]

Attachment: patch

Remaining issues:

* I'm not sure where in SetValue this should go.  For example, should it be inside the big |if| or outside of it?

* This causes onchange to not fire if you use autocomplete and then un-focus the textbox, probably for the same reason using autocomplete doesn't fire an oninput event.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 200483 [details] [diff] [review]
patch

Please use the -p flag for diffs; makes it a lot easier to see what's going on...

First of all, this seems like it would cause a pretty nontrivial a performance issue.  InitFocusedValue is slow.  Very slow, for anything nontrivial (try this very textarea with a comment that has some lines in it).  Given all the sites that set values on text controls that users don't actually interact with, we probably should not be running InitFocusedValue() unless we're in the unlikely case of this bug -- setting the value on a text control that currently has focus.

Also, breaking autocomplete firing onchange is no good.  That should be fixed before we take this patch.

Comment 9

[Asa Dotzler [:asa]]

We're very short on time for this release and we'd need a very safe fix to consider for 1.5. Setting blocking flag back to ?

Comment 10

[Jesse Ruderman]

I think the problem with form autocomplete is that autocomplete uses nsFormFillController::SetTextValue and nsFormFillController::SetTextValue calls SetValue directly.  I don't know the right way to fix that and I don't know whether other autocomplete (e.g. address bar) is broken in a similar way.

bz suggested setting mFocusedValue directly instead of calling InitFocusedValue.  That still does a copy but it at least avoids serializing a DOM.

Comment 11

[Daniel Veditz [:dveditz]]

I'm not getting popups from these testcases. I do from the original page when I click on a link, but that's a different issue (they've got an onclick handler).

Comment 12

[Jesse Ruderman]

If you open open the testcase in a background tab, it won't work until you reload due to bug 307933.

Comment 13

[Jesse Ruderman]

bz says messing with focus and onchange this close to Firefox 1.5 would be risky.

Comment 14

[Daniel Veditz [:dveditz]]

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

Comment 15

[Mike Schroepfer]

JST - any thoughts on this?

Comment 16

[Johnny Stenback (:jst)]

Attachment: Wrong diff...

This is the earlier patch for this bug with bz's suggestions in comment #8.

Comment 17

[Johnny Stenback (:jst)]

Attachment: Updated patch based on bz's comments.

Right diff this time.

Comment 18

[Darin Fisher]

Sicking: review this puppy!

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sorry about the late review, it got burried in my review queue.

sr=sicking

Comment 20

[Johnny Stenback (:jst)]

Fix landed on the trunk.

Comment 21

[Darin Fisher]

Pushing this out to beta2 so it can bake on the trunk a little more.

Comment 22

[Bryan]

Still get the random Fastclick.net pop-up browsing http://www.screamscape.com/ using the latest branch build: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060714 BonEcho/2.0b1 ID:2006071414

~B

Comment 23

[Krol Wojtilla]

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

Comment 24

[Bob Clary [:bc] (inactive)]

jst, was this checked into 1.8?

Comment 25

[Bryan]

(In reply to comment #24)
> jst, was this checked into 1.8?
> 

Bob, I'm not sure as I still see this at a number of sites.

~B