whatwg / xhr

XMLHttpRequest Standard
https://xhr.spec.whatwg.org/
Other
315 stars 131 forks source link

The upload listener flag is not respected in browsers #95

Closed berniegp closed 7 years ago

berniegp commented 8 years ago

Step 4 of send() states:

5) If one or more event listeners are registered on the associated XMLHttpRequestUpload object, then set upload listener flag.

Therefore, if any event listeners are added after send(), they should be ignored. I find this counter-intuitive; why prevent later event handlers from receiving events? In any case, the browser behavior (tested in latest Firefox and Chrome) does not match the spec.

Form what I can see, the upload listener flag is probably only used to set the use-CORS-preflight flag. I suggest removing mentions of the upload listener flag where it determines whether to fire upload progress events.

annevk commented 8 years ago

Do you have a testcase? This change was made in 667d4f3604aaee4e24bde33ed487cf8a98274e70 to align with implementations. Note that it only prevents later added listeners from receiving events if it wasn't set at all and only for listeners on the XMLHttpRequestUpload object.

berniegp commented 8 years ago

Here is my test case:

// Returns an array which contains all events fired by the xhr
function recordEvents(xhr) {
    var xhrEvents = [
        'loadstart',
        'progress',
        'abort',
        'error',
        'load',
        'timeout',
        'loadend',
    ];
    var events = [];
    var recordEvent = function(e, prefix) {
        prefix = prefix ? 'upload.' : '';
        events.push(prefix + e.type + '(' + e.loaded + ',' + e.total +
            ',' + e.lengthComputable +')');
    };
    var recordUploadEvent = function(event) {
        recordEvent(event, 'upload');
    };
    for (var i = 0; i < xhrEvents.length; i++) {
        xhr.addEventListener(xhrEvents[i], recordEvent);
        xhr.upload.addEventListener(xhrEvents[i], recordUploadEvent);
    }
    xhr.addEventListener('readystatechange', function() {
        events.push('readystatechange(' + this.readyState + ')');
    });
    return events;
}
var xhr;

// Test-case 1: effect of the upload listener flag on upload progress events
xhr = new XMLHttpRequest();
xhr.open('POST', './');
xhr.send('body!');
// listeners added AFTER send() -> upload listener flag === false
var events1 = recordEvents(xhr);

// Test-case 2: effect of the upload listener flag on upload progress events during abort()
xhr = new XMLHttpRequest();
xhr.open('POST', './');
xhr.send('body!');
// listeners added AFTER send() -> upload listener flag === false
var events2 = recordEvents(xhr);
xhr.abort();

// Wait until requests are done
setTimeout(function() {
    console.log(events1, events2);
}, 1000);

Results in Firefox:

[ "upload.progress(5,5,true)", "upload.load(5,5,true)", "upload.loadend(5,5,true)", "readystatechange(2)", "readystatechange(3)", "progress(1589,1589,true)", "readystatechange(4)", "load(1589,1589,true)", "loadend(1589,1589,true)" ]
[ "readystatechange(4)", "abort(0,0,false)", "loadend(0,0,false)", "upload.abort(0,5,true)", "upload.loadend(0,5,true)" ]

Results in Chrome:

["readystatechange(2)", "readystatechange(3)", "progress(1589,1589,true)", "readystatechange(4)", "load(1589,1589,true)", "loadend(1589,1589,true)"]
["readystatechange(4)", "upload.progress(0,0,false)", "upload.abort(0,0,false)", "upload.loadend(0,0,false)", "progress(0,0,false)", "abort(0,0,false)", "loadend(0,0,false)"]
annevk commented 8 years ago

Hmm, so what happens in the case of a cross-origin redirect? I'm guessing the cross-origin redirect would simply fail then...

tyoshino commented 8 years ago

A few days before the OP, I found bugs regarding upload in Chromium and am fixing them https://codereview.chromium.org/2427553002/.

berniegp commented 8 years ago

@annevk I don't know what happens for cross-origin redirects. I'm not familiar with those so I do not really know how to test them.

annevk commented 8 years ago

@berniegp basically when you fetch https://here.example/test.html and that redirects to https://elsewhere.example/ would be a cross-origin redirect.

annevk commented 7 years ago

I created https://github.com/w3c/web-platform-tests/pull/5122 for this. It seems Firefox does indeed fail the scenario I mentioned (Safari TP does too). Chrome fails a different test.

I haven't investigated the failures in depth, but I think we should keep the standard as-is given these results. I'll file browser bugs though and see what people say.

annevk commented 7 years ago

Bugs filed: