Make WordPress Core

Opened 7 years ago

Last modified 7 years ago

#42794 new defect (bug)

Can't upload new media files when multiple media dialogues are present on a single page

Reported by: carlobeltrame's profile carlobeltrame Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.7.19
Component: Media Keywords: has-patch has-screenshots
Focuses: javascript Cc:

Description

Layman error description: When there are more than one Inline Media Uploader on a page in the Admin, and one tries to upload a new image or file to the Media Library, the upload gets stuck and the image is never inserted into the media library (see attached screenshot). This happens to me when using the post-types-definitely plugin to create multiple media fields in a CPT. The exact same error (including error messages) has been described before (https://wordpress.org/support/topic/js-bug-prevents-uploading-media-in-pagepost-editor/?replies=11#post-6860868 and https://wordpress.org/support/topic/error-uploading-media-in-post-if-yoast-is-active/#post-6922117), and even though there it was related to a plugin combination, it is not in my case. The workaround proposed there is not applicable to my case and the error source I found has nothing to do with plugins.

Technical error description: When examined in the Firefox Developer Tools, the console shows the error TypeError: this.$index is undefined. In Chrome, the error is Cannot read property 'text' of undefined. Both of them point to wp-includes/js/media-views.js:UploaderStatus.info() https://core.trac.wordpress.org/browser/trunk/src/wp-includes/js/media-views.js?rev=40359#L8361

Technical analysis: Upon closer inspection of the code, the problem seems to be as follows: When we upload a file (by clicking Ok on the OS file dialogue), a callback handler calls info() on each UploadStatus object belonging to a loaded media dialogue. The method info() relies on the fact that normally, the method ready() on the same UploadStatus object is called before the call to info(). ready() uses JQuery to set (amongst others) the member variable $index inside the UploadStatus object. info() then tries to access this.$index on all UploadStatus objects and set their contents using the JQuery text() function. However, since ready() is not called before a media dialogue is opened for the first time, and our opened dialogue calls info() on all media dialogues, the access to this.$index fails in the unopened UploadStatus objects, throwing the above Javascript Error and making the script abort, therefore freezing the upload progress bar.

Workarounds: This analysis is confirmed by the fact that when I open all media dialogues on the page at least once before uploading an image in any of them, uploading works as expected (because all of the UploadStatuses are ready() and have their $index member variable defined). A workaround for the problem is to just use the inline media dialogues for selecting existing media from the media library, and using the Media->New menu to actually upload any files. The error happened in all Wordpress versions I tested (since 4.8.3).

Proposed solution: I can see a number of different solutions for this problem. The best one would be to only call info() on the UploadStatus belonging to the opened media dialogue. However, the info() calls are performed via a single event trigger (https://core.trac.wordpress.org/browser/trunk/src/wp-includes/js/media-views.js?rev=40359#L8289) that all info() methods subscribe to in the UploadStatus.initialize() function (relevant for this bug is the "add" trigger), and this trigger cannot distinguish its subscribers. Therefore I propose to instead call ready() at the end of the initialize() function. This way, ready() is only called once more than before per media dialogue and page load, and we can be certain that info() is only called on UploadStatuses that are ready().

Attachments (4)

upload-error-chrome.png (340.1 KB) - added by carlobeltrame 7 years ago.
Screenshot of the frozen uploader including the Javascript console showing the error
wp-patch-uploadstatus.diff (481 bytes) - added by carlobeltrame 7 years ago.
Proposed patch
minimal-working-example.php (2.0 KB) - added by carlobeltrame 7 years ago.
MWE for testing the bug and the proposed fix
Screenshot from 2018-01-19 10-39-52.png (73.2 KB) - added by carlobeltrame 7 years ago.
Screenshot right before opening the media modal

Download all attachments as: .zip

Change History (10)

@carlobeltrame
7 years ago

Screenshot of the frozen uploader including the Javascript console showing the error

@carlobeltrame
7 years ago

Proposed patch

#1 @carlobeltrame
7 years ago

  • Keywords has-patch dev-feedback has-screenshots added

#2 @carlobeltrame
7 years ago

  • Keywords dev-feedback removed

#3 @carlobeltrame
7 years ago

@flixos90 since I discovered this bug while using your definitely-plugins, would you be so kind to review this bug and the fix I proposed? Thanks!

#4 @carlobeltrame
7 years ago

  • Version changed from 4.8.2 to 3.7.19

Is there anything standing in the way of merging this bug fix? Judging by the SVN blame, this bug has existed since at least version 3.7.16 in Spetember 2015.
In the meantime, people who have the same problem can fix it by adding this to their plugin or theme functions.php file:

function gloggi_fix_media_uploader() {
?><script>
    var oldUploaderStatus = wp.media.view.UploaderStatus;
    wp.media.view.UploaderStatus = oldUploaderStatus.extend({
       initialize: function() {
           oldUploaderStatus.prototype.initialize.call(this);
           this.ready();
       }
    });
</script><?php
}
function gloggi_add_media_modification_js() {
    add_action( 'admin_print_footer_scripts', 'gloggi_fix_media_uploader' );
}
add_action( 'wp_enqueue_media', 'gloggi_add_media_modification_js' );

#5 @adamsilverstein
7 years ago

@carlobeltrame Welcome to trac and thanks for the detailed bug report.

I'd like to try to reproduce the error so i can do some digging and validate the fix. Can you provide the code snippet you are using for post-types-definitely to get the multiple image uploaders in a CPT? Also, maybe a screenshot of your screen before you open the media modal so i can see that as well.

@carlobeltrame
7 years ago

MWE for testing the bug and the proposed fix

@carlobeltrame
7 years ago

Screenshot right before opening the media modal

#6 @carlobeltrame
7 years ago

Sure! I have attached the requested files. If you need anything else, feel free to ask.

Note: See TracTickets for help on using tickets.