Page MenuHomePhabricator

Find a way to detect unhandled jQuery promise rejections in VisualEditor
Open, MediumPublic

Description

Steps to reproduce:

  1. Run this code in Chromium or Firefox with the debugger open:
var prom = Promise.resolve();
prom.then( function () {
        console.log( 'A' );
} ).then( function () {
        console.log( 'B' );
} ).then( function () {
        throw new Error( 'Foo' );
} ).then( function () {
        console.log( 'C' );
} ).catch( function ( ex ) { 
        console.log( 'Caught', ex );
} ).then( function () {
        console.log( 'D' );
} ).then( function () {
        throw new Error( 'Bar' );
} ).then( function () {
        console.log( 'E' );
} ).then( function () {
        console.log( 'Caught', ex );
} );
  1. Observe that the debugger does nothing special with the handled rejection Foo, but it reports the unhandled rejection 'Bar' (either breaking on it or logging it to console).
  1. Now, try the same again but replacing the native Promise.resolve() with jQuery's $.Deferred().resolve().

Expected behaviour:

Error detected: the debugger draws attention to the unhandled promise rejection Bar (but not the handled promise rejection Foo), whether in the same way as native promises or in some other way.

Observed behaviour:

Silent failure: the debugger does nothing special with the unhandled promise rejection Bar (other than the manually inserted console.log statement).

Discussion

Silently failing unhandled jQuery promise rejections are causing trouble in VisualEditor development. They occur fairly frequently, and waste developer time either through extra bug-hunting distraction or (in the worst case) deployed errors. The obvious options are:

  • Some general, clean mechanism at the jQuery level. I tried a few things with @Krinkle and it seems this may be infeasible right now. A fully general unhandled promise rejection event at the jQuery level has been requested and declined: https://github.com/jquery/jquery/issues/2908 .
  • Some clever hack that works for our purposes. (Perhaps hacking the jQuery code to mirror jQuery promises with native ones, in debug mode only?)
  • Give up: just try to detect unhandled promise rejections by heroically good code review.

Event Timeline

Change 539074 had a related patch set uploaded (by Divec; owner: Divec):
[VisualEditor/VisualEditor@master] WIP POC DONTMERGE NOREALLYDONT make jQuery promises fail loudly

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

JTannerWMF added a subscriber: JTannerWMF.

I am moving this to Current Work since @dchan is working on it

You can override jQuery.Deferred.exceptionHook. That will register all exceptions, not just unhandled ones (exceptions/errors only, not rejections), which is noisy but might still be useful for debug mode.

The technique in https://gerrit.wikimedia.org/r/539074 does work. And I've used that patchset a few times now to debug silent errors. But it's perhaps too hacky to merge even for debug mode only (it shadows each jQuery promise with a native promise). Maybe it could be merged but needing specific activation.

Removing task assignee due to inactivity, as this open task has been assigned for more than two years. See the email sent to the task assignee on February 06th 2022 (and T295729).

Please assign this task to yourself again if you still realistically [plan to] work on this task - it would be welcome.

If this task has been resolved in the meantime, or should not be worked on ("declined"), please update its task status via "Add Action… 🡒 Change Status".

Also see https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup for tips how to best manage your individual work in Phabricator.

@dchan Perhaps VisualEditor would make a good candidate to adopt in a highly-visible manner native use of Promise (with the es6-polyfills module as conditionally loaded polyfill for old Safari and old IE). It seems like that would effectively solve this issue.

As for MediaWiki more generally, upsteam jQuery already logs likely application errors to the console for visibility but this does not intergrate with devtools indeed, and to avoid noise from reasonably thrown and timely caught internal error objects it indeed does not log for plain Error and other app-custom error classes.

var prom = $.Deferred().resolve();
prom.then( function () {
} ).then( function () {
} ).then( function () {
        boom();
} ).then( function () {
} ).catch( function ( ex ) { 
} ).then( function () {
} ).then( function () {
        throw new Error( 'Bar' );
} ).then( function () {
} ).then( function () {
} );

[warning] jQuery.Deferred exception: boom is not defined
at ...

I'd be open to an improved version of jQuery.Deferred.exceptionHook that logs all errors if-and-only-if there is no catch handler by the time of the next tick. I don't know how do-able that would be, but that's essentially what native promises do and seems useful to approximate, but also likely non-trivial to do. As said, it may be more interesting for VisualEditor to look ahead to native Promises instead.

Note that you will be automatically insulated from use of Deferred in other APIs since these are interchangable and Promise-compatible, including in chained thenables, which will result in the same native debugging and logging, so long as VE's involvement in the chain somewhere uses a native Promise, it all works out. That is, you get immediate positive value even if other parts of our stack don't make the switch at the same time.

var prom = Promise.resolve(); // outer is native
prom.then( function () {
} ).then( function () {
        return $.Deferred().resolve();
} ).then( function () {
        return $.Deferred().reject( new Error( 'Foo' ) ); // observed and logged by native
} ).then( function () {
        return $.Deferred().resolve();
} ).catch( function ( ex ) { 
} ).then( function () {
        return $.Deferred().resolve();
} ).then( function () {
        return $.Deferred().reject( new Error( 'Foo' ) );
} ).then( function () {
        return $.Deferred().resolve();
} ).then( function () {
} );

[error] Uncaught (in promise) Error: Foo

var prom = $.Deferred().resolve(); // starts with Deferred
prom.then( function () {
} ).then( function () {
        return $.Deferred().resolve();
} ).then( function () {
        return $.Deferred().reject( new Error( 'Foo' ) );
} ).then( function () {
        return $.Deferred().resolve();
} ).catch( function ( ex ) { 
} );

Promise.resolve( prom ) // wrapped by native, will observe previous Error
.then( function () {
        return $.Deferred().resolve();
} ).then( function () {
        return $.Deferred().reject( new Error( 'Foo' ) );
} ).then( function () {
        return $.Deferred().resolve();
} ).then( function () {
} );

[error] Uncaught (in promise) Error: Foo

Krinkle triaged this task as Medium priority.Mar 14 2022, 3:24 PM
Krinkle moved this task from Inbox to Backlog on the MediaWiki-ResourceLoader board.
Krinkle removed a project: Patch-Needs-Improvement.
Krinkle moved this task from Inbox to Backlog: Maintenance on the Performance-Team board.