Opened 23 months ago

Closed 23 months ago

Last modified 23 months ago

#15198 closed bug (duplicate)

Fails to guard against zero-length array

Reported by: joemabel Owned by: joemabel
Priority: minor Milestone: none
Component: ui.dialog Version: 1.12.1
Keywords: Cc:
Blocked by: Blocking:

Description

Apologies for no jFiddle etc. here, this was in a program of 100,000+ lines of JS; I know exactly the fix needed, it's a simple piece of defensive programming.

In _createOverlay: function(), you have code as follows:

    if ( !this._allowInteraction( event ) ) {
        event.preventDefault();
        this._trackingInstances()[ 0 ]._focusTabbable();
    }

There are definitely circumstances where this._trackingInstances() can be an empty array, so this should be something like:

    if ( !this._allowInteraction( event ) ) {
        event.preventDefault();
        if (this._trackingInstances().length) { 
            this._trackingInstances()[ 0 ]._focusTabbable();
        }
    }

It might be better to put the return of this._trackingInstances() in a local variable to avoid calling it twice; I leave that to you. But it is definitely possible for the array returned to be an empty array, in which case this._trackingInstances()[ 0 ]._focusTabbable() will certainly throw an error in the existing code.

Change History (3)

comment:1 Changed 23 months ago by Scott González

Owner: set to joemabel
Status: newpending

Without a use case that creates a problem, we do not considered this a bug. You can't just say "There are definitely circumstances where this._trackingInstances() can be an empty array" and expect it to be taken as truth.

comment:2 Changed 23 months ago by Scott González

Resolution: duplicate
Status: pendingclosed

Duplicate of #15182.
It would have been better to report the actual bug that you encountered. Compare the difference between the wording in this ticket, which describes what could be a hypothetical problem, to the wording in #15182 which actually states that the error does occur, along with reproduction steps. That ticket was clearly a bug, where this sounded like a theoretical problem. Also note that the fix didn't require us checking for _trackingInstances() returning an empty array, because that's an invalid state.

comment:3 Changed 23 months ago by joemabel

Then feel perfectly free not to do obvious defensive programming. I don't understand enough about this to know what circumstances trigger it, and I'm writing as a user. In the jquery-ui code, it's obvious _trackingInstances() can return an empty array, and _createOverlay: function(), which calls it, does not check for that return.

Your site says you welcome bug reports, particularly with proposed fixes. I've patched it on our system. If no one else cares, then it's not my issue.

Version 0, edited 23 months ago by joemabel (next)
Note: See TracTickets for help on using tickets.