Works around a crashing browser bug affecting Android browser 2.3.x on some devices #1019
Conversation
…id 2.3.5. The bug was triggered by code in effects. Fixes #12497. A simple example (without jQuery) of JS that will trigger this crashing browser bug is here: http://jsfiddle.net/wCQvh/ This browser bug appears to have been resolved in Android 2.3.5. Unfortunately, there are a large number of active devices that cannot and/or will not be upgraded. The workaround in this commit preserves the same logic as the previous code but avoids triggering the browser bug.
| @@ -81,7 +81,7 @@ function Animation( elem, properties, options ) { | |||
| tick = function() { | |||
| var currentTime = fxNow || createFxNow(), | |||
| remaining = Math.max( 0, animation.startTime + animation.duration - currentTime ), | |||
| percent = 1 - ( remaining / animation.duration || 0 ), | |||
| percent = animation.duration ? (1 - (remaining / animation.duration)) : 1, | |||
There was a problem hiding this comment.
Two points:
- The existing logic ensured a numeric
percentin all cases, but this change makes it possible to get NaN. I don't think we care, but it's worth pointing out that there is in fact a functional change. - I was unable to reproduce your problem with the emulated sub-2.3.5 clients available on BrowserStack... could you be more specific about the error you encounter? Could it be fixed just by wrapping the division in parentheses instead of introducing a ternary expression?
There was a problem hiding this comment.
The error is a segfault. The following is output from the LG Esteem that exhibits the behavior: https://gist.github.com/33772b893b28f543484b I assume the behavior is similar for the other devices mentioned in the bug thread, but I don't have those particular devices on hand to test.
I also hoped that wrapping the division in parens would avoid the bug, but the crashing behavior persisted.
When I tried removing the || 0 without other changes, there was a test failure ("Animations with 0 duration don't ease (#12273)"). With my proposed fix, the test remained green.
If an alternative fix would be preferable, I'd be happy to test it on this device.
There was a problem hiding this comment.
Is it possible that this is a device specific issue... Can anyone confirm 2.3.4 on another device?
There was a problem hiding this comment.
It looks like a number of people have already confirmed the crashing behavior with animate on several devices and versions of 2.3.x in the bug thread: http://bugs.jquery.com/ticket/12497
|
I'm not even sure what exactly we are fixing... What is causing this crash? division? the opti? - I tested this on my Android 2.1-update1 phone and none of the fiddles fail for me... |
|
@gnarf37 I can't say for sure what is causing the segfault. This fiddle (http://jsfiddle.net/wCQvh/) was the simplest case I could get to trigger the crash. |
|
FYI, updated the title and description of the PR to more accurately reflect the affected population. |
|
The above and the previous version leave If the intention is to support undefined duration, the following would be significantly more readable: if (animation.duration > 0) {
remaining = Math.max( 0, animation.startTime + animation.duration - currentTime );
percent = 1 - (remaining / animation.duration);
} else {
remaining = 0;
percent = 1;
} |
|
My colleague succeeded in simplifying the crash-triggering fiddle even further: http://jsfiddle.net/wCQvh/24/ |
|
Worked for me on a Nexus One (Android 2.3.4), this is getting stranger by the minute. I changed to an alert just to be sure. http://jsfiddle.net/wCQvh/30/ @lukemelia is this a stock phone with a standard carrier software release? |
|
@dmethvin Yes, stock phone. Model No: LG-MS910, Android version: 2.3.4 Pretty bizarre. |
|
@lukemelia does dave's fiddle crash for you too? this seems insane Like which of these crashes: http://jsfiddle.net/Rms2H/ |
|
Add http://jsfiddle.net/Rms2H/4/ to the list too please |
|
I have a stock LG-P690F running 2.3.4. dmethvin's crashes, but none of yours do gnarf37. |
|
Yes that crashes. |
|
Sorry to keep pushing more into the "LG Test Queue" but thanks for running these http://jsfiddle.net/Rms2H/6/ Just want to confirm what is actually causing the issue |
|
No worries, I'm happy to help and I'm intrigued by this bug. Those both work. |
|
http://jsfiddle.net/aMvm6/1/ works too, so you don't need two variables. |
|
Next up - against actual jQuery edge, jQuery with the http://jsfiddle.net/nePkZ/1/ |
|
1 crashes, 3 & 4 seem to work |
…iousdannii - Closes gh-1019 (cherry picked from commit 8773067)
|
hi, i got android phone 2.3.5 (k-touch w655), without java with phone connected by 3g or wifi, everytime i start stock browser it crashes (unexpectably, force close) with phone rebooted and NOT connected by 3g or wifi, everytime i try to access stock browser settings it crashes (unexpectably, force close) if then i connect by 3g or wifi, it crashes inmediately (unexpectably, force close) i tried reinstalling same/other version browser.apk or uninstall it (not possible), editing com.android.browser_preferences.xml, deleting browser and dalvik cache, set miren browser as default (i can not establish other browser as default, not possible delete/change associations)... without success. links to internet try to open stock browser which is default but then crash where is the error? thanks 4 all. i get this system_app_crash@ log file: Process: com.android.browser java.lang.RuntimeException: Unable to start activity ComponentInfo{com.android.browser/com.android.browser.BrowserPreferencesPage}: android.view.InflateException: Binary XML file line #210: Error inflating class java.lang.reflect.Constructor |
This pull request works around a crashing browser bug affecting Android browser 2.3.x on some devices. The bug was triggered by code in
animatethat was introduce since 1.7.2.A bug against jQuery was filed here a couple of months ago: http://bugs.jquery.com/ticket/12497. The bug thread references a number of devices that exhibit this crash.
A simple example (without jQuery) of JS that will trigger this crashing browser bug is here: http://jsfiddle.net/wCQvh/
The workaround in this commit attempts to preserve the same logic as the previous code but avoids triggering the browser bug.