As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 777706 - Unable to tap the Twitter sign-in button
: Unable to tap the Twitter sign-in button
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 17 Branch
: ARM Android
: -- normal (vote)
: mozilla17
Assigned To: Nobody; OK to take it and work on it
:
: Andrew Overholt [:overholt]
Mentors:
http://www.twitter.com
Depends on:
Blocks: twitter.com 734503 773431
  Show dependency treegraph
 
Reported: 2012-07-26 07:05 PDT by Aaron Train [:aaronmt]
Modified: 2013-12-10 10:01 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
fixed
+
fixed


Attachments
Backout patch (14.20 KB, patch)
2012-07-31 11:33 PDT, Wesley Johnston (:wesj)
bugs: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image Aaron Train [:aaronmt] 2012-07-26 07:05:40 PDT
Tapping the sign-in button simply flashes the button orange.

--
Nightly (07/26)
Samsung Galaxy Nexus (Android 4.1.1)
Comment 1 User image Aaron Train [:aaronmt] 2012-07-26 07:07:54 PDT
Interesting

E/GeckoConsole( 2792): [JavaScript Error: "Error: Permission denied to access property '0'" {file: "https://mobile.twitter.com/" line: 3965}]
Comment 2 User image Kartikaya Gupta (email:kats@mozilla.com) 2012-07-26 08:57:34 PDT
If this is a recent regression it may have been caused by bug 773427 or bug 773431.
Comment 4 User image Kartikaya Gupta (email:kats@mozilla.com) 2012-07-26 12:14:46 PDT
For some reason having our event listener registered in capture mode instead of bubble mode (which is what the patch from bug 773431 does) results in the event objects touches property to become inaccessible. This is what is causing the script error in comment 1, and what is causing the link to not load. Simplified test page at http://people.mozilla.org/~kgupta/bug/777706.html - click on the text should result in an alert dialog in FF for Android, but currently does not.
Comment 5 User image Kartikaya Gupta (email:kats@mozilla.com) 2012-07-26 12:19:59 PDT
Commenting out the check for "aEvent.touches.length > 1" in browser.js fixes this, so it's probably some sort of javascript protection mechanism. i.e. if we access aEvent.touches from browser.js first then it becomes inaccessible to the content page, but if they access it first then it's all good.
Comment 6 User image Wesley Johnston (:wesj) 2012-07-26 14:59:19 PDT
Smaug pointed out this might be a regression from bug 734503. Backing it out locally to test.
Comment 7 User image Wesley Johnston (:wesj) 2012-07-26 17:04:14 PDT
Backing out bug 734503 kills the security exceptions, but the array method of getting touches (i.e. event.touches[0]) return undefined even though event.touches.length === 1. Going to bug some more people....
Comment 8 User image Wesley Johnston (:wesj) 2012-07-26 17:47:40 PDT
I have almost no idea how this works, but it looks like the dom-bindings are doing some caching of their own. Is that causing us to get something that's been wrapped incorrectly when the page tries to access it later?

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/dombindingsgen.py#95

Will keep looking at this tomorrow.
Comment 9 User image Boris Zbarsky [:bz] (still a bit busy) 2012-07-26 18:23:55 PDT
Hmm.  So the touchlist uses the document as the parent if created via createTouchList.

But if it's gotten off an event it uses the _event_ as the parent.

And I bet we've got no parenting on the event object so the touchlist gets wrapped in the chrome scope, and all's bad after that.

This used to work because the touchlist didn't use to be a wrappercache, so it got separately wrapped in every single scope....

Would it make any sense to use the event target as a parent instead of the event?
Comment 10 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-26 18:26:25 PDT
Which event target?
Comment 11 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-26 18:27:22 PDT
Ok, I realized that's a stupid question, because events can't end up bubbling from chrome to content (I hope).
Comment 12 User image Boris Zbarsky [:bz] (still a bit busy) 2012-07-26 18:31:55 PDT
Events cannot bubble from chrome to content, no.

The right event target is probably the explicitOriginalTarget or whatnot.  Basically, whatever will only be affected by the retargeting when leaving native anonymous content.  Assuming we have such a thing.
Comment 13 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2012-07-27 04:14:06 PDT
There isn't event target when event hasn't been dispatched.
Comment 14 User image Boris Zbarsky [:bz] (still a bit busy) 2012-07-27 08:32:23 PDT
But in those situations you won't have script getting the touches off the event either, right?
Comment 15 User image Boris Zbarsky [:bz] (still a bit busy) 2012-07-27 08:33:08 PDT
I guess events can get redispatched, though.  Hrm.

Can we just associate the event with a window and use that window as the parent here?  I know you were talking about doing that for events in general.
Comment 16 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2012-07-27 09:03:43 PDT
(In reply to Boris Zbarsky (:bz) from comment #15)
> Can we just associate the event with a window and use that window as the
> parent here? 
Yes, that is my plan for ParisBindings+Events, but we need to fix this bug in some other way
asap, I think.
Comment 17 User image Wesley Johnston (:wesj) 2012-07-31 11:33:18 PDT
Created attachment 647612 [details] [diff] [review]
Backout patch

This backs out bug 734503 and the touchlist parts of bug 768669. Using this twitter and kat's test page are fixed.
Comment 18 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2012-08-01 02:59:09 PDT
Do we need the backout also in aurora, or even in beta?
Comment 19 User image Kartikaya Gupta (email:kats@mozilla.com) 2012-08-01 08:05:38 PDT
I would like to have this backed out in aurora (16) and beta (15), because then I can uplift bug 773431 to those channels without regressing twitter.
Comment 21 User image Wesley Johnston (:wesj) 2012-08-02 10:45:15 PDT
Going to wait to make sure these are ok on central before requesting approval. That ok kats?
Comment 22 User image Kartikaya Gupta (email:kats@mozilla.com) 2012-08-02 10:51:14 PDT
Sure, sounds good.
Comment 23 User image Ryan VanderMeulen [:RyanVM] 2012-08-02 19:08:28 PDT
https://hg.mozilla.org/mozilla-central/rev/84ae895140f3
Comment 24 User image Matt Brubeck (:mbrubeck) 2012-08-06 16:37:41 PDT
Ready to request Aurora/Beta approval?
Comment 25 User image Wesley Johnston (:wesj) 2012-08-06 16:45:43 PDT
Comment on attachment 647612 [details] [diff] [review]
Backout patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): regression from 734503 and 768669. backout of the relevent parts of both patches. unnoticed until bug 773431 made it visible. can't move bug 773431 forward without this.
User impact if declined: Any touches that we (or extensions) take in chrome suddenly make them inaccessible by content
Testing completed (on m-c, etc.): landed on mc last week. No regressions I know of
Risk to taking this patch (and alternatives if risky): should just be reverting us to the behavior gecko has had since forever before this
String or UUID changes made by this patch: none
Comment 26 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-08-07 07:39:04 PDT
Comment on attachment 647612 [details] [diff] [review]
Backout patch

approving backout for branches.
Comment 27 User image Wesley Johnston (:wesj) 2012-08-07 09:22:12 PDT
I apologize. This isn't needed on beta.

https://hg.mozilla.org/mozilla-central/rev/84ae895140f3
Comment 28 User image Scoobidiver (away) 2012-08-07 10:51:38 PDT
Landed on Aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/0004e9b268ae

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