Skip to content

Ajax: Account for Android 2.3 not firing window.onerror on script errors #2458

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
mgol wants to merge 1 commit into from
Closed

Ajax: Account for Android 2.3 not firing window.onerror on script errors #2458

mgol wants to merge 1 commit into from

Conversation

@mgol
Copy link
Member

@mgol mgol commented Jul 8, 2015

Android 2.3 doesn't fire the window.onerror handler, just accept the reality
there and skip the test.

Refs gh-1573
Refs gh-1786

TBH, this seems pretty bad. My initial attempt was to add the code block at the end of the test but then, once the following:

jQuery.ajax({
    url: "data/badjson.js",
    dataType: "script",
    throws: true
});

is invoked, jQuery.active stays >=1 forever. This may affect all subsequent AJAX requests, right? If I understand it correctly, one error in a script loaded via jQuery.ajax with dataType: "script" breaks all subsequent jQuery.ajax uses, no ajaxStart & ajaxStop events will fire.

cc @jaubourg

@mgol mgol mentioned this pull request Jul 8, 2015
Closed
@jaubourg
Copy link
Member

jaubourg commented Jul 8, 2015

The ajax code (and thus the maintaining of the active count) is exception-proof as far as conversions are concerned.

Every conversion (including script to execution) is try/catch protected. Even if we didn't have this, given we use a script tag in globalEval (used for same-domain scripts), the error is not actually thrown from within the ajax code anyway.

(as a side note, this throws option is a remnant from the past and is not used in the ajaxTest code anymore due to said change in globalEval, so it can be safely removed)

For cross-domain scripts, we only had issues with browsers that did not fire error events on script tags, but I'm pretty sure it's not a thing anymore with our current support grid.

@mgol
Copy link
Member Author

mgol commented Jul 8, 2015

@jaubourg I don't fully understand. This test indicates that Android 2.3 doesn't fire window.onerror for script errors in dynamically inserted scripts (even non-cross-domain) i.e. what jQuery.ajax({..., dataType: 'script'}) does. When I just ignore that window.onerror doesn't fire in Android 2.3 in such a case, I'm left with jQuery.active === 1 which then breaks subsequent tests. What's your take on this?

EDIT: typo corrected.

@mgol mgol force-pushed the android2.3-ajax branch from 0921c44 to 2e38938 Compare July 8, 2015 13:07
@jaubourg
Copy link
Member

jaubourg commented Jul 8, 2015

So ajaxdoesn't throw (thanks to the try/catch I mentioned) and doesn't notify an error that can be inspected with a fail handler.

I'm beginning to suspect this wonderful browser simply halts the execution in case of an error but you'd need to test globalEval with a script that throws an exception in Android to be sure. Something we have no test for in unit/core.js btw, unless I'm missing something.

@mgol mgol mentioned this pull request Jul 20, 2015
Closed
@mgol mgol removed the Needs review label Jul 28, 2015
@mgol
Copy link
Member Author

mgol commented Jul 28, 2015

Per yesterday's meeting, we're cantfixing #1786 on both branches so this PR is good to go (minus the Fixes reference that needs to be changed to Refs).

@mgol mgol self-assigned this Jul 28, 2015
@mgol mgol added this to the 3.0.0 milestone Jul 28, 2015
@mgol mgol added the Ajax label Jul 28, 2015
@mgol
Ajax: Account for Android 2.3 not firing window.onerror on script errors
ec43c27 
Android 2.3 doesn't fire the window.onerror handler, just accept the reality
there and skip the test.

Refs jquerygh-1573
Refs jquerygh-1786
Refs jquery/jquery.com#108
@mgol mgol force-pushed the android2.3-ajax branch from 2e38938 to ec43c27 Compare July 28, 2015 11:13
@mgol mgol closed this in 6044fb6 Jul 28, 2015
@mgol mgol deleted the android2.3-ajax branch July 28, 2015 11:20
mgol added a commit that referenced this pull request Jul 28, 2015
@mgol
Ajax: Account for Android 2.3 not firing window.onerror on script errors
b3eb2a1 
Android 2.3 doesn't fire the window.onerror handler, just accept the reality
there and skip the test.

(cherry-picked from 6044fb6)

Refs gh-1573
Refs gh-1786
Refs jquery/jquery.com#108
Closes gh-2458
riichard pushed a commit to riichard/jquery that referenced this pull request Sep 20, 2015
@mgol @riichard
Ajax: Account for Android 2.3 not firing window.onerror on script errors
c3d2202 
Android 2.3 doesn't fire the window.onerror handler, just accept the reality
there and skip the test.

Refs jquerygh-1573
Refs jquerygh-1786
Refs jquery/jquery.com#108
Closes jquerygh-2458
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@mgol mgol

Labels

Ajax

Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@mgol @jaubourg @jquerybot