Page MenuHomePhabricator

Wrap jQuery AJAX callbacks in try..catch via mw.errorLogging
Open, Stalled, LowPublic2 Story Points

Description

Split out from the general async callback wrapping task as this part proved a lot harder and the upstream code is outdated and not very useful.

See these comments for details.

Related Objects

StatusAssignedTask
OpenNone
OpenNone
OpenTgr
OpenNone
OpenTgr
StalledNone
ResolvedTgr
ResolvedGilles
OpenTgr
ResolvedTgr
Resolvedcsteipp
ResolvedTgr
DeclinedTgr
StalledTgr
ResolvedTgr
ResolvedTgr
ResolvedTgr
ResolvedTgr
ResolvedTgr
OpenNone
Resolvedjcrespo
ResolvedAklapper
ResolvedTgr
ResolvedTgr

Event Timeline

Tgr created this task.Mar 10 2015, 10:09 AM
Tgr updated the task description. (Show Details)
Tgr raised the priority of this task from to Needs Triage.
Tgr claimed this task.
Tgr added a subscriber: Tgr.
Restricted Application added a project: Multimedia. · View Herald TranscriptMar 10 2015, 10:09 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 195826 had a related patch set uploaded (by Gergő Tisza):
[TEST] Wrap arguments to $.Callbacks.add in try..catch

https://gerrit.wikimedia.org/r/195826

Tgr added a comment.Mar 11 2015, 2:11 AM

The easy but invasive way to do this is to replace $.Callbacks.add() (which will affect at least all promise and animation operations in jQuery); the hard but more targeted way is to decorate $.ajax() and modify the promise it returns, but then we also need to modify any promise that returns (via ˙.promise() and .then`), and so on.

Ori suggested that vbench can be used to check the performance penalty of the first version.

Tgr set Security to None.Mar 11 2015, 2:12 AM
Tgr edited a custom field.
Tgr renamed this task from Wrap jQuery AJAX callbacks in try..catch via mw.eventLogging to Wrap jQuery AJAX callbacks in try..catch via mw.errorLogging.Mar 13 2015, 2:36 AM
Tgr triaged this task as High priority.Mar 14 2015, 12:54 AM
Tgr added a comment.Mar 19 2015, 10:15 PM

A third alternative is to replace the XHR object with a proxy (that is possible via the xhr property of the AJAX options object) and use that to wrap the callback. That's the most targeted of the three approaches, but somewhat fragile as there is no way to detect when properties of the proxy object are changed (ES6 proxies are only supported by Firefox at this time; defineProperty setters only work with known property names and using them on a native object might result in errors; __defineSetter__ is reasonably well-supported (all major browsers except IE 8-10) but non-standard and again might behave unexpectedly on a native object).

Tgr added a comment.Mar 19 2015, 10:36 PM

The two approaches that seems the most useful for XHR proxying:

  • bail out on browsers which do not support Object.defineProperty (basically just IE 8); define a setter for xhr.onreadystatechange. Worst case this might result on attempts to set the value (and thus all AJAX calls) to error out, but that seems unlikely.
  • bail out on browsers which do not support __defineSetter__ (IE 8-10); replace the XHR object with a proxy which uses __defineSetter__ to forward any property changes of the proxy to the real XHR object; set up proxy functions for all methods of the real XHR object. Whenever the real onreadystatechange is called, mirror all properties of the real XHR object to the proxy. This avoids messing with the native object, but involves much more faking which will probably break if someone else tries to mess with the native object.
Tgr added a comment.EditedMar 20 2015, 1:34 AM
In T92247#1133722, @Tgr wrote:

define a setter for xhr.onreadystatechange

Unfortunately this breaks in Chrome. This works as expected:

var xhr = $.ajaxSettings.xhr();
xhr.open( 'GET', '/w/api.php?action=query&format=json&meta=siteinfo', true );
xhr.send();
Object.defineProperty( xhr, 'onreadystatechange', { configurable: true, enumerable:true, value: mw.log, writable: true } );

This does not work, even though inspection shows xhr.onreadystatechange to have the correct value:

var xhr = $.ajaxSettings.xhr();
xhr.open( 'GET', '/w/api.php?action=query&format=json&meta=siteinfo', true );
xhr.send();
Object.defineProperty( xhr, 'onreadystatechange', { configurable: true, enumerable:true, get: $.noop, set: $.noop } );
Object.defineProperty( xhr, 'onreadystatechange', { configurable: true, enumerable:true, value: mw.log, writable: true } );

(reported upstream: #469024)

Tgr added a comment.Mar 20 2015, 2:18 AM
In T92247#1133722, @Tgr wrote:

replace the XHR object with a proxy which uses __defineSetter__

__defineSetter__ also only works on specified property names; must have confused it with PHP magic functions. So there is no point in using it; defineProperty does the same and has better support.

Change 198178 had a related patch set uploaded (by Gergő Tisza):
Make AJAX callback test harder to pass

https://gerrit.wikimedia.org/r/198178

Change 198183 had a related patch set uploaded (by Gergő Tisza):
[WIP] Replace XHR in $.ajax with a proxy object with error catching

https://gerrit.wikimedia.org/r/198183

Change 198178 merged by jenkins-bot:
Make AJAX callback test harder to pass

https://gerrit.wikimedia.org/r/198178

Tgr changed the task status from Open to Stalled.EditedMar 25 2015, 3:15 AM

Discussed with Ori and agreed to abandon this for now, concentrate on the easiest + safest part (window.onerror) and see how much ground that covers. (Originally I believed it to be blocked by the CORS issues but actually for most browsers on most pageviews scripts are loaded from localStorage so there are no cross-domain limitations. And while that won't cover all browsers, or all pageviews on modern browsers, the ones covered will be more valuable than the ones not, due to T90524 and the lack of source map support.)

Change 195826 abandoned by Gergő Tisza:
[TEST] Wrap arguments to $.Callbacks.add in try..catch

Reason:
Abandoning, see T92247 #1147500. Might be revisited later.

https://gerrit.wikimedia.org/r/195826

Change 198183 abandoned by Gergő Tisza:
[WIP] Replace XHR in $.ajax with a proxy object with error catching

Reason:
Abandoning, see T92247#1147500. Might be revisited later.

https://gerrit.wikimedia.org/r/198183

Aklapper lowered the priority of this task from High to Low.Dec 18 2016, 8:47 AM